Fix NNX MTP zero-loss, qwix MTP crash, and DeepSeek scan-axis sharding corruption#4525
Open
ecnal-cienet wants to merge 1 commit into
Open
Fix NNX MTP zero-loss, qwix MTP crash, and DeepSeek scan-axis sharding corruption#4525ecnal-cienet wants to merge 1 commit into
ecnal-cienet wants to merge 1 commit into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ecnal-cienet
force-pushed
the
fix/nnx-mtp-and-scan-axis-sharding
branch
from
July 17, 2026 19:57
38e9f54 to
f89c21e
Compare
ecnal-cienet
marked this pull request as ready for review
July 17, 2026 21:25
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
ecnal-cienet
requested review from
A9isha,
NuojCheng,
RissyRan,
SurbhiJainUSC,
abhinavclemson,
aireenmei,
bvandermoon,
darisoy,
dipannita08,
gagika,
gobbleturk,
hengtaoguo,
huytransformer,
igorts-git,
jiangjy1982,
jshin1394,
khatwanimohit,
liudangyi,
richjames0,
shralex,
suexu1025,
vipannalla and
xibinliu
as code owners
July 17, 2026 21:25
xibinliu
reviewed
Jul 17, 2026
ecnal-cienet
force-pushed
the
fix/nnx-mtp-and-scan-axis-sharding
branch
from
July 18, 2026 01:20
f89c21e to
207cb23
Compare
…g corruption
Three independent bugs in the NNX path, surfaced by running DeepSeek-V3 with
multi-token prediction.
mtp_loss was silently 0. mtp_losses/mtp_acceptance subclass nnx.Intermediate and
nnx type filters match by isinstance, so the generic nnx.pop(model,
nnx.Intermediate) in loss_fn consumed them before the dedicated pop ran. That pop
then returned empty and calculate_mtp_loss fell through to its 0.0 default. Pop
the subclasses first. Linen is unaffected; it keys mutable collections by name.
qwix quantization crashed with "roll requires ndarray, got NoneType". NNX modules
are stateful, so quantize_model must run a real forward pass, but only tokens,
positions and segment ids were passed and the MTP block reads the decoder targets
unconditionally. Pass dummy targets, restoring the semantics of the
is_initializing() guard the NNX port left commented out. Linen never traces a
forward during quantization, so it never hit this.
DeepSeek params were left replicated, tripping assert_params_sufficiently_sharded
once the crash above was fixed. _create_scanned_layers records each stack's axis
name in nnx.PARTITION_NAME ("dense_layers"/"moe_layers"), but the scan-axis
round-trip hardcoded the literal "layers". The name never matched, so the fallback
stripped a real logical axis instead of the scan axis and a bogus "layers" was
inserted: ('embed','dense_layers','mlp') became ('dense_layers','layers','mlp').
'embed' is the only carrier of fsdp, hence P(None, ...). This is neither an MTP
nor a qwix bug: it corrupts on every forward for multi-stack models, but is
normally invisible because the sharding snapshot is taken before the first train
step. qwix makes it fatal only because it quantizes inside model construction,
within the traced snapshot. Resolve the axis name per-variable from
nnx.PARTITION_NAME, and raise on inconsistent metadata rather than blindly
stripping an axis, as flax's own nnx.spmd.remove_axis does.
Verified on deepseek3-tiny (v6e-8) with sharding_tolerance untouched: nnx+mtp and
nnx+mtp+qwix now both report mtp_loss 1.224, matching the Linen reference. The
sharding metadata round-trip goes from 29 of 32 params corrupted to 0. gpt3-52k,
gpt3-52k+qwix, gemma2-2b and deepseek3-tiny with scan_layers=false all train clean.
Add regression tests for all three. multi_token_prediction_test.py drives the real
loss_fn and asserts a non-zero MTP loss; quantizations_nnx_mtp_test.py quantizes an
MTP model through qwix without crashing; maxtext_utils_nnx_test.py checks the
scan-axis helpers keep the real logical axis. Each fails on the pre-fix code.
ecnal-cienet
force-pushed
the
fix/nnx-mtp-and-scan-axis-sharding
branch
from
July 18, 2026 02:25
207cb23 to
b347f81
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes three bugs in the NNX path surfaced by running DeepSeek-V3 with multi-token prediction. They are independent; the third is not an MTP bug at all and was hiding behind the second.
1.
mtp_lossis silently 0 under NNX (b/535659954)mtp_losses/mtp_acceptancesubclassnnx.Intermediate, and NNX type filters match byisinstance. So the genericnnx.pop(model, nnx.Intermediate)inloss_fnconsumed them before the dedicated pop ran, which then returned{};calculate_mtp_lossfell through to itsreturn 0.0default. No error, just a wrong number.The comment claiming these variables are "not Intermediate, so the nnx.pop above misses them" was the false premise behind the bug.
Fix: pop the specific subclasses before the generic
nnx.Intermediatepop. Linen is unaffected — it keys mutable collections by literal name.2.
TypeError: roll requires ndarray ... got NoneTypewith qwix (b/535662227)NNX modules are stateful, so
qwix.quantize_modelmust run a real forward pass. MaxText passed only tokens/positions/segment_ids, so the MTP block receivedtarget_ids=Noneandjnp.rollraised. Linen never hits this becausequantize_linen_modelrejects model inputs and never traces a forward.Fix: supply dummy targets at that call site, gated on
mtp_num_layers > 0, restoring the semantics of theis_initializing()guard that the NNX port left commented out inmodels.py.3. DeepSeek params silently replicated under NNX (pre-existing)
Surfaced once (2) was fixed:
AssertionError: Unsharded parameter percentage (2.60%) exceeds tolerance (2.00%)._create_scanned_layersrecords each stack's axis name innnx.PARTITION_NAME—dense_layers/moe_layersfor DeepSeek — but_apply_layers_sequentiallyhardcoded the literal"layers"when round-tripping the scan axis. The name never matched, so the fallbackwhile len(l) > ndim: l.pop(0)deleted the real leading logical axis andnnx_add_scan_axisinserted a bogus'layers':'embed'is the only carrier offsdp, so the params becameP(None, ...).This is neither an MTP bug nor a qwix bug: it reproduces at
mtp_num_layers=0and with qwix entirely absent (29 of 32 params corrupted by a single plain forward). It normally stays invisible becauseget_abstract_state_nnxsnapshots shardings before the first train step, so the damage lands on a discarded traced copy. qwix makes it fatal only because it quantizes from inside model construction, i.e. inside the traced snapshot. Standard models passmetadata_axis_name="layers", matching the literal — only DeepSeek-style multi-stack models are exposed.Fix: resolve the axis name per-variable from
nnx.PARTITION_NAMErather than a caller literal, and raise instead of blindly stripping an axis when the metadata is inconsistent. Flax's ownnnx.spmd.remove_axisraises on this same mismatch; this reimplementation corrupted silently.sharding_toleranceis untouched — the params are genuinely sharded again.Test
enable_nnx=false pure_nnx_decoder=false pure_nnx=falsefor Linen, +use_qwix_quantization=true quantization=fp8_fullfor qwix.Checklist
Before submitting this PR, please make sure (put X in square brackets):
gemini-reviewlabel.