Fix scale_warmup: apply SSR to warmup boundary in *WithWarmupScheduler classes - #3941
Open
AnjaniKAgr wants to merge 2 commits into
Open
Fix scale_warmup: apply SSR to warmup boundary in *WithWarmupScheduler classes#3941AnjaniKAgr wants to merge 2 commits into
AnjaniKAgr wants to merge 2 commits into
Conversation
The four *WithWarmupScheduler classes computed the warmup-end boundary
with `_convert_time(self.t_warmup, state)` (ssr=1.0) even when
scale_warmup=True, while the warmup ramp (a nested LinearScheduler) was
scaled by ssr. The two halves of the schedule then disagreed about where
warmup ends: for ssr>1 the LR multiplier jumped discontinuously mid-warmup
(e.g. 0.5 -> 1.0 at the unscaled boundary), and for ssr<1 an unintended
plateau delayed the start of decay. This contradicted the documented
contract ("To scale the entire schedule, set scale_warmup=True").
Scale the boundary by ssr when scale_warmup is set, consistent with the
ramp. Behavior is unchanged when scale_warmup=False or ssr==1.0, so the
existing tests are unaffected.
Add a regression test probing the previously-buggy region (ssr=2.0). It
fails on the old code (obtains 1.0 instead of 0.5 at the mid-ramp point)
and passes after the fix.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Linear/Cosine/Polynomial WithWarmupScheduler documented the post-warmup fraction as tau_w = (t - t_warmup) / t_max, but the code uses (t - t_warmup) / (t_max - t_warmup). With the documented denominator the decay would never reach alpha_f at the end of training; the implemented denominator is correct. Fix the docstrings to match the code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.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
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?
When
scale_warmup=True, the warmup ramp is scaled by the scale-schedule ratio (viaself.warmup_scheduler(state, ssr)), but the warmup-end boundary and the post-warmup decay origin are computed from the unscaledt_warmup:The two halves of the schedule then disagree about where warmup ends, for absolute-unit warmups (
ba/ep/tok/sp):t_warmup='500ba'andssr=2.0, the ramp should span [0, 1000)ba, but the multiplier jumps from 0.5 to 1.0 at the unscaled 500ba boundary.ssr * t_warmupandt_warmup, and the decay phase is computed against the unscaled warmup end. Withssr=0.02andt_warmup='10000ba'(max_duration 1000ep), the LR sits at 1.0 for 9,800 batches — 49% of the entire run — before decay begins.This contradicts the documented contract ("SSR also scales the warmup period").
Fix: apply
ssrto the boundary computation whenscale_warmup=True, in all four affected classes (MultiStepWithWarmupScheduler,LinearWithWarmupScheduler,CosineAnnealingWithWarmupScheduler,PolynomialWithWarmupScheduler;ConstantWithWarmupSchedulerdelegates to Linear and is fixed transitively). The four edits are line-identical:The scaled
t_warmupthen flows consistently into both the boundary check and the decay fraction(t - t_warmup) / (t_max - t_warmup), so ramp endpoint and decay origin agree with no discontinuity.A companion commit corrects the post-warmup
tau_wformula in these schedulers' docstrings to match the implemented(t - t_warmup) / (t_max - t_warmup).Behavior notes
scale_warmup=False(default): unchanged — the ternary passesssr=1.0.ssr == 1.0: unchanged.dur-unit warmups: unchanged —_convert_timeignoresssrforTimeUnit.DURATIONsincemax_durationis pre-scaled by the Trainer. The bug only manifests for absolute units.Why existing tests didn't catch this
The existing
scale_warmup=Truevectors only sample points inside the scaled warmup and one point deep into decay — never at or past the boundary where the two halves disagree. Additionally, at the suite'sMAX_DURATION='1000ep', the ssr<1 error near the boundary is ~1e-4 per batch, below theabs=1e-3tolerance.Tests
Added a regression case to
test_scheduler_init:LinearWithWarmupScheduler(t_warmup='500ba', scale_warmup=True)atssr=2.0, probing the mid-ramp point at the unscaled boundary. The error there is 0.5 in LR multiplier — 500x the test tolerance.Verified locally: on unfixed
mainthe regression case fails withObtained: 1.0, Expected: 0.5 ± 1e-3(1 failed, 1051 passed); with the fix the fulltests/optim/test_scheduler.pysuite passes (1052 passed). Happy to extend the regression coverage to the other three classes if desired — the call sites are identical.