Skip to content

Sanitize StepMutator attributes with to_pod() in _graph_info - #3339

Merged
talsperre merged 1 commit into
Netflix:masterfrom
sertansenturk-deliveroo:fix/graph-info-stepmutator-to-pod
Aug 15, 2026
Merged

Sanitize StepMutator attributes with to_pod() in _graph_info#3339
talsperre merged 1 commit into
Netflix:masterfrom
sertansenturk-deliveroo:fix/graph-info-stepmutator-to-pod

Conversation

@sertansenturk-deliveroo

Copy link
Copy Markdown
Contributor

Summary

node_to_dict() in graph.py sanitizes classic step decorator attributes with to_pod(), but not StepMutator/config-decorator attributes. This fix applies the same to_pod() sanitization to both.

Context / Motivation

A StepMutator whose constructor takes a non-primitive argument gets that raw object embedded in _graph_info, since the second branch of the decorators list in node_to_dict() skips to_pod(). Any consumer that later unpickles _graph_info without that argument's class importable fails with ModuleNotFoundError. This breaks the Metaflow UI's DAG tab for any flow using such a StepMutator, since the UI backend has no reason to have a given decorator's package installed. Fixes #3338.

Root cause: the two branches of node_to_dict()'s decorators list construction are inconsistent. node.decorators (classic StepDecorator) goes through to_pod(); chain(node.wrappers, node.config_decorators) (StepMutator/config decorators) does not.

Failure modes considered:

  • A StepMutator with only primitive constructor arguments (str/int/float/dict/list): unaffected, to_pod() passes these through unchanged.
  • A StepMutator with no constructor arguments at all (bare decorator): unaffected, _args/_kwargs are empty and to_pod() on an empty dict is a no-op.

Changes Made

  • Wrap the StepMutator/config-decorator attributes dict in to_pod() in node_to_dict() (metaflow/graph.py).
  • Add test_step_mutator_non_primitive_attribute_sanitized_in_output_steps to test/unit/test_graph_structure.py, which fails without the fix and passes with it.

Testing

  • New unit test added and confirmed it fails without the fix (assert isinstance(..., str) fails on the raw object) and passes with it.
  • Full unit suite: cd test/unit && python -m pytest -q (local runtime, no cloud/k8s involved since this is a pure DAG-serialization path): 563 passed.
  • black --check on both changed files: clean.
  • Manually reproduced the original bug end to end (see DAG data leaks raw StepMutator constructor args, breaking Metaflow UI when the argument's class isn't importable #3338 for the standalone repro script) before writing the fix, confirming the exact ModuleNotFoundError failure mode this fix addresses.

AI disclosure

Parts of this PR (investigation, implementation, and test) were developed with AI assistance (Claude Code). I reviewed and understand every change and can answer questions about it.

node_to_dict() in graph.py sanitizes classic step decorator attributes
with to_pod(), but not StepMutator/config-decorator attributes. A
StepMutator constructor argument that is a non-primitive object gets
embedded raw in _graph_info as a result. Any consumer that later
unpickles _graph_info without that argument's class importable fails
with ModuleNotFoundError, breaking the Metaflow UI's DAG tab for any
flow using such a StepMutator.

Apply the same to_pod() sanitization to both branches of the decorators
list in node_to_dict().

Fixes Netflix#3338
@sertansenturk-deliveroo
sertansenturk-deliveroo marked this pull request as ready for review August 14, 2026 08:46
@greptile-apps

greptile-apps Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Applies to_pod() consistently to StepMutator and config-decorator attributes when generating _graph_info, preventing non-portable Python objects from being embedded in graph metadata.

  • Aligns StepMutator/config-decorator serialization with classic step decorators.
  • Adds a regression test covering a non-primitive StepMutator constructor argument.

Confidence Score: 5/5

The PR appears safe to merge, with the serialization fix aligned to the existing classic-decorator behavior and covered by a focused regression test.

The changed branch now applies the same portable serialization contract as its sibling decorator branch, preventing raw custom objects from leaking into persisted graph metadata without breaking any established consumer contract.

Important Files Changed

Filename Overview
metaflow/graph.py Applies the established portable-object conversion to StepMutator/config-decorator attributes without introducing an actionable defect.
test/unit/test_graph_structure.py Adds focused regression coverage confirming that a non-primitive StepMutator argument is sanitized in output graph metadata.

Reviews (1): Last reviewed commit: "Sanitize StepMutator attributes with to_..." | Re-trigger Greptile

@talsperre talsperre left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed the fix and regression coverage. The change is correctly scoped to sanitize StepMutator/config-decorator attributes in _graph_info, and I found no blocking issues.

@codecov

codecov Bot commented Aug 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (master@4fce948). Learn more about missing BASE report.

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #3339   +/-   ##
=========================================
  Coverage          ?   30.65%           
=========================================
  Files             ?      381           
  Lines             ?    52654           
  Branches          ?     9288           
=========================================
  Hits              ?    16142           
  Misses            ?    35316           
  Partials          ?     1196           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@talsperre
talsperre merged commit 2fb3c91 into Netflix:master Aug 15, 2026
42 checks passed
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.

DAG data leaks raw StepMutator constructor args, breaking Metaflow UI when the argument's class isn't importable

2 participants