python: improve CLI option parsing with FluxArgumentParser#7675
Open
grondo wants to merge 9 commits into
Open
Conversation
Problem: Python flux commands use argparse.ArgumentParser with the default allow_abbrev=True, which silently matches abbreviated option strings. This can cause unintended behavior when tokens intended for a job command are consumed as abbreviated Flux options. Add FluxArgumentParser, a subclass of argparse.ArgumentParser that sets allow_abbrev=False and defaults to the Flux help formatter. It supports a hidden_aliases kwarg on add_argument() for option strings that should be recognized but suppressed from --help output, allowing migration from previously-working abbreviations without unnecessarily breaking existing usage. The companion _FluxArgumentGroup class ensures the same behavior applies when add_argument() is called on an argument group. Update FluxHelpFormatter in util.py to filter hidden_option_strings from the formatted option invocation string. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Problem: FluxArgumentParser and its hidden_aliases support have no unit tests, making it difficult to verify correctness or catch regressions. Add t/python/t0047-flux-argument-parser.py covering allow_abbrev enforcement, default help formatter behavior, subparser class inheritance, and hidden_aliases operation via both direct parser add_argument() calls and argument groups. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Problem: Python flux commands use argparse.ArgumentParser with allow_abbrev=True, which silently matches abbreviated option strings. This can cause unintended behavior when tokens intended for a job command are consumed as abbreviated Flux options. Replace all argparse.ArgumentParser usage in command and library Python files with FluxArgumentParser from flux.cli.argparse, which enforces allow_abbrev=False and applies the Flux help formatter by default. Add hidden singular aliases (--state, --plugin, --require, --broker-opt) for plural options where abbreviation previously worked via allow_abbrev, to avoid unnecessarily breaking existing usage during the transition. Fix a typo in t2272-job-begin-time.t where --begin=time=+1s relied on --begin abbreviating --begin-time. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Problem: Submission commands (flux run, submit, alloc, batch) accept a job command and its arguments as a trailing REMAINDER positional. With GNU-style argparse, option-like tokens that appear after the job command are still consumed as Flux options, making behavior depend on argument ordering in unexpected ways. Add a posix=True constructor parameter to FluxArgumentParser that splits argv at the first non-option argument before parsing. Options before the first positional are parsed normally; everything from the first positional onward is injected into the REMAINDER dest. A user-supplied '--' separator is preserved as the first element of the positionals. If there is no REMAINDER dest and positionals remain, the parser raises an "unrecognized arguments" error. Negative-number- like tokens (e.g. -1) are treated as non-options when the parser has no negative-number optionals, matching stock argparse behavior. Options with nargs='+' or nargs='*' greedily consume consecutive non-option tokens before the split point. Enable posix=True in MiniCmd.create_parser() so all submission commands get this behavior. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Problem: The posix=True mode added to FluxArgumentParser has no unit tests, making it difficult to verify correctness or catch regressions. Add a TestPosixMode class to t/python/t0047-flux-argument-parser.py covering option-before-command parsing, option-after-command capture, user-supplied '--' preservation, short option value skipping, and the guarantee that no implicit '--' appears in the REMAINDER result. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Problem: The new POSIX mode in FluxArgumentParser has no coverage in the existing per-command sharness tests, making it easy to regress without notice. Add one test to each of t2710 (submit), t2711 (run), t2712 (alloc), t2713 (bulksubmit), and t2714 (batch) verifying that a recognized Flux option appearing after the job command is captured in the REMAINDER positional rather than consumed by the Flux parser. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Problem: FluxArgumentParser sets allow_abbrev=False to reject abbreviated long options, but on Python 3.6/3.7 that setting also breaks short-option concatenation (-vvv, -n4) due to bpo-26967: those versions gate the entire _get_option_tuples call in _parse_optional on allow_abbrev. Simply skipping allow_abbrev=False on old Pythons would leave EL8 users (3.6/3.7) with long-option abbreviations enabled and behavior differing by platform. Backport the bpo-26967 fix for Python 3.6/3.7 by overriding _get_option_tuples at class body level under a sys.version_info < (3, 8) guard: for long options (starting with two prefix chars), return [] to suppress prefix matching; for short options fall through to super() so concatenation works normally. On Python >= 3.8, set allow_abbrev=False (the bpo-26967 fix moved the gate inside _get_option_tuples in 3.8, so only long-option abbreviation is gated there). 3.6/3.7 are frozen/EOL so the private-method override cannot drift. The override was verified against the real CPython 3.6 argparse.py: -vvv, -vn4, and -n 4 all work; --foo abbreviations of --foo-bar are rejected. With uniform behavior across all supported Pythons, the abbrev tests in t0047 need no version skip. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Problem: Users have been relying on --dry as an abbreviation for --dry-run in flux submit, run, alloc, batch, bulksubmit, cancel, update, and modprobe load. With allow_abbrev=False this abbreviation no longer works. Add --dry as a hidden alias for --dry-run in MiniCmd.create_parser() covering all submission commands, in flux-cancel, in flux-update, and in the load subcommand of flux-modprobe, to avoid unnecessarily breaking existing usage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Problem: flux-run, flux-submit, flux-alloc, flux-batch, and flux-bulksubmit now stop option processing at the first non-option argument and no longer accept abbreviated long options, but the man pages do not describe this behavior. Add a prose note at the end of the OTHER OPTIONS section (via the shared job-other-options.rst fragment included by all five submission command pages) describing the new behavior: option processing stops at the first non-option argument so COMMAND and its arguments are never consumed as Flux options, and long options may no longer be abbreviated. The note also clarifies that a ``--`` separator is still accepted and that flux-alloc forwards it to the broker. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #7675 +/- ##
==========================================
+ Coverage 83.99% 84.02% +0.03%
==========================================
Files 577 578 +1
Lines 97511 97614 +103
==========================================
+ Hits 81907 82024 +117
+ Misses 15604 15590 -14
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds a
FluxArgumentParserclass and uses it throughout the Python CLI commands so they share consistent argument parsing behavior. The changes implemted inFluxArgumentParservs Pythonargparseinclude:allow_abbrev=False) On Python 3.6/3.7, whereallow_abbrev=Falsewould also break short-option concatenation (-vvv,-n4) due to bpo-26967, the 3.8 fix is backported via a version-gated override instead.hidden_aliasesextension toadd_argument()preserves a few commonly-used former abbreviations (--dry,--state,--plugin,--require,--broker-opt) without advertising them in--help.flux run,submit,alloc,batch,bulksubmit) now use POSIX-style option parsing: Withposix=True,FluxArgumentParseroption processing stops at the first non-option argument, so tokens after the job command are never consumed as Flux options regardless of ordering. A--separator still works as before.formatter_class=boilerplate.Fixes #7644