-
Notifications
You must be signed in to change notification settings - Fork 669
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
Change retry error from RuntimeError to FlyteRecoverableException #5128
Conversation
Thank you for opening this pull request! 🙌 These tips will help get your PR across the finish line:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5128 +/- ##
==========================================
- Coverage 59.03% 59.01% -0.02%
==========================================
Files 645 645
Lines 55726 55726
==========================================
- Hits 32896 32888 -8
- Misses 20233 20240 +7
- Partials 2597 2598 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Daniel Sola <[email protected]>
471c8f1
to
82c82b1
Compare
Thanks for opening this! For your ref link to work, I think you'll need to add (Docs are not in great shape right now, so updating them is not straightforward, but we're working on a big project to overhaul them and make them easier to contribute to.) |
Signed-off-by: Daniel Sola <[email protected]>
Signed-off-by: Daniel Sola <[email protected]>
Congrats on merging your first pull request! 🎉 |
Tracking issue
https://unionai.atlassian.net/browse/DOCS-385
Why are the changes needed?
It turns out that all user exceptions are considered non-recoverable unless the exception is a subclass of FlyteRecoverableException (Tasks - Flyte).
This means that the RuntimeError in the retry example here will not actually retry.
We want to change the error in the example to one which will trigger a retry of the task if the decorator is used.
Slack convo: https://unionai.slack.com/archives/C06RE4ZR5QQ/p1711576283580859
What changes were proposed in this pull request?
As only user exceptions that are subclasses of
FlyteRecoverableException
are compatible with retries,FlyteRecoverableException
is used rather than aRuntimeError
.How was this patch tested?
This workflow was ran various times with
RuntimeError
(version CT3eWXJm_xOmb73ko1ww1w) and withFlyteRecoverableException
(version d7nmtvMO6afNxNs4WFbTqA). WithRuntimeError
no retires were triggered, while withFlyteRecoverableException
retries were triggered.Screenshots
Check all the applicable boxes
Related PRs
Docs link