Skip to content

Fix mutable_flow test flow passing a stale second argument to extract_step_decorator_from_decospec - #3358

Open
Anai-Guo wants to merge 1 commit into
Netflix:masterfrom
Anai-Guo:fix-mutable-flow-decospec-arity
Open

Fix mutable_flow test flow passing a stale second argument to extract_step_decorator_from_decospec#3358
Anai-Guo wants to merge 1 commit into
Netflix:masterfrom
Anai-Guo:fix-mutable-flow-decospec-arity

Conversation

@Anai-Guo

Copy link
Copy Markdown

extract_step_decorator_from_decospec accepts exactly one argument
(metaflow/decorators.py:692):

def extract_step_decorator_from_decospec(decospec: str):

but both call sites in the mutable_flow test flow still pass a second
positional {}:

# test/test_config/mutable_flow.py:61  (ModifyFlow.mutate)
env_deco, _ = extract_step_decorator_from_decospec(decos[0], {})

# test/test_config/mutable_flow.py:110 (ModifyFlow2.mutate)
env_deco, _ = extract_step_decorator_from_decospec(deco, {})

Both are reached from a FlowMutator.mutate(), which runs at flow-definition
time, so mutable_flow.py raises TypeError: extract_step_decorator_from_decospec() takes 1 positional argument but 2 were given before any step executes. The flow
is registered as a test case in test/test_config/test.py:96.

The two call sites in library code already pass a single argument, which is what
makes this a stale-caller bug rather than a signature question:

Call site Args Status
metaflow/decorators.py:768 1
metaflow/user_decorators/mutable_step.py:340 1
test/test_config/mutable_flow.py:61 2
test/test_config/mutable_flow.py:110 2

Binding the real upstream signature against all four call sites, with the two
library callers as the control group:

extract_step_decorator_from_decospec (decospec)
  TypeError test/test_config/mutable_flow.py:61  (decos[0], {}) -> too many positional arguments
  TypeError test/test_config/mutable_flow.py:110 (deco, {})     -> too many positional arguments
  OK        CONTROL metaflow/decorators.py:768
  OK        CONTROL metaflow/user_decorators/mutable_step.py:340

This PR drops the extra {} at both sites so the test flow matches the
signature and the two library callers. Nothing else changes — the return value
is still unpacked as (step_deco, has_args_kwargs), exactly as
mutable_step.py:340 does.

🤖 Generated with Claude Code

…rator_from_decospec

`extract_step_decorator_from_decospec` takes a single `decospec` argument, but
both call sites in the `mutable_flow` test flow still pass a second `{}`
positional, so `ModifyFlow`/`ModifyFlow2` raise TypeError as soon as the flow
is mutated.

Both production call sites already pass one argument.
@greptile-apps

greptile-apps Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes the mutable-flow configuration test by removing obsolete second arguments from two calls to extract_step_decorator_from_decospec.

  • Aligns both test-flow call sites with the helper’s single-argument signature.
  • Preserves the existing decorator parsing and return-value handling.

Confidence Score: 5/5

The PR appears safe to merge; the narrowly scoped changes correct two stale test call sites without altering behavior beyond preventing their argument-count errors.

Both modified calls now match the parser’s single-argument signature while retaining the same inputs, tuple unpacking, and subsequent decorator mutation logic.

Important Files Changed

Filename Overview
test/test_config/mutable_flow.py Both stale two-argument calls are correctly updated to use the helper’s existing single-argument interface, with no issues identified.

Reviews (1): Last reviewed commit: "fix(test_config): drop the stale second ..." | Re-trigger Greptile

@Shriprasad-P Shriprasad-P left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The extra {} is definitely stale, but I don't think removing it is sufficient to make this flow work against the current mutable-step API.

MutableStep.decorator_specs now yields (name, fq_name, args, kwargs) tuples. These two paths still call .startswith("environment:") on those values, so they can fail before extract_step_decorator_from_decospec() is reached.

There are a couple of related stale API usages here as well: the decorator attributes are already available from the tuple, and add_decorator() now expects them through deco_kwargs= (with duplicates=OVERRIDE when replacing the existing environment decorator).

The current implementation in test/ux/core/flows/config/mutable_flow.py already follows that pattern. I think test/test_config/mutable_flow.py should be aligned with that implementation rather than continuing to parse these specs through extract_step_decorator_from_decospec.

So the arity change itself is correct, but the test flow still appears incompatible with the current decorator-spec API.

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.

2 participants