Remove deprecated and unsafe type=bool usage on boolean CLI flags#5528
Open
ahmadki wants to merge 3 commits into
Open
Remove deprecated and unsafe type=bool usage on boolean CLI flags#5528ahmadki wants to merge 3 commits into
ahmadki wants to merge 3 commits into
Conversation
argparse.BooleanOptionalAction stores a literal True/False constant, so the type, choices, and metavar keyword arguments are never applied. Passing them is meaningless and deprecated: older interpreters ignore them, and newer ones reject them outright, raising TypeError when the parser is built. Five RL flags in _add_rl_args passed type=bool to BooleanOptionalAction. Remove it; behavior is unchanged wherever the code runs. Add a static guard test that scans the megatron package for BooleanOptionalAction calls passing type/choices/metavar, plus a behavioral test asserting the RL flags still register and parse as real booleans. Signed-off-by: Ahmad Kiswani <kiswani.ahmad@gmail.com>
argparse applies the `type` callable to the raw string, and `bool()` of any
non-empty string is True, so `type=bool` on a value-taking argument means
`--flag False` silently *enables* the flag (only `--flag ""` yields False).
Two flags were affected:
- megatron/training/arguments.py: --onnx-safe (the parsed attribute is unused
and no caller passes the flag)
- examples/post_training/modelopt/convert_model.py: --mix-hidden-states
(consumed only as a truthy check, default False, no caller passes a value)
Both are pure on/off toggles, so convert them to action='store_true'
(default False), matching the surrounding flags (e.g. --quick-geglu).
Extend the argparse guard test to also reject add_argument(type=bool) anywhere
under megatron/, so this footgun cannot reappear in library code.
Signed-off-by: Ahmad Kiswani <kiswani.ahmad@gmail.com>
These Click options are boolean flags (declared with is_flag=True or the --x/--no-x slash form). Click resolves such flags to its BOOL type regardless of the type argument, so type=bool is redundant: the resolved option type is BOOL with or without it, and parsing is identical. It is also a documented anti-pattern (pallets/click#1062); combined with is_flag it has historically made a missing flag default to None instead of the declared default. Removing it is behavior-preserving on the pinned Click (8.4.1), verified by introspection (resolved type is BOOL either way) and by parity tests showing the missing-flag value is unchanged (the explicit False/True default), and the affected commands still build and --help cleanly. No CLI behavior changes. The value-taking --record-checkpoints option (declared type=str elsewhere) is unaffected. Signed-off-by: Ahmad Kiswani <kiswani.ahmad@gmail.com>
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
2 participants
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.
What does this PR do ?
Removes
type=bool(and related) usage on boolean CLI options. Across thecodebase this usage is deprecated at best and dangerous at worst, and adds a
guard test so the unsafe cases cannot reappear.
argparse.BooleanOptionalActionstores a literal bool, sotype,choices,and
metavarare never applied. Passing them is deprecated: olderinterpreters ignore them and newer ones reject them outright, which makes the
parser fail to build. Removed
type=boolfrom five RL flags in_add_rl_args.type=boolon a value-taking argparse argument is dangerous: argparse runsthe raw string through
bool(), andbool()of any non-empty string isTrue, so--flag Falsesilently enables the flag. Converted--onnx-safeand
--mix-hidden-statestoaction='store_true'.type=boolon a Clickis_flagor--x/--no-xoption is redundant: Clickresolves these to its
BOOLtype regardless (verified on the pinned Click8.4.1, behavior identical). Removed it from seven options in the CI scripts.
megatron/, so they cannot be reintroduced.Issue tracking
Linked issue: N/A (small bug fix and cleanup)
Contribution process
Pre-checks