-
Notifications
You must be signed in to change notification settings - Fork 260
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 <michaellai901026@gmail.com>
Signed-off-by: Mecoli1219 <michaellai901026@gmail.com>
Signed-off-by: Mecoli1219 <michaellai901026@gmail.com>
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 <michaellai901026@gmail.com>
Signed-off-by: Mecoli1219 <michaellai901026@gmail.com>
Signed-off-by: Mecoli1219 <michaellai901026@gmail.com>
Signed-off-by: Mecoli1219 <michaellai901026@gmail.com>
@pingsutw I created 2 new unit tests, and both of them passed. |
Signed-off-by: Kevin Su <pingsutw@apache.org>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2419 +/- ##
===========================================
+ Coverage 52.89% 76.17% +23.28%
===========================================
Files 297 182 -115
Lines 23957 18574 -5383
Branches 3655 3658 +3
===========================================
+ Hits 12671 14149 +1478
+ Misses 11178 3796 -7382
- Partials 108 629 +521 ☔ View full report in Codecov by Sentry. |
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