Update get_serializable_workflow to consistently generate the same subworkflow order #1011
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
vvasavada-fn
requested review from
wild-endeavor,
kumare3 and
eapolinario
as code owners
|
Thank you for opening this pull request! 🙌 These tips will help get your PR across the finish line:
|
Signed-off-by: Vrinda Vasavada <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1011 +/- ##
==========================================
- Coverage 86.39% 86.26% -0.13%
==========================================
Files 255 255
Lines 24417 24417
Branches 2775 2776 +1
==========================================
- Hits 21095 21064 -31
- Misses 2850 2865 +15
- Partials 472 488 +16 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Vrinda Vasavada <[email protected]>
eapolinario
approved these changes
May 20, 2022
|
Congrats on merging your first pull request! 🎉 |
wild-endeavor
added a commit
that referenced
this pull request
May 20, 2022
Signed-off-by: Yee Hing Tong <[email protected]>
Merged
eapolinario
added a commit
that referenced
this pull request
May 24, 2022
* backport #1011 to 0.26 Signed-off-by: Yee Hing Tong <[email protected]> * update pytest-flyte protocol in requirements file Signed-off-by: Yee Hing Tong <[email protected]> * bump black to 22.3.0 and make fmt Signed-off-by: Yee Hing Tong <[email protected]> * Fix url in sqlite tests Signed-off-by: Eduardo Apolinario <[email protected]> * pip install -r requirements.txt for plugins tests Signed-off-by: Eduardo Apolinario <[email protected]> * delete ci for integration tests Signed-off-by: Yee Hing Tong <[email protected]> Signed-off-by: Eduardo Apolinario <[email protected]> * bump flyteidl to 0.21.11 to pick up cache_serialize change Signed-off-by: Eduardo Apolinario <[email protected]> * Fix docs failures Signed-off-by: Eduardo Apolinario <[email protected]> * Bump version of codecov-action to 1.5.2 Signed-off-by: Eduardo Apolinario <[email protected]> Co-authored-by: Eduardo Apolinario <[email protected]>
reverson
pushed a commit
to reverson/flytekit
that referenced
this pull request
May 27, 2022
* backport flyteorg#1011 to 0.26 Signed-off-by: Yee Hing Tong <[email protected]> * update pytest-flyte protocol in requirements file Signed-off-by: Yee Hing Tong <[email protected]> * bump black to 22.3.0 and make fmt Signed-off-by: Yee Hing Tong <[email protected]> * Fix url in sqlite tests Signed-off-by: Eduardo Apolinario <[email protected]> * pip install -r requirements.txt for plugins tests Signed-off-by: Eduardo Apolinario <[email protected]> * delete ci for integration tests Signed-off-by: Yee Hing Tong <[email protected]> Signed-off-by: Eduardo Apolinario <[email protected]> * bump flyteidl to 0.21.11 to pick up cache_serialize change Signed-off-by: Eduardo Apolinario <[email protected]> * Fix docs failures Signed-off-by: Eduardo Apolinario <[email protected]> * Bump version of codecov-action to 1.5.2 Signed-off-by: Eduardo Apolinario <[email protected]> Co-authored-by: Eduardo Apolinario <[email protected]>
eapolinario
pushed a commit
that referenced
this pull request
Jun 17, 2022
…bworkflow order (#1011) * update get_serializable_workflow Signed-off-by: Vrinda Vasavada <[email protected]> * make fmt Signed-off-by: Vrinda Vasavada <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
We observed a bug when registering a workflow that directly calls one or more workflows. The first time I register it, it completes successfully. In subsequent times, we would expect it to skip the workflow / tasks, because there are no changes and the registration already happened. However, sometimes when you go to register again, instead of skipping, it produces an error that "workflow with different structure already exists with id resource_type". This is because the serialized versions of the parent workflow are different across calls to registration.
This PR this issue by ensuring that
get_serializable_workflowreturns thesubworkflowdictionaries in the same order every time it is called.Type
Are all requirements met?
Complete description
The main open question I have is is this the correct place for this change to be made? Or should this be made on the flyteadmin side?
Tracking Issue
fixes flyteorg/flyte#2508
Follow-up issue
NA