Repository navigation
Muon config update - #2959
Muon config update#2959
Conversation
📝 WalkthroughWalkthroughThe function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/megatron/bridge/recipes/utils/optimizer_utils.py`:
- Line 63: Fix the docstring typo for the lion_beta2 parameter in
optimizer_utils.py: change the description text that currently reads "momuntem
EMA" to "momentum EMA" (the docstring entry for lion_beta2 is the target).
- Line 37: The default value for muon_scalar_optimizer is using single quotes
which breaks ruff-format rules; update the declaration of muon_scalar_optimizer
in optimizer_utils.py (the variable named muon_scalar_optimizer) to use double
quotes for the string literal (i.e., change 'adam' to "adam") so it matches the
project's quoting convention and fixes CI formatting churn.
- Around line 99-102: The OptimizerConfig constructor is being given
muon_extra_scale_factor, muon_scalar_optimizer, lion_beta1, and lion_beta2
unconditionally which can raise TypeError on older megatron-core versions;
follow the existing pattern used for muon_nesterov and check
_OPTIMIZER_CONFIG_FIELDS before including each of these four keys into the
kwargs passed to OptimizerConfig—build a kwargs dict, conditionally add
muon_extra_scale_factor, muon_scalar_optimizer, lion_beta1, and lion_beta2 only
if they are present in _OPTIMIZER_CONFIG_FIELDS, then call
OptimizerConfig(**kwargs).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ed485335-4649-4346-bc0a-32e63c158518
📒 Files selected for processing (1)
src/megatron/bridge/recipes/utils/optimizer_utils.py
5e95bb4 to
23caed8
Compare
23caed8 to
3c1a34e
Compare
3c1a34e to
6c8d9d7
Compare
6c8d9d7 to
c55a504
Compare
|
/claude review |
| muon_extra_scale_factor=muon_extra_scale_factor, | ||
| muon_scalar_optimizer=muon_scalar_optimizer, | ||
| lion_beta1=lion_beta1, | ||
| lion_beta2=lion_beta2, |
There was a problem hiding this comment.
The existing _MUON_NESTEROV_KWARG shim (line 25) guards against OptimizerConfig not having the muon_nesterov field. Should these four new fields (muon_extra_scale_factor, muon_scalar_optimizer, lion_beta1, lion_beta2) have the same guard? If Bridge needs to support MCore versions that predate these fields, passing them directly will raise a TypeError at runtime.
There was a problem hiding this comment.
I can add this, but I don't think I should?
|
Missing test coverage: |
01ecd9e to
4b13055
Compare
|
Added tests. |
|
@janEbert please check contribute.md and fix lint. |
Signed-off-by: janEbert <janpabloe@nvidia.com>
Signed-off-by: janEbert <janpabloe@nvidia.com>
Signed-off-by: janEbert <janpabloe@nvidia.com>
Signed-off-by: janEbert <janpabloe@nvidia.com>
4b13055 to
17688d5
Compare
|
Fixed lints. |
Signed-off-by: janEbert <janpabloe@nvidia.com>
Add new Muon arguments from MCore #2241 and #3813 to
distributed_muon_with_cosine_annealing.Summary by CodeRabbit