Repository navigation
[Python] Only give an unknown option a value that directly follows it - #40647
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review Summary
This PR fixes a bug in PipelineOptions.get_all_options(retain_unknown_options=True) where non-adjacent unknown arguments were erroneously paired together after known arguments were stripped by argparse. By tracking the original indices of unknown arguments in the raw flag list, it ensures an unknown flag only consumes a value that directly follows it and properly handles --flag=value options followed by stray positional arguments.
🔍 General Feedback
- The implementation in
_find_positionsandget_all_optionsis clean, concise, and handles edge cases (like--separators, single-dash options, inline=values, and known positionals) accurately. - The test coverage added in
pipeline_options_test.pyis thorough and directly verifies all relevant flag combinations. - One minor edge case to be aware of (not introduced by this PR, as
self._flagsis already re-parsed multiple times): if a caller passes a single-use iterator (e.g., a generator) asflagstoPipelineOptions(flags), it would be exhausted during__init__. Normalizingself._flagsto alistin__init__could be a nice future hardening, though out of scope for this fix.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
|
Assigning reviewers: R: @claudevdm for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
6bd5f33 to
ff664f7
Compare
ff664f7 to
fddb45b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40647 +/- ##
==========================================
+ Coverage 79.83% 80.09% +0.25%
==========================================
Files 494 494
Lines 83575 83630 +55
==========================================
+ Hits 66722 66981 +259
+ Misses 16853 16649 -204
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
a035011 to
5659c17
Compare
With retain_unknown_options, get_all_options registers each unknown flag with argparse and parses the flags again. Whether an unknown flag takes a value was decided from the list of unknown args alone, but those are not necessarily adjacent in the original flags. For --unknown_flag --job_name=foo bar the unknown args are ['--unknown_flag', 'bar'], so --unknown_flag was registered as taking a value, and parsing the original flags again failed with "expected one argument". This also made Pipeline.run() fail. Look up where each unknown arg came from in the original flags and only treat an unknown flag as taking a value when that value directly follows it there. A flag written as --flag=value always uses its inline value, so it is no longer registered under the name "--flag=value" when a stray argument follows it. Fixes apache#22434
5659c17 to
60dd1fa
Compare
Codecov flagged three lines of the fix as untested: the sys.argv path taken when no flags are passed, and an arg that _find_positions does not find.
|
@jrmccluskey could you take a look when you have a moment? It has been waiting for review since mid-October. The latest commit covers the three lines Codecov flagged: the |
jrmccluskey
left a comment
jrmccluskey
left a comment
There was a problem hiding this comment.
A few suggestions for readability, but the underlying ideas are good!
| return _f | ||
|
|
||
|
|
||
| def _find_positions(args, flags): |
There was a problem hiding this comment.
Nit: add type annotations
| has_value = ( | ||
| '=' not in unknown_args[i] and i + 1 < len(unknown_args) and | ||
| not unknown_args[i + 1].startswith('-') and | ||
| positions[i] is not None and positions[i] + 1 < len(all_flags) and | ||
| not all_flags[positions[i] + 1].startswith('-')) |
There was a problem hiding this comment.
This boolean evaluation block is somewhat unwieldy to follow, I would suggest splitting it out into a helper + adding a docstring just for readability purposes.
There was a problem hiding this comment.
Done in 5d8aaa8. The check now lives in _has_separate_value, with a docstring that explains why the value has to directly follow the flag in the original flags. Behavior is unchanged and the existing unknown option tests still pass.
Review follow-up: add type annotations to _find_positions and replace the inline boolean expression in get_all_options with _has_separate_value, which has a docstring. No behavior change.
Fixes #22434
PipelineOptions.get_all_options(retain_unknown_options=True)decided whether an unknown flag takes a value by looking at the next entry in the list of unknown args. argparse takes the known options out of that list, so entries that are next to each other there aren't always next to each other in the original flags. For--unknown_flag --job_name=foo bar,--unknown_flagwas registered as taking a value and parsing failed withexpected one argument, which also brokePipeline.run().Now an unknown flag only takes a value when the value directly follows it in the original flags. Cases that already worked behave as before. One related case changes:
--flag=valuefollowed by a stray argument now keeps its inline value (the input that broke the load tests in #22422).Tests: new cases in
pipeline_options_test.py, six of which fail without the change and pass with it. I also added aCHANGES.mdentry under Bugfixes for 2.78.0.