This repository has been archived by the owner on Nov 3, 2023. It is now read-only.
[params] Support options depending on options in parse_kwargs. #3900
Conversation
This file contains 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
stephenroller
commented
Aug 4, 2021
Comment on lines
+1207
to
+1209
| unparsed_args = set(kwargs.keys()) | ||
| while unparsed_args: | ||
| string_args = [] |
There was a problem hiding this comment.
These 3 lines are the real change (the rest is just indentation)
Comment on lines
+1252
to
+1254
| if kwname not in kwname_to_action: | ||
| new_unparsed_args.add(kwname) | ||
| continue |
There was a problem hiding this comment.
These lines are new
Comment on lines
+1271
to
+1280
| if new_unparsed_args == unparsed_args: | ||
| # if we have converged to a fixed point with no improvements, we | ||
| # truly found some unreachable args | ||
| raise KeyError( | ||
| f'Failed to parse one or more kwargs: {", ".join(new_unparsed_args)}' | ||
| ) | ||
| else: | ||
| raise TypeError(f"Don't know what to do with {action}") | ||
| # We've seen some improvements on the number of unparsed args, | ||
| # iterate again | ||
| unparsed_args = new_unparsed_args |
There was a problem hiding this comment.
These lines are new
ankitade
approved these changes
Aug 4, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Patch description
@ankitade reported an issue where systems using
parse_kwargs(such as internal customers and the Colab notebooks) could not correctly use Mutator options. Debugging, it appears this derives from not having enough repeated iterations ofadd_cmdline_argsbeing called with options that depend on other options.Previously: Parse 'task=' and call the Task.add_cmdline_args. However, since
--mutatorshasn't been added yet,partial_optdoesn't contain any mutators and we don't load these in.After: We repeatedly parse kwargs until there are remaining unparsed arguments, or we do not see any improvements (fixed point)
Testing steps
New CI examples that previously would not pass.