fix(argo): order conditional input fallback by DAG#3233
Conversation
8f040c7 to
b411cf4
Compare
b411cf4 to
3d6c4eb
Compare
| ] | ||
| return len(cond_in_funcs) > 1 and len(cond_in_funcs) == len(node.in_funcs) | ||
|
|
||
| def _skippable_input_steps_in_dag_order(self, node): |
There was a problem hiding this comment.
Changes pass my existing test cases for conditionals so should be fine from that perspective.
Greptile SummaryThis PR fixes a bug where chained
Confidence Score: 5/5All three changes are narrowly scoped to the conditional-input-path selection logic, and the new test directly reproduces the reported regression. The logic is correct across all traced scenarios: chained skips, partial execution, and the step-name prefix collision that triggered the original bug. The two observations are cosmetic and do not affect runtime behaviour. No files require special attention; Important Files Changed
Reviews (1): Last reviewed commit: "fix: order Argo conditional input fallba..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # If the input-path is from a conditional, we want to pick the one that is last-in-line in the DAG. | ||
| # The order of graph parsing ensures that the steps are in reverse order of occurrence, so the first one is the latest. | ||
| latest_conditional_in_graph = trimmed[:1] | ||
| # Argo passes skippable_steps in reverse DAG order, so the first matching | ||
| # executed skippable step is the latest conditional predecessor. |
There was a problem hiding this comment.
_skippable_input_steps_in_dag_order in the compilation code that guarantees this order. A reader of this standalone script has no way to verify that contract from the comment alone. Tightening the wording makes the invariant self-documenting.
| # If the input-path is from a conditional, we want to pick the one that is last-in-line in the DAG. | |
| # The order of graph parsing ensures that the steps are in reverse order of occurrence, so the first one is the latest. | |
| latest_conditional_in_graph = trimmed[:1] | |
| # Argo passes skippable_steps in reverse DAG order, so the first matching | |
| # executed skippable step is the latest conditional predecessor. | |
| # If the input-path is from a conditional, we want to pick the one that is last-in-line in the DAG. | |
| # skippable_steps is passed in reverse DAG order (ensured by _skippable_input_steps_in_dag_order), | |
| # so the first matching executed step is the latest conditional predecessor. |
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!
| paths_by_step = {} | ||
| for path in trimmed: | ||
| paths_by_step.setdefault(_step_name_from_path(path), path) |
There was a problem hiding this comment.
path contains no / separator, _step_name_from_path returns None, which is silently inserted as a key in paths_by_step. This is harmless today (the None key is never looked up via skippable_steps), but it is an unexpected state. A guard keeps the dict's invariant explicit and prevents surprises if the code evolves.
| paths_by_step = {} | |
| for path in trimmed: | |
| paths_by_step.setdefault(_step_name_from_path(path), path) | |
| paths_by_step = {} | |
| for path in trimmed: | |
| step_name = _step_name_from_path(path) | |
| if step_name is not None: | |
| paths_by_step.setdefault(step_name, path) |
2 participants
Summary
conditional_input_pathsconditional_input_pathschoose the latest executed skippable predecessor when all executed inputs are skippableFixes #3230
Tests
uv run --with '.[dev]' python -m pytest test/unit/test_argo_conditional_input_paths.py test/unit/test_argo_workflows_cli.py -quv run --with pre-commit pre-commit run --files metaflow/plugins/argo/conditional_input_paths.py metaflow/plugins/argo/argo_workflows.py test/unit/test_argo_conditional_input_paths.py test/unit/test_argo_workflows_cli.py