Skip to content

Warn when a repeated -t silently discards earlier targets - #1883

Closed
Mohit-Ak wants to merge 1 commit into
MFlowCode:masterfrom
Mohit-Ak:fix/warn-repeated-multi-value-flags
Closed

Mohit-Ak wants to merge 1 commit into
MFlowCode:masterfrom
Mohit-Ak:fix/warn-repeated-multi-value-flags

Conversation

@Mohit-Ak

Copy link
Copy Markdown
Contributor

-t is declared with nargs="+", so argparse registers it with the default store action. A second occurrence of the flag doesn't append to the first — it replaces it. -t pre_process -t simulation therefore parses to ['simulation'], and the generated batch script has one fewer step than the user wrote, with nothing printed to say so.

Confirmed against the real generated parser (cli/argparse_gen.generate_parser over MFC_CLI_SCHEMA, which is what args.parse uses):

invocation parsed targets
-t simulation ['simulation']
-t pre_process simulation ['pre_process', 'simulation']
-t pre_process -t simulation ['simulation']
--targets pre_process --targets simulation ['simulation']
-t pre_process -t simulation -t post_process ['post_process']

On the two suggested fixes

The issue offered either a warning or action="extend". I went with the warning, but only after testing extend — it turns out to be actively wrong here, because targets has a non-empty list default:

invocation current action="extend"
-t simulation ['simulation'] ['pre_process', 'simulation', 'post_process', 'simulation']
-t pre_process simulation ['pre_process', 'simulation'] ['pre_process', 'simulation', 'post_process', 'pre_process', 'simulation']

extend appends to the default rather than replacing it, so -t simulation would silently start running all three targets — it converts a quiet drop into a quiet addition, which is worse: -t simulation is the restart-from-existing-data invocation, and the whole point of passing it is not to re-run pre_process. Every -t call site in the repo (test/case.py, test/convergence.py, bench.py) would be affected too. So: warning only, and parsing behaviour is left exactly as it is.

What the fix does

_warn_on_repeated_multi_value_flags counts occurrences of each list-valued flag in cli_argv and prints a yellow note when one appears more than once. It's driven off the CLI schema rather than a hardcoded -t: _multi_value_flags walks the command's own arguments plus everything it pulls in through include_common, and yields any argument declared nargs="+" or "*". That means --gpus and --only are covered by the same check — they have the identical shape and the identical failure mode — and any future nargs="+" option is covered automatically. Short/long/= spellings all count toward the same option.

End to end, on the exact command from the report:

$ ./mfc.sh run examples/1D_sodshocktube/case.py --dry-run -t pre_process -t simulation

 -t / --targets was given 2 times, but it takes a space-separated list and only the
 last occurrence is kept. Earlier values were discarded; pass them together as e.g.
 --targets A B.
 ...
                                    | --targets simulation

The banner underneath still reads --targets simulation, which is the behaviour the warning is there to explain.

Testing

toolchain/mfc/test_args_repeated_flags.py — 16 tests covering the repeated short form, the long form, mixed spellings, the = form, three repeats, targets reaching build via include_common, --gpus, and the schema walk itself. The silent controls matter as much as the warnings: -t pre_process simulation, -t simulation, and no flag at all must all stay quiet, and one test asserts parse() still returns argparse's unmodified result.

RED→GREEN was proved in a throwaway worktree with args.py restored from upstream/master and the tests copied in — using a behavioural test that imports only names present on upstream, so it runs identically on both sides:

RED  (unpatched args.py):  1 failed, 2 passed
     AssertionError: a target was silently discarded: nothing was printed to warn the user
GREEN (patched args.py):   3 passed

The 2 that pass on both sides are the controls (intended form, single target) — they confirm the failure is the dropped-target case specifically and not a broken harness.

Both CI gates, re-run against the committed SHA with a clean tree:

command CI workflow result
./mfc.sh lint test-toolchain-compat.yml 705 passed, 9 subtests passed
./mfc.sh precheck -j 4 lint-toolchain.yml 7/7 checks pass

Fixes #1875

-t is declared with nargs="+", so it takes a space-separated list and argparse
stores it with the default "store" action. A second occurrence replaces the
first rather than appending, so `-t pre_process -t simulation` parses to just
[simulation] and the generated batch script runs simulation against an empty
restart_data/ -- with nothing in the output saying a target was dropped.

Warn when a list-valued flag appears more than once. The check walks the CLI
schema for every argument declared nargs="+"/"*" (including those inherited
through include_common), so --gpus and --only are covered too, not just -t.
Parsing behaviour is deliberately unchanged: the warning is advisory.
@sbryngelson

Copy link
Copy Markdown
Member

doesn't use pr template

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

A repeated -t silently discards the earlier targets

2 participants