-
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
Fix: Improve error handling in workflow compilation when output binding fails #2047
Conversation
Signed-off-by: Fabio Graetz <[email protected]>
Signed-off-by: Fabio Graetz <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2047 +/- ##
==========================================
- Coverage 85.98% 82.53% -3.46%
==========================================
Files 308 313 +5
Lines 22946 23602 +656
Branches 3468 3535 +67
==========================================
- Hits 19731 19479 -252
- Misses 2615 3498 +883
- Partials 600 625 +25 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Fabio Graetz <[email protected]>
Signed-off-by: Fabio Graetz <[email protected]>
Signed-off-by: Fabio Graetz <[email protected]>
flytekit/core/workflow.py
Outdated
) | ||
bindings.append(b) | ||
except Exception as e: | ||
raise AssertionError(f"Failed to bind output {output_names[0]} for function {self.name}: {e}") from e |
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.
@wild-endeavor should these be AssertionErrors? or FlyteExceptions?
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 used AssertionError
in analogy to how it is done here but I'm happy to change to FlyteException
if you prefer.
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.
Let's do it like this 50ac42f?
one comment else lgtm |
Signed-off-by: Fabio Grätz <[email protected]>
Signed-off-by: Fabio Grätz <[email protected]>
Signed-off-by: Fabio Grätz <[email protected]>
Why are the changes needed?
When registering this workflow, one gets the following obvious error:
However, the exception does not hint which workflow/output is causing the error. In a large workflow with numerous sub workflows and dozens of tasks, it took one of our ML engineers quite some time to figure out where exactly the
return
was missing. They asked whether we could make this simpler.What changes were proposed in this pull request?
I propose to catch errors raised by
binding_from_python_std
in thePythonFunctionWorkflow
'scompile
function just as is done e.g. here.This way, finding the error would have been a no-brainer.
How was this patch tested?
For the example workflow above, the error message now is:
Check all the applicable boxes