fix(lr_schedules): make --lr_range_test_staircase an opt-in flag - #8337
fix(lr_schedules): make --lr_range_test_staircase an opt-in flag#8337Anai-Guo wants to merge 1 commit into
Conversation
type=bool applies the builtin to the raw argv string, so every spelling of
off ("False", "false", "0") is a non-empty string and evaluates to True.
The flag could be turned on but never off from the command line, and
override_lr_range_test_params copies that value straight into the scheduler
config, where LRRangeTest turns it into _staircase_interval instead of
_continuous_interval.
--cycle_momentum, the other boolean in this same parser, already uses
store_true, as does every other boolean flag in the package. Match it.
Signed-off-by: Anai-Guo <antai12232931@outlook.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67370efe0d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| group.add_argument("--lr_range_test_staircase", | ||
| type=bool, | ||
| default=False, | ||
| action='store_true', |
There was a problem hiding this comment.
Add the required sign-off trailer
This is a non-merge commit, but its message contains no Signed-off-by trailer, violating the repository's commit/CI requirement and potentially failing DCO enforcement. Recreate the commit using git commit --signoff with the configured author identity.
AGENTS.md reference: AGENTS.md:L6-L8
Useful? React with 👍 / 👎.
What
--lr_range_test_staircaseis declared withtype=bool, so argparse applies the builtin to the raw argv string. Every spelling of "off" is a non-empty string, and non-empty strings are truthy:The flag can be turned on but never off. The only value that yields
Falseis the empty string.Why it matters
The parsed value is not cosmetic.
override_lr_range_test_paramscopies it straight into the scheduler config:https://github.com/deepspeedai/DeepSpeed/blob/master/deepspeed/runtime/lr_schedules.py#L143-L144
and
LRRangeTestturns it into the interval function that shapes the whole LR range test:So a user who writes
--lr_range_test_staircase Falsesilently gets themath.floor()staircase schedule instead of the continuous one.add_tuning_argumentsis public API (re-exported fromdeepspeed/__init__.py), anddocs/_tutorials/lrrt.mddocuments"lr_range_test_staircase": falseas a supported setting — reachable through the JSON config, but not through the CLI flag that is supposed to mirror it.The fix
Use
action='store_true', matching--cycle_momentum— the other boolean in this very same parser, with the samedefault=Falseand the sameoverride_*path:--lr_range_test_staircasewas the onlytype=boolargument in the package; every other boolean flag already usesstore_true.Behaviour change, stated plainly
This does change the CLI surface:
--lr_range_test_staircase Truepreviously parsed toTrueand now errors withunrecognized arguments: True. That seemed the better trade, because the alternative spelling--lr_range_test_staircase Falseis currently silently wrong — a loud error is easier to fix than a schedule that quietly differs from what was asked for. Omitting the flag still yieldsFalse, so the default is unchanged and existing runs that never passed the flag are unaffected.If you would rather keep accepting an explicit value, the alternative is a
str_to_bool-style converter — happy to switch, though there is no such helper in the repo today and it would make this flag the lone exception to thestore_trueconvention instead of the lone exception to it in the other direction.Tests
Two additions to
tests/unit/runtime/test_lr_schedulers.py(plain functions, no distributed setup):test_lr_range_test_staircase_is_an_opt_in_flag— the absent/present contract, asserted alongside--cycle_momentumso the two booleans stay in agreement.test_lr_range_test_staircase_reaches_scheduler_params— both directions surviveoverride_lr_range_test_paramsinto the scheduler config.Verified against this branch: 3/3 pass with the patch; 2/3 fail without it (the
False-by-default case passes either way, since that direction was never broken).yapf==0.40.0andflake8==5.0.4per.pre-commit-config.yamlboth report clean on the two changed files.🤖 Generated with Claude Code