-
Notifications
You must be signed in to change notification settings - Fork 301
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
Improve error message when omitting task kwarg in workflows #1921
Conversation
Signed-off-by: Fabio Grätz <[email protected]>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1921 +/- ##
==========================================
+ Coverage 54.71% 54.78% +0.06%
==========================================
Files 305 306 +1
Lines 22732 22782 +50
Branches 3453 3454 +1
==========================================
+ Hits 12438 12481 +43
- Misses 10122 10129 +7
Partials 172 172
☔ View full report in Codecov by Sentry. |
"supports a subset of Python’s semantics. When calling tasks, all kwargs have to be provided." | ||
) | ||
|
||
raise _user_exceptions.FlyteAssertion(error_msg) |
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.
In the review, it would be good to think whether the if statement if isinstance(entity, Task) and _default is not None:
is not covering cases where this error message should be added or does cover cases where it shouldn't be added.
I tested the following:
@task
def foo(bar: int = 1) -> int: # default value that doesn't work
return bar
@workflow
def wf() -> int:
return foo()
flytekit.exceptions.user.FlyteAssertion: Error encountered while executing 'wf':
Input bar of type <FlyteLiteral simple: INTEGER> was not specified for function test.foo.
Flyte workflow syntax is a domain-specific language (DSL) for building execution graphs which supports a subset of Python’s semantics. When calling tasks, all kwargs have to be provided.
@task
def foo(bar: int) -> int: # No default value
return bar
@workflow
def wf() -> int:
return foo()
flytekit.exceptions.user.FlyteAssertion: Error encountered while executing 'wf':
Input bar of type <FlyteLiteral simple: INTEGER> was not specified for function test.foo
The issue is not a limitation of DSL vs python but actually no value provided for the kwarg.
@task
def foo(bar: int) -> int:
return bar
@workflow
def sub_wf(bar: int=1) -> int:
return foo(bar=bar)
@workflow
def wf() -> int:
return sub_wf()
No error, as expected.
@task
def foo(bar: int) -> int:
return bar
@workflow
def sub_wf(bar: int) -> int: # no default value
return foo(bar=bar)
@workflow
def wf() -> int:
return sub_wf()
flytekit.exceptions.user.FlyteAssertion: Error encountered while executing 'wf':
Encountered error while executing workflow 'sub_wf':
Input bar of type <FlyteLiteral simple: INTEGER> was not specified for function sub_wf
- Now with dynamic
@task
def foo(bar: int) -> int:
return bar
@dynamic
def sub_wf(bar: int) -> int: # no default value
return foo(bar=bar)
@workflow
def wf() -> int:
return sub_wf()
flytekit.exceptions.user.FlyteAssertion: Error encountered while executing 'wf':
Input bar of type <FlyteLiteral simple: INTEGER> was not specified for function test.sub_wf
@task
def foo(bar: int) -> int:
return bar
@dynamic
def sub_wf(bar: int=1) -> int: # default value that doesn't work
return foo(bar=bar)
@workflow
def wf() -> int:
return sub_wf()
flytekit.exceptions.user.FlyteAssertion: Error encountered while executing 'wf':
Input bar of type <FlyteLiteral simple: INTEGER> was not specified for function test.sub_wf.
Flyte workflow syntax is a domain-specific language (DSL) for building execution graphs which supports a subset of Python’s semantics. When calling tasks, all kwargs have to be provided.
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.
would it be better to show the python type in the error message? like
Input bar of type [int] was not specified for function test.
we can get the python type from interface.inputs[k]
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.
Good call, thanks!
Signed-off-by: Fabio Grätz <[email protected]>
Signed-off-by: Fabio Grätz <[email protected]>
Signed-off-by: Fabio Grätz <[email protected]>
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.
Thanks, LGTM
…#1921) Signed-off-by: Fabio Grätz <[email protected]> Co-authored-by: Fabio Grätz <[email protected]>
TL;DR
Improve error message when, in a workflow, one omits to pass an optional kwarg to a task.
In python, the following is perfectly valid:
In Flyte, the following workflow (which only adds task/workflow decorators) is not:
The user is confronted with this error message:
Many Flyte users are ML engineers that are not necessarily aware of the following (source):
For these users, the above error message is very confusing and does not give a hint why something that works in python wouldn't work in a flyte workflow.
Since I was asked about this particular limitation by several of our platform users, I propose that we improve the error message so that the error explains itself and users are informed about flyte workflows being a DSL, not python.
Type
Neither of the following but improved error message.
Are all requirements met?
Complete description
NA
Tracking Issue
NA
Follow-up issue
NA