Repository navigation
Fix OneCycle overwriting beta2, starting at the wrong momentum, and get_last_lr before step - #9048
davrens101 wants to merge 2 commits into
Conversation
…et_last_lr before step OneCycle cycles only betas[0], but it rebuilt every param group's betas as (mom, 0.99), so the optimizer's beta2 was replaced at construction and on every step(). It also set the initial beta1 to cycle_min_mom although the schedule is at cycle_max_mom at step 0 (as get_mom() reports), and it discarded the initial lr it applied, so get_last_lr() asserted before the first step() while the other schedulers return it. Keep each group's own beta2, start at cycle_max_mom, and record the initial lr in _last_lr. Signed-off-by: davrens101 <davis@example.com>
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b45fddd920
ℹ️ 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".
| assert [group["betas"] for group in optimizer.param_groups] == [(0.9, 0.999), (0.9, 0.95)] | ||
|
|
||
| for _ in range(3): | ||
| scheduler.step() |
There was a problem hiding this comment.
Exercise the beta changes in an integration training loop
This change alters the Adam beta values consumed by the first and subsequent model updates, but this loop only invokes scheduler.step() on a bare optimizer and never performs an optimizer/DeepSpeed training step. The existing TestOneCycle.test_mom integration loop only records get_mom()—which already returned the expected maximum before this patch—so it would not catch either fixed optimizer-beta defect. Add and execute a minimal SimpleModel training test that verifies the optimizer betas or resulting parameters through actual updates.
AGENTS.md reference: AGENTS.md:L81-L82
Useful? React with 👍 / 👎.
| if last_batch_iteration == -1: | ||
| update_lr(optimizer.param_groups, self.min_lrs) | ||
| # Record the initial lr so get_last_lr() works before the first step(), like the other schedules. | ||
| self._last_lr = update_lr(optimizer.param_groups, self.min_lrs) |
There was a problem hiding this comment.
Refresh
_last_lr after restoring OneCycle state
When a stage-0 checkpoint taken after the first scheduler step is restored and get_last_lr() is queried before the next step, this constructor value is stale: DeepSpeedEngine._load_checkpoint restores the optimizer at engine.py:4735-4738 and then OneCycle.load_state_dict() restores only last_batch_iteration, leaving _last_lr at cycle_min_lr. The method therefore silently reports the minimum instead of the optimizer LR saved in the checkpoint; persist/restore _last_lr or refresh it from the restored param groups.
Useful? React with 👍 / 👎.
…estore P1: add integration training test exercising betas through an actual DeepSpeed optimizer step so the beta fix (cycle only betas[0], keep each group's beta2) is verified end to end. P2: persist _last_lr in OneCycle.state_dict()/load_state_dict() so a restored checkpoint reports the optimizer LR rather than a stale cycle_min_lr when get_last_lr() is queried before the next step. Signed-off-by: davrens101 <davis@example.com>
|
Pushed fixes to this branch: persisted |
Fixes #8750
Fixes #8951
Fixes #8752
What was wrong
Three small defects in
OneCycle(deepspeed/runtime/lr_schedules.py) all sit in the same two initializer methods, so I fixed them together._initialize_momentumbuilt every param group's betas as(mom, 0.99). Onlybetas[0]is meant to cycle, so the optimizer's own beta2 (Adam/AdamW default 0.999, or a per-group value) was replaced with 0.99 when the scheduler was constructed and again on everystep()([BUG] OneCycle overwrites the optimizer's beta2 with 0.99 #8750).betas[0]was set tocycle_min_mom. The schedule at step 0 is atcycle_min_lr, whereget_mom()returnscycle_max_mom, so the first optimizer update used a momentum the schedule never reports ([BUG] OneCycle applies the minimum momentum to the first Adam update #8951)._initialize_lrappliedcycle_min_lrto the optimizer but threw the returned lrs away, soget_last_lr()raisedAssertionError: need to call step() firstuntil the firststep().WarmupLR,WarmupDecayLR,WarmupCosineLRandLRRangeTestall return the initial lr ([BUG] OneCycle.get_last_lr() raises before the first step() while other schedules return the initial lr #8752).How I saw it
On master (6f96b4a), Adam with
betas=(0.9, 0.999),OneCycle(opt, cycle_min_lr=1e-4, cycle_max_lr=1e-3, cycle_first_step_size=10):With the change,
after initis(0.9, 0.999), matchingget_mom()apart from the unchanged beta2,get_last_lr()returns[0.0001], and beta2 stays 0.999 (and 0.95 for a group created with 0.95) through the cycle.What changed
_initialize_momentumreads each group's existingbetas[1]and buildsmin_moms/max_momsfrom it, then starts the optimizer atmax_moms(only whenlast_batch_iteration == -1, as before)._initialize_lrstores the result ofupdate_lrin_last_lr.tests/unit/runtime/test_lr_schedulers.py: two new non-distributed tests (per-group beta2 kept and initial momentum matchesget_mom();get_last_lr()before the first step). One existing assertion intest_one_cycle_accepts_per_group_lr_and_momentum_listsexpected the initial beta1 to becycle_min_mom(0.8, 0.85); it now expectscycle_max_mom(0.9, 0.95). Its later peak/bottom assertions are unchanged.What I ran
CPU only, Windows 11, torch 2.15.0+cpu (as reported by
torch.__version__in my environment), Python 3.12, CPU accelerator.deepspeed/acceleratorand twoopsentries are git symlinks that Windows checks out as text files, so I replaced them locally with directory junctions (marked skip-worktree, not part of the commit).pytest tests/unit/runtime/test_lr_schedulers.py -k one_cyclegave 3 failed, 12 passed (the two new tests plus the updated per-group test).test_lr_schedulers.pymodule: 45 passed, 45 failed. All 45 failures are theDistributedTestclasses failing withValueError: cannot find context for 'forkserver', which is a Windows limitation of the test launcher, not this change. I readTestOneCycle::test_lr/test_mom: they useget_lr()/get_mom()and do not depend on the initial optimizer betas, but I could not run them.Unpublished follow-up: restoring
_last_lrfrom an older checkpointThe local follow-up commit
3df189a6is not pushed; this PR remains at2b8b59d34add1d6225d53ac49cc54d61e12c2f1e. It remains held for human review, DCO attestation and the outstanding validation described below. It addstest_one_cycle_get_last_lr_after_checkpoint_restorecoveringload_state_dictdirectly:get_last_lr()on a scheduler restored from a checkpoint written before_last_lrwas stored, and the same thing withlast_batch_iterationreused from the checkpoint.I ran the four parameter combinations against the real scheduler/optimizer/state-dict code (isolated harness, exact source):
self._last_lrdefault)So 3 of 4 configurations fail without the follow-up; all 4 pass with it. The module-level run needs the Windows symlink workaround described above; this isolated proof uses the same classes and logic.
Honest limits
Not validated: any GPU, ZeRO stage, offload or precision configuration, and the distributed
TestOneCyclecases. This only changes scheduler bookkeeping that runs on the host before any ZeRO or precision handling.WarmupLR/WarmupDecayLR/WarmupCosineLR/LRRangeTestdo not touch betas, so there was no sibling momentum path to check. Users who relied on the old beta2 of 0.99 underOneCyclewill now get the optimizer's configured value; that is the documented behavior but it is a numerical change for them.Correction: this PR duplicates earlier open work. #8495 and #8751 preserve beta2, #8753 records the initial learning rate, and #8953 fixes the initial momentum endpoint. My earlier duplicate search missed those PRs. I am withdrawing this combined PR rather than competing with them; the local checkpoint-restore follow-up remains unpublished.
I used an AI coding assistant to write and test this change, and the numbers above come from its runs on my machine.