-
Notifications
You must be signed in to change notification settings - Fork 291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Core feature] Flytekit should support unsafe
mode for types
#2419
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Mecoli1219 <[email protected]>
Signed-off-by: Mecoli1219 <[email protected]>
Signed-off-by: Mecoli1219 <[email protected]>
return t1(a=a) | ||
|
||
@workflow | ||
def wf1_wo_unsafe2(a: int) -> int: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you test this workflow in the sandbox cluster?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I only test it on my local environment. Can I ask how to try it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR can help you, you can either use imageSpec or Dockerfile to create your own image.
#1870
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from flytekit import task, Secret, workflow, ImageSpec
flytekit_dev_version = "https://github.com/Mecoli1219/flytekit.git@c7c9a2ad2bdf6799ec324955a023281cca030038"
image = ImageSpec(
registry="localhost:30000",
platform="linux/amd64",
apt_packages=["git"],
packages=[
f"git+{flytekit_dev_version}",
],
)
@task(unsafe=True, container_image=image)
def t1(a) -> int:
if type(a) == int:
return a + 1
return 0
@workflow(unsafe=True)
def wf1_with_unsafe(a) -> int:
return t1(a=a)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above code will fail. The reason is detailed in flyteorg/flyte#5261.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from flytekit import task, Secret, workflow, ImageSpec
from typing import Tuple
flytekit_dev_version = "https://github.com/Mecoli1219/flytekit.git@9885189b2af95c0c0dfa94ff92b71e865d95cf80"
image = ImageSpec(
registry="localhost:30000",
platform="linux/amd64",
builder="fast-builder",
apt_packages=["git"],
packages=[
f"git+{flytekit_dev_version}",
],
)
@task(unsafe=True, container_image=image)
def t1(a) -> int:
if type(a) == int:
return a + 1
return 0
@workflow
def wf1_with_unsafe() -> Tuple[int, int, int]:
a1 = t1(a=1)
a2 = t1(a="1")
a3 = t1(a=None) #! This will fail
return a1, a2, a3
Currently, the input with a value None
will fail, but the issue is solved with this PR (flyteorg/flyte#5408).
Signed-off-by: Mecoli1219 <[email protected]>
Signed-off-by: Mecoli1219 <[email protected]>
Signed-off-by: Mecoli1219 <[email protected]>
Signed-off-by: Mecoli1219 <[email protected]>
@pingsutw I created 2 new unit tests, and both of them passed. |
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2419 +/- ##
===========================================
+ Coverage 46.48% 74.99% +28.51%
===========================================
Files 199 279 +80
Lines 20707 24265 +3558
Branches 2665 2679 +14
===========================================
+ Hits 9625 18197 +8572
+ Misses 10610 5341 -5269
- Partials 472 727 +255 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Mecoli1219 <[email protected]>
flytekit/core/task.py
Outdated
@@ -130,6 +130,7 @@ def task( | |||
pod_template: Optional["PodTemplate"] = ..., | |||
pod_template_name: Optional[str] = ..., | |||
accelerator: Optional[BaseAccelerator] = ..., | |||
unsafe: bool = ..., |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to have a more explicit name. Something like:
unsafe: bool = ..., | |
pickle_untyped: bool = ..., |
Signed-off-by: Mecoli1219 <[email protected]>
Signed-off-by: Mecoli1219 <[email protected]>
Signed-off-by: Mecoli1219 <[email protected]>
@@ -112,6 +112,7 @@ def __init__( | |||
node_dependency_hints: Optional[ | |||
Iterable[Union["PythonFunctionTask", "_annotated_launch_plan.LaunchPlan", WorkflowBase]] | |||
] = None, | |||
pickle_untyped: bool = False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kumare3 Since you proposed the safe
parameter name, are you okay with using pickle_untyped
?
My concerned is that safe=False
could mean so many unsafe behavior. pickle_untyped
is explicit about the behavior.
flytekit/types/pickle/pickle.py
Outdated
uri = lv.scalar.blob.uri | ||
return FlytePickle.from_pickle(uri) | ||
|
||
def to_literal(self, ctx: FlyteContext, python_val: T, python_type: Type[T], expected: LiteralType) -> Literal: | ||
if python_val is None: | ||
raise AssertionError("Cannot pickle None Value.") | ||
# raise AssertionError("Cannot pickle None Value.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes sure. I'll remove that recently
Signed-off-by: Mecoli1219 <[email protected]>
Signed-off-by: Mecoli1219 <[email protected]>
Signed-off-by: Mecoli1219 <[email protected]>
unsafe
mode for typesunsafe
mode for types
unsafe
mode for typesunsafe
mode for types
NVM, it is still the same problem as mentioned in the comment above 6 months ago. @task(pickle_untyped=True, container_image=image)
def t1(a) -> int:
if type(a) == int:
return a + 1
return 0
@workflow
def wf1_with_unsafe() -> Tuple[int, int, int]:
a1 = t1(a=1)
a2 = t1(a="1")
a3 = t1(a=None) #! This will fail
return a1, a2, a3 Currently, the input with a value |
Signed-off-by: Mecoli1219 <[email protected]>
I revert the support for |
Tracking issue
Related to flyteorg/flyte#5319
Why are the changes needed?
Use Case: When the user wants to apply Flytekit to their Python codebase, but annotating the type is too hard or time-consuming, we should support
unsafe
mode for easy usage.What changes were proposed in this pull request?
unsafe
kwarg to bothtask
andworkflow
, and the default value ofunsafe
isFalse
unsafe
kwarg isTrue
and the types of inputs or outputs are not specified (which is None), replace the type from None to Optional[Any]. This will triggerFlytePickleTransformer
to handle each unspecified type.How was this patch tested?
Two tests are created to examine the ability of
unsafe
kwarg.test_unsafe_input_wf_and_task
: This test mainly examines the unspecified input types. If theunsafe
is False, the workflow or task should throw errors. Otherwise, it should succeed.test_unsafe_wf_and_task
: This test mimics the use case mentioned above, where the user might not specify all the types in their codebase. The workflow should succeed if theunsafe
kwarg of all tasks and workflow are set toTrue
.Setup process
Screenshots
Check all the applicable boxes
Related PRs
flyteorg/flyte#5319
Docs link