-
Notifications
You must be signed in to change notification settings - Fork 332
[BUG] sync_node_executions fails for workflows with BranchNode #3274
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
base: master
Are you sure you want to change the base?
Conversation
-e Signed-off-by: machichima <[email protected]>
-e Signed-off-by: machichima <[email protected]>
-e Signed-off-by: machichima <[email protected]>
-e Signed-off-by: machichima <[email protected]>
-e Signed-off-by: machichima <[email protected]>
-e Signed-off-by: machichima <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3274 +/- ##
==========================================
- Coverage 83.35% 76.44% -6.92%
==========================================
Files 347 245 -102
Lines 28791 23657 -5134
Branches 2960 2961 +1
==========================================
- Hits 23999 18084 -5915
- Misses 3956 4711 +755
- Partials 836 862 +26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
-e Signed-off-by: machichima <[email protected]>
| if execution._node.branch_node is not None: | ||
| logger.info( | ||
| "Skipping branch node execution for now - branch nodes will " "not have inputs and outputs filled in" | ||
| ) |
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 fixed it here. 6a4c2d8
Tracking issue
Part of flyteorg/flyte#6490
Why are the changes needed?
When using flytekit's
FlyteRemoteand callingremote.waiton an execution of a workflow that contains aBranchNode, we will get following error:What changes were proposed in this pull request?
In some cases, the
execution.metadata.is_parent_nodefor branch node is not True, which lead to the block without managing branch node separately. Moving the logic handling branch node before checkingis_parent_nodeto handle both is and is not branch node casesHow was this patch tested?
integration test
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
Summary by Bito
This pull request enhances the handling of branch nodes in Flytekit's execution logic, fixing a bug related to their management and ensuring they are handled correctly regardless of their parent node status. It introduces a new conditional workflow and tests to improve the library's robustness for complex workflows by preventing errors associated with missing attributes in branch nodes.