Repository navigation
[Python] Only give an unknown option a value that directly follows it - #40647
Thanatos9404 wants to merge 1 commit into
Conversation
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
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). |
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.