Skip to content

fix: argo conditional issue - #3340

Open
saikonen wants to merge 3 commits into
masterfrom
fix/argo-conditional-issue
Open

fix: argo conditional issue#3340
saikonen wants to merge 3 commits into
masterfrom
fix/argo-conditional-issue

Conversation

@saikonen

Copy link
Copy Markdown
Collaborator

PR Type

  • Bug fix

Issue

Fixes #3334

AI Tool Usage

Claude Code for test cases and fix iteration

@saikonen
saikonen requested a review from talsperre August 14, 2026 23:40

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some added test cases have issues, will fix those in a bit. The main fix was verified against a separate test-suite.

@saikonen
saikonen marked this pull request as ready for review August 18, 2026 18:35
@greptile-apps

greptile-apps Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This bug fix preserves recursively calculated conditional ancestry for nested Argo split-switch nodes instead of overwriting it during the graph-wide traversal.

  • Skips redundant top-level visits of nested split-switch nodes.
  • Adds regression coverage for both graph-name orderings and a single-switch flow.

Confidence Score: 4/5

The production fix appears safe to merge, with a non-blocking gap in how directly the regression test verifies the generated Argo dependency.

The traversal change preserves the recursively established ancestry needed for nested conditionals, while the only accepted concern is that tests stop at intermediate metadata rather than confirming the final || dependency output.

Files Needing Attention: test/unit/test_argo_nested_conditional_join.py

Important Files Changed

Filename Overview
metaflow/plugins/argo/argo_workflows.py Preserves nested conditional-parent metadata by avoiding a redundant traversal that previously made dependency semantics graph-order-dependent.
test/unit/test_argo_nested_conditional_join.py Covers parser metadata for nested and simple switches, but does not assert the final Argo dependency expression associated with the regression.

Reviews (1): Last reviewed commit: "simplify fix" | Re-trigger Greptile

Comment on lines +198 to +202

# Inner branch nodes are conditional
for name in ("b_sub_a", "b_sub_b"):
assert name in aw.conditional_nodes, f"{name} should be in conditional_nodes"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Assert the emitted dependency expression

This regression test checks intermediate conditional-node metadata but not the generated Argo depends field. As a result, a downstream regression that restores && between mutually exclusive branches can pass these assertions while leaving the deployed join unable to run.

Knowledge Base Used: Argo Workflows plugin

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Argo: conditional-join depends computed as && instead of || depending on step-name alphabetical order

1 participant