Enhance optimizer state loading with runtime overrides - #3720
Conversation
There was a problem hiding this comment.
Pull request overview
Adds checkpoint-load behavior to re-apply runtime LR bounds and scheduler progress when --override-opt-param-scheduler/--override-opt_param-scheduler is enabled, preventing optimizer state dict values from pinning the schedule to checkpoint settings.
Changes:
- Restore
max_lr/min_lron optimizer param groups from runtime args when scheduler override is enabled. - Align
opt_param_scheduler.num_stepswithargs.consumed_train_samplesand refresh LR via a no-increment scheduler step.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for param_group in optimizer.param_groups: | ||
| if ( | ||
| param_group.get('is_decoupled_lr', False) | ||
| and args.decoupled_lr is not None | ||
| ): | ||
| param_group['max_lr'] = args.decoupled_lr | ||
| param_group['min_lr'] = ( | ||
| args.decoupled_min_lr | ||
| if args.decoupled_min_lr is not None | ||
| else args.min_lr | ||
| ) | ||
| else: | ||
| param_group['max_lr'] = args.lr | ||
| param_group['min_lr'] = args.min_lr | ||
| # Synchronize scheduler num_steps with consumed_train_samples | ||
| # to ensure lr calculation is based on current training progress | ||
| if opt_param_scheduler.num_steps != args.consumed_train_samples: | ||
| print_rank_0( | ||
| f" > WARNING: scheduler num_steps ({opt_param_scheduler.num_steps}) " | ||
| f"differs from consumed_train_samples ({args.consumed_train_samples}). " | ||
| f"Resetting scheduler num_steps to match consumed_train_samples." | ||
| ) | ||
| opt_param_scheduler.num_steps = args.consumed_train_samples | ||
| opt_param_scheduler.step(increment=0) | ||
| print_rank_0( | ||
| " > restored optimizer param_group max_lr/min_lr from runtime args " | ||
| "because --override-opt_param-scheduler is set" | ||
| ) |
There was a problem hiding this comment.
args.override_opt_param_scheduler can be true even when optimizer or opt_param_scheduler is None (many call sites invoke load_checkpoint(..., None, None)). This block will then raise (e.g., optimizer.param_groups / opt_param_scheduler.num_steps). Please guard the override logic with checks that optimizer is not None, not a stub optimizer, and opt_param_scheduler is not None before accessing them (or skip with a warning).
| for param_group in optimizer.param_groups: | |
| if ( | |
| param_group.get('is_decoupled_lr', False) | |
| and args.decoupled_lr is not None | |
| ): | |
| param_group['max_lr'] = args.decoupled_lr | |
| param_group['min_lr'] = ( | |
| args.decoupled_min_lr | |
| if args.decoupled_min_lr is not None | |
| else args.min_lr | |
| ) | |
| else: | |
| param_group['max_lr'] = args.lr | |
| param_group['min_lr'] = args.min_lr | |
| # Synchronize scheduler num_steps with consumed_train_samples | |
| # to ensure lr calculation is based on current training progress | |
| if opt_param_scheduler.num_steps != args.consumed_train_samples: | |
| print_rank_0( | |
| f" > WARNING: scheduler num_steps ({opt_param_scheduler.num_steps}) " | |
| f"differs from consumed_train_samples ({args.consumed_train_samples}). " | |
| f"Resetting scheduler num_steps to match consumed_train_samples." | |
| ) | |
| opt_param_scheduler.num_steps = args.consumed_train_samples | |
| opt_param_scheduler.step(increment=0) | |
| print_rank_0( | |
| " > restored optimizer param_group max_lr/min_lr from runtime args " | |
| "because --override-opt_param-scheduler is set" | |
| ) | |
| if optimizer is None or optimizer.is_stub_optimizer or opt_param_scheduler is None: | |
| print_rank_0( | |
| " > WARNING: --override-opt_param-scheduler is set, but optimizer or " | |
| "opt_param_scheduler is not available; skipping override of scheduler " | |
| "and optimizer param_group max_lr/min_lr." | |
| ) | |
| else: | |
| for param_group in optimizer.param_groups: | |
| if ( | |
| param_group.get('is_decoupled_lr', False) | |
| and args.decoupled_lr is not None | |
| ): | |
| param_group['max_lr'] = args.decoupled_lr | |
| param_group['min_lr'] = ( | |
| args.decoupled_min_lr | |
| if args.decoupled_min_lr is not None | |
| else args.min_lr | |
| ) | |
| else: | |
| param_group['max_lr'] = args.lr | |
| param_group['min_lr'] = args.min_lr | |
| # Synchronize scheduler num_steps with consumed_train_samples | |
| # to ensure lr calculation is based on current training progress | |
| if opt_param_scheduler.num_steps != args.consumed_train_samples: | |
| print_rank_0( | |
| f" > WARNING: scheduler num_steps ({opt_param_scheduler.num_steps}) " | |
| f"differs from consumed_train_samples ({args.consumed_train_samples}). " | |
| f"Resetting scheduler num_steps to match consumed_train_samples." | |
| ) | |
| opt_param_scheduler.num_steps = args.consumed_train_samples | |
| opt_param_scheduler.step(increment=0) | |
| print_rank_0( | |
| " > restored optimizer param_group max_lr/min_lr from runtime args " | |
| "because --override-opt_param-scheduler is set" | |
| ) |
| for param_group in optimizer.param_groups: | ||
| if ( | ||
| param_group.get('is_decoupled_lr', False) | ||
| and args.decoupled_lr is not None | ||
| ): | ||
| param_group['max_lr'] = args.decoupled_lr | ||
| param_group['min_lr'] = ( | ||
| args.decoupled_min_lr | ||
| if args.decoupled_min_lr is not None | ||
| else args.min_lr | ||
| ) | ||
| else: | ||
| param_group['max_lr'] = args.lr | ||
| param_group['min_lr'] = args.min_lr |
There was a problem hiding this comment.
This overwrites max_lr/min_lr for all non-decoupled param groups with args.lr/args.min_lr, which will clobber any runtime per-group LR bounds (e.g., ParamGroupOverride / muP-generated max_lr/min_lr). If the intent is to restore the runtime config, consider capturing the pre-checkpoint optimizer.param_groups LR bounds before optimizer.load_state_dict(...) and reapplying those per-group values after loading (instead of setting everything to args.*).
| opt_param_scheduler.load_state_dict(state_dict['lr_scheduler']) | ||
| else: | ||
| opt_param_scheduler.load_state_dict(state_dict['opt_param_scheduler']) | ||
|
|
There was a problem hiding this comment.
There appears to be trailing whitespace on this blank line. Please remove it to avoid formatter/lint noise.
| opt_param_scheduler.step(increment=0) | ||
| print_rank_0( | ||
| " > restored optimizer param_group max_lr/min_lr from runtime args " | ||
| "because --override-opt_param-scheduler is set" |
There was a problem hiding this comment.
The user-facing message references --override-opt_param-scheduler, but the documented/aliased flag also uses the dashed form --override-opt-param-scheduler. Consider printing the dashed form (or both) to match typical CLI usage and the PR description.
| "because --override-opt_param-scheduler is set" | |
| "because --override-opt-param-scheduler is set" |
| if args.override_opt_param_scheduler: | ||
| for param_group in optimizer.param_groups: |
There was a problem hiding this comment.
This adds new runtime-override behavior for optimizer param groups / scheduler state during checkpoint load, but there are no tests covering the override_opt_param_scheduler path. Please add a unit test (likely extending tests/unit_tests/test_checkpointing.py::test_load_checkpoint) that sets the flag and verifies max_lr/min_lr and num_steps behavior (and that load_checkpoint(..., None, None) remains safe).
| if args.override_opt_param_scheduler: | |
| for param_group in optimizer.param_groups: | |
| if ( | |
| args.override_opt_param_scheduler | |
| and optimizer is not None | |
| and not getattr(optimizer, "is_stub_optimizer", False) | |
| and opt_param_scheduler is not None | |
| ): | |
| for param_group in getattr(optimizer, "param_groups", []): |
|
We are changing our review process and marking all open, unlabeled PRs as draft. This change will go in effect starting once #3659 is merged. Moving forward, all PRs will be required to start as draft PRs. If you wish to get your PR merged, mark your PR as “Ready for review”. Read more about the new process at submit.md. |
|
@yhgalaxy Requesting to add unit-tests |
|
What should I do? |
|
/claude fix |
|
Hi @yhgalaxy Could you fix unit tests, please? |
|
@yhgalaxy please fix failing tests |
|
/ok to test b5e4908 |
|
/ok to test a285c04 |
The optimizer state dict can overwrite per-group max_lr/min_lr values when loading a checkpoint. Previously the override path captured and restored the runtime param_group's existing max_lr/min_lr, which never applied the user's new lr settings from the command line. Re-apply runtime lr bounds directly from args after loading: regular groups get args.lr/args.min_lr, decoupled groups get args.decoupled_lr/args.decoupled_min_lr when set. Also synchronize scheduler num_steps with consumed_train_samples so lr calculation is based on current training progress. Add a unit test covering the override path, and fix the test mocks so checkpoint param_groups actually flow through save/load (MockOptimizer) and use_distributed_optimizer is disabled for the torch-format case. Signed-off-by: yhgalaxy <yhgalaxy@outlook.com>
|
/ok to test 40234ed |
|
/ok to test 28dc589 |
|
/ok to test 715ca42 |
1 similar comment
|
/ok to test 715ca42 |
|
/ok to test c4904ea |
Signed-off-by: yhgalaxy <yhgalaxy@outlook.com>
|
Minor nit: could you please make the usage of quotes consistent? I.e., in your changes, only use double quotes (which seem preferred across the code base) instead of mixing double and single quotes? Note that I don't want you to change the entire file, just your changes. |
Signed-off-by: yhgalaxy <yhgalaxy@outlook.com>
|
Thanks! |
|
/ok to test 38c5d09 |
|
/ok to test 30aa43f |
|
Hi @yhgalaxy . I just resolved the merge conflict to unblock, but looks like there's a formatting issue now. can you just run |
Signed-off-by: yhgalaxy <yhgalaxy@outlook.com>
|
/ok to test efb7eb0 |
|
/ok to test 9ddbe0d |
Signed-off-by: yhgalaxy <yhgalaxy@outlook.com>
|
/ok to test 6086b8a |
Added logic to override optimizer parameter scheduler settings based on runtime arguments when option --override-opt-param-scheduler on. This includes restoring max_lr and min_lr values and synchronizing scheduler num_steps with consumed_train_samples.
What does this PR do ?
Contribution process
flowchart LR A[Pre-checks] --> B[PR Tests] subgraph Code Review/Approval C1[Expert Review] --> C2[Final Review] end B --> C1 C2 --> D[Merge]Pre-checks
Core 0.8)Code review
The following process is enforced via the CODEOWNERS file for changes into
megatron/core. For changes outside ofmegatron/core, it is up to the PR author whether or not to tag the Final Reviewer team.For MRs into `main` branch
Feel free to message or comment the @mcore-oncall to help accelerate your merge into main. The less complex your PR is, the faster it will be approved and merged!
(Step 1): Add PR label
Expert Review(Step 2): Collect the expert reviewers reviews
Expert Reviewlabel when your PR is ready for review.Final Review might get declined if these requirements are not fulfilled.
(Step 3): Final Review
Final Reviewlabel(Optional Step 4): Cherry-pick into release branch
If this PR also needs to be merged into
core_r*release branches, after this PR has been merged, selectCherry-pickto open a new PR into the release branch.For MRs into `dev` branch
The proposed review process for `dev` branch is under active discussion.MRs are mergable after one approval by either
eharper@nvidia.comorzijiey@nvidia.com.Merging your PR
Any member of core-adlr and
core-nemowill be able to merge your PR.