fix: dynamic var spec serialization, JSON indexes, and StepMutator sequencing#3285
fix: dynamic var spec serialization, JSON indexes, and StepMutator sequencing#3285npow wants to merge 3 commits into
Conversation
Greptile SummaryThis PR fixes several dynamic decorator value roundtrip issues. The main changes are:
Confidence Score: 4/5The dynamic-var spec serialization path needs fixes before merging.
metaflow/decorators.py Important Files Changed
Reviews (1): Last reviewed commit: "Fix StepMutator pre-mutate sequencing fo..." | Re-trigger Greptile |
| payload["default"] = value.default | ||
| encoded = base64.urlsafe_b64encode( | ||
| json.dumps(payload, separators=(",", ":"), sort_keys=True).encode("utf-8") |
There was a problem hiding this comment.
Default Serialization Can Crash
When a decorator dynamic var uses a valid Python default such as datetime, bytes, an enum, or a custom object, this path stores that object in the JSON payload and json.dumps(payload) raises during make_decorator_spec(). The resolver accepts defaults as Python values and returns them directly, so preserving the metadata this way can turn task launch or deployment into a serialization failure.
| if payload.startswith(_DYNAMIC_VAR_JSON_PREFIX): | ||
| data = json.loads( | ||
| base64.urlsafe_b64decode( | ||
| payload[len(_DYNAMIC_VAR_JSON_PREFIX) :].encode("ascii") | ||
| ).decode("utf-8") |
There was a problem hiding this comment.
An existing legacy sentinel for a variable whose name starts with json:, such as __dynvar__:json:foo, now enters the new base64 decode branch instead of the legacy fallback. That makes parsing the decorator spec raise from urlsafe_b64decode rather than reconstructing DynamicVar("json:foo"), breaking backward compatibility for those saved specs.
Summary
Three bug fixes for the dynamic decorator values framework from #2970, found during integration testing with the Maestro orchestrator.
1. Preserve dynamic var spec metadata (
df8dc15f)Decorator.translate_decorator_specwas reconstructingDynamicVarobjects from the__dynvar__:sentinel but losingpertaskanddefaultmetadata. Fixed by base64-encoding a JSON payload into the sentinel so all fields survive the spec → string → spec roundtrip.2. Handle JSON stringified dynamic var indexes (
d55d75e4)Foreach split indexes stored as Python
intkeys ({0: "val"}) become string keys ({"0": "val"}) when serialized through JSON (e.g., via Maestro).resolve_dynamic_vars_from_storenow accepts both integer and stringified integer keys.3. Fix StepMutator pre-mutate sequencing (
2bc95e1e)FlowSpec._pre_step_decorator_hookswas callingstep_decorator.pre_mutate_stepfor every step on everystep_decorator, butStepMutator.pre_mutate_stepshould only be called on the step that owns the mutator. Fixes incorrect dynamic var resolution when multiple steps have mutators.PR Type
Test plan
test/unit/test_dynamic_var_decorator_spec.py)Fixes bugs in #2970.