This repository has been archived by the owner on Oct 9, 2023. It is now read-only.
Added IsFailurePermanent flag on DynamicTaskStatus #567
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
12 tasks
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
eapolinario
approved these changes
Jun 30, 2023
Signed-off-by: Daniel Rammer <[email protected]>
eapolinario
pushed a commit
to eapolinario/flytepropeller
that referenced
this pull request
Aug 9, 2023
* added IsFailurePermanent flag on DynamicTaskStatus Signed-off-by: Daniel Rammer <[email protected]> * fixed linter Signed-off-by: Daniel Rammer <[email protected]> * cleaned up retryable vs permanent reporting Signed-off-by: Daniel Rammer <[email protected]> * make generate Signed-off-by: Daniel Rammer <[email protected]> --------- Signed-off-by: Daniel Rammer <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TL;DR
Currently dynamic tasks treat all failures and retryable which results in unintended behavior when subtasks are attempted multiple times. This PR adds a
IsFailurePermanentfield on theDynamicTaskStatusstruct to indicate that the failure is permanent, and therefore should not be recovered.Type
Are all requirements met?
Complete description
The choice to use a new flag is entirely out of backwards compatibility. Other options include:
(1) Using the ExecutionError in flyteplugins io rather than flyteidl ExecutionError: This would make retrieval of in progress dynamic task status fail when it expects a different error type on the field.
(2) Naming the flag
IsRecoverablevsisPermanent: This is difficult because intuition says that we should useIsRecoverablewhich adheres more to the Flyte standard. The biggest issue is that dynamics treat all failures as retryable by default now, so this would change the default to be non-recoverable (ie.falseforIsRecoverable) which changes the default behavior. It is VERY difficult to ensure there will be no regressions.Tracking Issue
fixes flyteorg/flyte#3606
Follow-up issue
NA