fix(client): allow non-numeric run/task IDs for Argo and Step Functions#3260
fix(client): allow non-numeric run/task IDs for Argo and Step Functions#3260dheerenmohta wants to merge 1 commit into
Conversation
Run IDs are not always numeric. Orchestrators produce prefixed string IDs:
- Argo Workflows: "argo-<workflow-name>" (e.g. "argo-moviestatsflow-68z2h")
- AWS Step Functions: "sfn-<execution-name>"
The numeric-only check would cause Run('FlowName/argo-...') and
Run('FlowName/sfn-...') to raise MetaflowInvalidPathspec, breaking all
users who access runs created via orchestrators through the Client API.
Replace the strict numeric pattern with a permissive alphanumeric pattern
that also allows hyphens and underscores, matching all current ID formats.
Fixes Netflix#948 (partial)
Greptile SummaryThis PR adds content-based validation for run IDs and task IDs when parsing a
Confidence Score: 3/5The change adds new client-side validation for pathspec run IDs and task IDs where none existed before; while it handles Argo and standard SFN UUIDs correctly, it can incorrectly reject valid SFN run IDs whose execution names contain characters like The new metaflow/client/core.py — specifically the Important Files Changed
Reviews (1): Last reviewed commit: "fix(client): allow non-numeric run/task ..." | Re-trigger Greptile |
| # "sfn-<execution-name>" for AWS Step Functions | ||
| # Task IDs follow the same pattern — numeric by default but may be prefixed by | ||
| # orchestrators that pass their own identifiers. | ||
| _RUN_ID_PATTERN = re.compile(r'^[a-zA-Z0-9][a-zA-Z0-9\-_]*$') |
There was a problem hiding this comment.
Pattern rejects valid SFN run IDs with dots or other legal characters
AWS Step Functions execution names can legally contain . (and other characters like +, @) per the SFN StartExecution API. When a state machine is triggered externally — e.g. by an EventBridge rule with a custom execution name such as prod.2024-01-15 — the run ID stored by Metaflow is sfn-prod.2024-01-15. After this PR, any Run('MyFlow/sfn-prod.2024-01-15') call will raise MetaflowInvalidPathspec, whereas before this PR (where no content validation existed) it would look up the run normally. The original code's correctness guarantee was "only check component count" — this pattern narrows that guarantee without covering the full character set that SFN execution names allow.
| # "sfn-<execution-name>" for AWS Step Functions | ||
| # Task IDs follow the same pattern — numeric by default but may be prefixed by | ||
| # orchestrators that pass their own identifiers. | ||
| _RUN_ID_PATTERN = re.compile(r'^[a-zA-Z0-9][a-zA-Z0-9\-_]*$') |
There was a problem hiding this comment.
Using
\- in the middle of a character class works (Python re treats it as a literal hyphen) but is non-idiomatic. The conventional and unambiguous style is to place the hyphen at the end of the class.
| _RUN_ID_PATTERN = re.compile(r'^[a-zA-Z0-9][a-zA-Z0-9\-_]*$') | |
| _RUN_ID_PATTERN = re.compile(r'^[a-zA-Z0-9][a-zA-Z0-9_-]*$') |
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!
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3260 +/- ##
=========================================
Coverage ? 28.65%
=========================================
Files ? 381
Lines ? 52519
Branches ? 9268
=========================================
Hits ? 15049
Misses ? 36442
Partials ? 1028 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Problem
The pathspec validation introduced in #948 uses a numeric-only pattern (
^[0-9]+$) for run IDs and task IDs. However, Metaflow run IDs are not always numeric:argo-moviestatsflow-68z2h(seeargo_workflows_cli.py:965)sfn-my-execution-name(seestep_functions_cli.py:549)This means any user doing
Run('MyFlow/argo-...')orStep('MyFlow/sfn-.../start')via the Client API will get a spuriousMetaflowInvalidPathspec, completely breaking orchestrator-based workflows.The original pre-#948 code only checked component count, not content — this PR preserves that correctness while still adding meaningful content validation.
Fix
Replace the strict
_NUMERIC_ID_PATTERNcheck for run/task positions with a permissive_RUN_ID_PATTERNthat accepts alphanumeric strings plus hyphens and underscores. This covers all current formats:"1748291234567890""argo-myflow-abc12""sfn-prod-execution"Test
Companion test coverage is added in PR #5 (test branch
test/client-pathspec-orchestrator-run-ids).Fixes #948 (partial — critical correctness bug)