Guard LRRangeTest and OneCycle schedulers against zero step sizes#8166
Guard LRRangeTest and OneCycle schedulers against zero step sizes#8166ebarkhordar wants to merge 1 commit into
Conversation
LRRangeTest divides the step index by self.step_size and OneCycle divides cycle_first_step_size by total_size (first + second step size), both taken unvalidated from user config. A zero step size raises a bare ZeroDivisionError instead of a clear configuration error. Reject a non-positive step size at construction with a ValueError, mirroring the existing warmup_num_steps guards (deepspeedai#8126, deepspeedai#8142, deepspeedai#8151). Valid configs are unaffected. Signed-off-by: Ehsan Barkhordar <realbarkhordar@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1673e819f8
ℹ️ 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".
| cycle_second_step_size) if cycle_second_step_size is not None else cycle_first_step_size | ||
|
|
||
| self.total_size = cycle_first_step_size + cycle_second_step_size | ||
| if self.total_size <= 0: |
There was a problem hiding this comment.
Reject zero first cycle step independently
When cycle_first_step_size=0 and cycle_second_step_size is positive, this sum-only check passes because total_size > 0, but self.step_ratio becomes 0. A get_lr() call before the first step() then enters _get_scale_factor() with x == 0 and evaluates x / self.step_ratio, raising the same ZeroDivisionError this guard is meant to prevent; DeepSpeed already exercises pre-training get_lr() via TestGetLrBeforeTrain. Please reject a zero first-step size separately, or explicitly handle a zero-length warm-up half.
Useful? React with 👍 / 👎.
Problem
Two learning-rate schedulers in
deepspeed/runtime/lr_schedules.pydivide by a step-size value taken directly from user config, with no validation, so a0step size crashes with a bareZeroDivisionErrorinstead of a clear configuration error:LRRangeTestdivides the step index byself.step_sizein_continuous_interval/_staircase_interval. Withlr_range_test_step_size=0the firststep()raisesZeroDivisionError.OneCyclecomputesself.step_ratio = cycle_first_step_size / self.total_sizein_initialize_cycle, wheretotal_size = cycle_first_step_size + cycle_second_step_size. When both halves are0, the constructor raisesZeroDivisionError.Repro (CPU-only):
The sibling
WarmupLR/WarmupCosineLRconstructors already reject invalidwarmup_num_stepsthis way (#8126, #8142, #8151); these two schedulers were skipped.Fix
Validate at construction, before the division:
LRRangeTest.__init__: reject a non-positivelr_range_test_step_sizewith aValueError, mirroring the existingwarmup_num_stepsguard exactly.OneCycle._initialize_cycle: reject a non-positivetotal_size(cycle_first_step_size + cycle_second_step_size) with aValueError.No behavior change for valid configs: the guards only fire when the value is
<= 0, which previously crashed (or, for a negativeOneCycletotal, produced a meaningless schedule).Testing
Added CPU-only regression tests next to the existing scheduler-validation tests. They raise
ZeroDivisionError(OneCycle) or silently accept the misconfig (LRRangeTest) on current master, and pass with this change:yapf, flake8, codespell clean via
pre-commit run. DCO signed off.