Repository navigation
Conversation
|
@claude review |
|
Claude: Summary本 PR 引入 Main Flowchart after this PRflowchart TD
A["Muon.step"] --> B["_create_muon_tasks"]
B --> C["_build_muon_task<br/>_param_lr_ratio (changed)"]
C --> D{"muon_split_sizes?"}
D -- "no" --> E["self._newton_schulz_func"]
D -- "yes" --> F["partial(_muonsplit_newton_schulz, ...)"]
E --> G["muon_update_newton_schulz<br/>_muon_matrix_shape -> 2D/3D (changed)"]
F --> G
G --> H["newton_schulz_func(matrices, epsilon=epsilon)"]
H --> I["zeropower / triton<br/>accept 2D or 3D (changed)"]
H --> J["_muonsplit_newton_schulz<br/>TypeError: missing num_experts"]
style C fill:#ffe9a8
style G fill:#ffe9a8
style I fill:#ffe9a8
style J fill:#ffb3b3
核心原理实现与单测
抽象与信息隐藏评估
单测建议
其他 IssuesCritical
Warning
VerdictREQUEST_CHANGES |
c34f94f to
30eee54
Compare
|
@claude review |
|
Claude: Summary本 PR 引入 核心原理实现与单测
其他 IssuesWarning
VerdictAPPROVE |
Co-authored-by: Cursor <cursoragent@cursor.com>
Introduce a shared _muon_matrix_shape contract so orthogonalization and learning-rate scaling agree on 2D/3D logical matrices, and align the Triton Newton-Schulz API with the same layout. Co-authored-by: Cursor <cursoragent@cursor.com>
Align MuonSplit and built-in Newton-Schulz with the no-num_experts callback contract, reject non-2D expert layouts, and cover the shared matrix shape rules through Muon.step(). Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Keep curves out of the repo; attach them in the PR description instead. Co-authored-by: Cursor <cursoragent@cursor.com>
Depthwise conv1d weights ([C, 1, K]) are not matrices; routing them through Muon flattens them into degenerate logical matrices whose spectral-norm lr ratio explodes (observed x45 on Qwen3.5 conv1d, the root cause of the step-10 grad_norm spike). Route non-2D tensors to the AdamW group instead, matching Megatron and Automodel.
5405706 to
b1f007a
Compare
Motivation
Muon updates need one consistent answer to: what is the logical matrix being orthogonalized, and what
(rows, cols)shouldlr_ratiouse? Two problems hide in this question.Problem 1 — NS orthogonalization shape and
lr_ratioshape disagreeThe two places that interpret parameter layout disagreed:
muon_update_newton_schulz)flatten=Trueandndim >= 3(e.g.Conv3d/Conv1dweights), the tensor was flattened to one 2D matrix viaflatten(start_dim=1), e.g.[1152, 3, 2, 16, 16] → (1152, 1536).lr_ratio(precomputed fromp.shape[-2] // num_experts,p.shape[-1])(16, 16)for RMS scaling.So NS orthogonalized a large flattened matrix, while the effective step size was scaled as if the matrix were the tiny trailing slice — update direction and learning-rate scale described different shapes. Additionally, the NS callback reinterpreted MoE layout via a separate
num_expertsargument, making the optimizer ↔ callback contract easy to get wrong (MuonSplit / Triton / custom NS).Problem 2 — non-matrix tensors are routed into Muon at all
Qwen3.5-VL/Qwen3.5 language layers start with a depthwise causal
conv1d [8192, 1, 4](and the vision tower haspatch_embedConv3d[1152, 3, 2, 16, 16]). These are not matrices:flatten=Trueturned[8192, 1, 4]into one[8192, 4]matrix, so NS orthogonalized across channels, coupling and magnitude-equalizing 8192 independent 4-tap filters — the update direction is distorted by construction.lr_ratiowas computed on the wrong slice. The old init used(p.shape[-2], p.shape[-1]) = (1, 4)→rmsratio0.4, while NS actually ran on[8192, 4].[8192, 4], the ratio jumps to0.2*sqrt(8192) ≈ 18.10(×45.25), which reproduced as agrad_normspike at step 10 (module-grad hooks pinned it tolayers.0.self_attn.conv1d).megatron/core/optimizer/muon.py), and NeMo Automodel's dion grouping sendsndim == 2to Muon and everything else to fused AdamW (nemo_automodel/components/optim/dion.py).Fix
1.
_muon_matrix_shape— single source of truth for logical Muon layoutnum_experts > 1on 2D[E*rows, cols](E, rows, cols)— per-expert NSflatten=Trueor already 2D(rows, prod(rest))— one matrix (incl. conv flatten)flatten=Falseandndim >= 3(batch, rows, cols)— batch of last-two-dim matricesBoth
_param_lr_ratioandmuon_update_newton_schulzcall this helper, so orthogonalization shape andlr_ratioalways match. Default / Triton NS then only accept already-shaped 2D/3D inputs (no silent[E*M, N] + num_expertsreinterpretation on the optimizer path).2. Route only 2D matrices to Muon (
p.ndim == 2inMuonConfig)MuonConfignow sends 3D weights (depthwiseconv1d, Conv3d) and 1D tensors to the AdamW group, matching Megatron-LM and NeMo Automodel. MoE fused expert weights are unaffected: they are stored as 2D[E * rows, cols]and keep thenum_expertsbatch path. Regression tests intests/optim/test_muon_param_routing.py(2D → Muon,conv1d→ AdamW, 1D → AdamW, fused experts keepnum_experts; the conv1d case fails under the oldndim >= 2routing).Change summary
_muon_matrix_shape; wire NS +lr_ratiothrough it.num_expertscallback contract; harden MuonSplit accordingly.p.ndim == 2inMuonConfig); 3D weights go to the AdamW group.Results: PR vs main (Muon SFT)
Same configs,
all2all, no checkpoints. Experiment 1 is 3×PR vs 3×main repeats (mean + min–max band). Experiments 2–3 are fresh A/B runs at the final PR head (both fixes,5405706b): deterministic setup — step-1 losses match exactly on both sides.Experiment 1 — GLM-5.2 Muon SFT (35 steps; 3×PR vs 3×main repeats)
Experiment 2 — Qwen3.5-VL Muon SFT (20 steps; PR vs main, both fixes)
Experiment 3 — Qwen3.5 text-only Muon SFT (20 steps; PR vs main, both fixes)