fix(muon x dspark): Markov embedding should not use Muon#866
fix(muon x dspark): Markov embedding should not use Muon#866WindChimeRan wants to merge 2 commits into
Conversation
The Muon/AdamW split promised in its own docstring that embeddings go to
AdamW, but tested `_ADAMW_NAME_HINTS = ("embed_tokens", "lm_head")` against
the parameter name. DSpark's Markov head owns an `nn.Embedding` called
`markov_head.markov_w1` -- 2D, both dims > 1, matching no hint -- so every
Muon DSpark run has been putting a token-indexed lookup table through
Newton-Schulz orthogonalization.
Match on `isinstance(module, nn.Embedding)` instead. Naming is incidental;
an embedding is a lookup table rather than a linear map between feature
spaces, so the orthogonalized update is meaningless for it however it is
spelled. Adding `markov_w1` to the hint tuple would have fixed this one
instance and rearmed the trap for the next embedding.
Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Merge Protections🔴 1 of 1 protections blocking · waiting on 👀 reviews
🔴 Require approval from approved reviewers listWaiting for any of
This rule is failing.All pull requests must have at least one approving review from a member of the approved reviewers list before merging.
|
`DSparkDraftModel.__init__` attaches the Markov and confidence heads after `super().__init__()`, which has already run `post_init()` at the end of DFlash's constructor. The heads are never visited, so they keep raw torch defaults: N(0, 1) for `markov_w1`, against the 0.02 `initializer_range` every other draft matrix gets. The head's whole output is added straight to the draft logits, so training opened against a random per-token bigram prior of std 0.58 reaching 3.3 logits. Re-running the sweep drops that to 0.006. Already-initialized modules carry `_is_hf_initialized` and are skipped, as are the NaN-filled verifier heads, so nothing else moves. This has to land with the Muon routing change rather than after it. On main the Markov embedding sits in the Muon group, whose effective decay is lr*wd = 1e-3*0.1 = 1e-4 per step against AdamW's 1e-4*0.01 = 1e-6 -- 100x. Rows that never receive gradient (most of a 150k vocab) were being dragged from 1.0 to ~0.05 over a 30k-step run, accidentally landing near the convention. Moving them to AdamW without fixing the init would leave them at ~0.97 for the whole run instead. Signed-off-by: Ranran Haoran Zhang <ranzhang@redhat.com>
Purpose
Two defects in how DSpark's Markov head is initialized and optimized. They have to land together — fixing either alone leaves training worse than it is today, for the reason in the last section.
1. The head misses the weight-init sweep
DSparkDraftModel.__init__attaches the Markov and confidence heads aftersuper().__init__(), which already ranpost_init()at the end of DFlash's constructor (dflash/core.py:124). The heads are never visited and keep raw torch defaults.Measured on a real model:
markov_head.markov_w1.weightmarkov_head.markov_w2.weightlayers.0.self_attn.q_proj.weightmarkov_w1is annn.Embedding, so it gets N(0, 1) — 50× theinitializer_rangeevery other draft matrix gets. The head's entire output is added to the draft logits (dspark/core.py:170) with nothing in between, so training opens against a random per-token bigram prior:markov_biasstdCalling
post_init()once more at the end of__init__fixes it. Modules the first pass covered carry_is_hf_initializedand are skipped, and the deliberately NaN-filledlm_head/embed_tokens/verifier_lm_headsentinels survive — verified, nothing but the two heads changes.DeepSpec builds its Markov head before
post_init()(modeling/dspark/qwen3/modeling.py:252then:268), so it getsinitializer_rangeand does not have this bug.2. Muon orthogonalizes the Markov embedding
The Muon/AdamW split promises in its docstring that embeddings go to AdamW, but tests the parameter name:
_ADAMW_NAME_HINTS = ("embed_tokens", "lm_head").markov_head.markov_w1is 2D with both dims > 1 and matches no hint, so it goes to Muon.Muon replaces the momentum-smoothed gradient with its nearest orthogonal matrix, equalizing the update across singular directions. That is meaningful for a hidden
Linear, where both axes are learned feature spaces. An embedding's row axis is a token id, so the table is a set of independent vectors rather than a linear map — orthogonalizing couples every token's update to every other token's gradient and flattens a spectrum that mostly encodes token frequency. Hence the convention this code already cites atoptimizers.py:24.Matching on
isinstance(module, nn.Embedding)rather than addingmarkov_w1to the tuple: name-matching is what created the gap, and extending it would re-arm the trap for the next embedding.Why these cannot be split
The Muon group's effective decay is
lr * wd = 1e-3 * 0.1 = 1e-4per step (muon_lrdefaults to10 * lr). The AdamW group's is1e-4 * 0.01 = 1e-6. 100×.Most rows of a 150k-entry embedding never receive gradient on a given dataset, so decay is the only force acting on them. An untrained row starting at std 1.0:
main(Muon)On
mainthe aggressive Muon decay drags untrained rows down to ~0.05 over a realistic run — accidentally near the 0.02 convention, masking the init bug. Correcting the routing alone removes that compensation while leaving the 50× init, so those rows stay at ~0.97 for the entire run and inject the full ±3.3-logit bias whenever such a token precedes. Reaching 0.05 under AdamW decay would take ~3M steps.Tests
Two, one per defect, each red before its own change and green after.
test_markov_head_is_covered_by_the_weight_init_sweep— the Markov embedding's std must match a matrix the sweep certainly covered, rather than a hardcoded constant:test_embeddings_route_to_adamw_regardless_of_name— an embedding under a name no hint matches must land in AdamW, while an ordinary matrix still reaches Muon:ruff checkandruff format --checkpass oversrc/andtests/.Notes
Only fresh training is affected; any run loading a checkpoint overwrites these weights.
Two things I deliberately did not change:
markov_head.markov_w2.weightisnn.Linear(markov_rank, draft_vocab_size), whose rows are indexed by the next token id and whose output is added to the logits — the output-side half of the same factorized token×token table, and arguably subject to the same exclusion aslm_head. Routing it would need a mechanism a model can use to declare "this matrix is token-indexed", since nothing about its type distinguishes it fromq_proj. That deserves its own design rather than another name hint.initializer_rangeis the right scale for a logit-bias head at all, versus starting it at zero so the head is a no-op at init. Matching DeepSpec is the conservative choice; near-zero is defensible.Checklist
I have filled in: