fix: decoders and pipeline parity gaps of linen to nnx migrations - #4288
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
783a66a to
7064594
Compare
| def get_layer_to_pipeline(blocks, cfg): | ||
| if cfg.decoder_block == DecoderBlockType.DEEPSEEK: | ||
| return blocks[1] # return the sparse block | ||
| else: | ||
| return blocks[0] | ||
|
|
||
| cfg = self.config | ||
| base_stage = get_layer_to_pipeline(decoder_blocks, cfg) | ||
| if cfg.set_remat_policy_on_layers_per_stage: | ||
| policy = self.get_remat_policy() | ||
| base_stage = self.set_remat_policy([base_stage], policy)[0] | ||
| if cfg.num_layers_per_pipeline_stage == 1: | ||
| stage_module = base_stage(config=cfg, mesh=self.mesh, quant=self.quant, model_mode=self.model_mode) | ||
| elif cfg.scan_layers_per_stage: | ||
| stage_module = self.scan_decoder_layers( | ||
| cfg, | ||
| base_stage, | ||
| base_stage_cls, | ||
| cfg.num_layers_per_pipeline_stage, | ||
| "layers_per_stage", | ||
| cfg, | ||
| self.mesh, | ||
| in_axes_tuple=(nn.broadcast,) * 4, | ||
| model_mode=self.model_mode, | ||
| ) | ||
| else: | ||
| stage_module = SequentialBlockDecoderLayers( | ||
| decoder_layer=base_stage, | ||
| num_decoder_layers=cfg.num_layers_per_pipeline_stage, | ||
| config=cfg, | ||
| mesh=self.mesh, | ||
| quant=self.quant, | ||
| model_mode=self.model_mode, | ||
| self.quant, | ||
| self.model_mode, | ||
| rngs=rngs, | ||
| remat_policy=per_stage_remat, | ||
| apply_remat=apply_per_stage_remat, | ||
| ) | ||
| return stage_module |
There was a problem hiding this comment.
remove get_layer_to_pipeline dead code (unused anymore)
262fa8e to
bc467c2
Compare
c8206f0 to
b2663d2
Compare
| decoder_segment_ids=segment_ids, | ||
| deterministic=True, | ||
| model_mode=MODEL_MODE_TRAIN, | ||
| ) |
There was a problem hiding this comment.
backward pass check is missing
| cfg = self._make_ds4_config(scan_layers=False) | ||
| mesh = _make_mesh(cfg) | ||
| decoder = NNXDecoder(config=cfg, mesh=mesh, model_mode=MODEL_MODE_TRAIN, rngs=nnx.Rngs(params=0, dropout=1)) | ||
| self.assertEqual(len(decoder.layers), cfg.num_decoder_layers) |
There was a problem hiding this comment.
these are very weak checks. If we only care about shapes/length, instead of real values, we could use CPU AOT test. We don't need real TPU devices to run these tests.
| cfg = self._make_ds4_config(scan_layers=True) | ||
| _, logits, expected = self._build_and_run(cfg) | ||
| self.assertEqual(logits.shape, expected) | ||
| self.assertTrue(jnp.all(jnp.isfinite(logits))) |
There was a problem hiding this comment.
jnp.isfinite is also a weak test. We could use AOT test for similar functionalities
NuojCheng
left a comment
There was a problem hiding this comment.
Nice changes! Some comments in the test section, I feel the tests are weak, mostly checking shapes/isinfinite, which can be replaced by CPU AOT tests. Also backward pass tests seems missing. Can we compare gradients values say with/without using the selected code path, e.g.
maxtext/tests/unit/moe_test.py
Lines 604 to 723 in d30c749
cf1a449 to
8ced337
Compare
74102cb to
965d4d8
Compare
|
Hi @NuojCheng Quick summary of the gradient-parity test change. What we changed. We now do the exact gradient check on CPU only (CI's Why. That's only exactly true on CPU. On TPU the recomputed backward runs its matmuls in What was failing. The remat gradient tests on Things we tried that didn't work:
If you have anny suggestions/idea — a better way to check the backward on TPU, or a tolerance you'd prefer, or better implementation— |
09f399c to
f218929
Compare
|
I have moved the check so that it now runs on all platforms, including TPU. Before proceeding further, I would be happy to hear your thoughts and suggestions on the current approach. Below is the context on what was failing and how it is currently addressed. What was failingThe tests initially used an exact per-element check ( We tried several tolerance configurations, but none of them worked reliably: We also tried using float32 activations and setting Current Fix (already pushed on current branch)Instead of a per-element check, the current implementation bounds the aggregate relative L2 error over the entire gradient to under 5% on all platforms: A functional backward regression typically moves this norm by around 100%, whereas the On TPU (v6e-8), we observed the following relative errors: The individual outlier differences are relatively large, but they wash out in the overall norm check. Please let me know if you have any suggestions on this validation method or if you would prefer a different approach. I am happy to adjust based on your feedback. Thank you! |
| if isinstance(v, nnx.Variable) | ||
| ), ( | ||
| "Non-RngState variable found in layers_mutables catch-all partition. " | ||
| "Only RngState variables (RngKey/RngCount) should be present." |
There was a problem hiding this comment.
could you help me understand why the assertion is removed?
There was a problem hiding this comment.
The assertion was originally added as a safety check. In the old 3-way split (line 894), layers_mutables was the catch-all bucket, and we carried it through the jax.lax.scan loop. The assert ensured we didn't accidentally carry non_trainable variables (like BatchStat) through the scan, which would cause issues.
However, with models like DeepSeek-V4, the layers now legitimately contain non_trainable variables (specifically, the static hash routing tables used in MoE). These fell into the layers_mutables catch-all and triggered the assertion crash.
To fix this and support non_trainable variables, I updated the code to use a 4-way split:
- We explicitly extract
nnx.RngStateinto its own bucket (layers_rng), which is the only thing we carry through the loop. - The remaining variables fall into
layers_non_trainable, which we safely broadcast (loop-invariant) across the scan and discard the output copy.
Because we now explicitly separate the RNG state from the non-trainable state and handle both correctly, the old assertion is no longer needed.
f218929 to
d90b660
Compare
144aaa7 to
7340dce
Compare
7746a61 to
28238c1
Compare
|
Hi @NuojCheng — pushed an update and rebased onto latest main. Ready for re-review. Summary: You were right on both points you raised. I was wrong on both. Two of my earlier answers Correction 1 — the assertion removal was hiding a real bugWhat I said: the non-trainable state is "safely broadcast, and the output copy discarded." What is actually true: discarding that copy silently loses mutations. I ran a stage that adds 1 to a non-trainable value on every call, starting from 0:
The output was still finite and the right shape. Nothing warned us. Linen does this correctly. It carries the value when the collection is mutable I also had the cost backwards. I avoided carrying it because I thought it would stack per Fixed, with a regression test that fails on the old code. Correction 2 — my gradient explanation named the wrong causeWhat I said: bf16 recompute plus DeepSeek Top-K router flips. What the measurements show:
Three things are wrong with my explanation:
"L2 norm is too weak" — you were right, and it is now fixedThe problem. The old check was a single aggregate: Measured on the scanned stage (8 gradient leaves; these are norm fractions, so they combine in
So a bug confined to a bias or a layer-norm gain was invisible. What I changed.
Two different quantities are involved below, so to be unambiguous: aggregate error is over all The 2% aggregate is measured, not guessed: the worst real aggregate deviation anywhere is 1.17e-2, The per-leaf bound is deliberately not a plain relative check. It is a combined absolute+relative Why the absolute term is required — and this is the part I got wrong first. A relative-only bound
Where the bound does and does not fire. Swept, not asserted — leaf size as a fraction of total
Stated limitation: a leaf holding ≤0.1% of the total gradient norm can be arbitrarily wrong and Note that tightening the aggregate alone would not have fixed the original problem: in the 3%-leaf No false failures: decoder suite green on CPU and TPU, including the stages that show the real bf16 Your "weak checks" comment found the mostI mutation-tested the suite. The method: break the code a test covers, then check the test turns red. 15 tests stayed green while the thing they tested was broken. 8 were in this PR. All fixed, each verified by mutation:
7 were pre-existing. I fixed the worst one: making the decoder ignore its input tokens entirely Two blind spots, both closed:
Both now check the traced jaxpr by primitive identity, not by string match. ( Four source fixes
Verified clean
How to verifyXLA_FLAGS=--xla_force_host_platform_device_count=4 JAX_PLATFORMS=cpu \
python3 -m pytest tests/unit/nnx_pipeline_test.py -q # 15 passed
JAX_PLATFORMS=cpu python3 -m pytest tests/unit/nnx_decoders_test.py -q # 76 passed, 4 skipped
JAX_PLATFORMS=tpu python3 -m pytest tests/unit/nnx_{pipeline,decoders}_test.py -q # 95 passed
JAX_PLATFORMS=tpu python3 -m pytest tests/integration/pipeline_parallelism_test.py -q # 15 passedThanks for pushing back on both points. The non-trainable bug only surfaced because you asked. |
28238c1 to
eb75636
Compare
Description
Closes the remaining behavioral gaps between the pure-NNX decoder/pipeline path and the Linen reference.
The NNX path now reproduces Linen for DeepSeek-V4, per-stage remat, and the pipeline's non-trainable / repeat-level-remat handlning.
What's included
DeepSeek-V4 NNX decoder port
DEEPSEEK4inNNXDecoder.get_decoder_layer(was missing → ValueError at construction) and added full decoder-level handling: norm dispatch (RMSNorm), scanned + non-scanned init,_apply_deepseek4_scanned_blocks(prefix first_num_hash_layers unroll + paired HCA/CSA scan), global layer_idx, and decoder_input_tokens threading — matching Linen_apply_deepseek4_scanned_blocks.Per-stage pipeline remat parity (set_remat_policy_on_layers_per_stage)
NNXSequentialPipelineStage/NNXScannedPipelineStage, wired from both stage builders, incl.num_layers_per_pipeline_stage == 1.remat_policy='full'resolves toNone(== full remat, as Linen nn.remat(policy=None)); the oldif policy is not Nonegate silently dropped remat for the default 'full' policy. Now gated on the flag via an explicitapply_rematargument.Pipeline Linen→NNX migration parity (pipeline.py)
non_trainablecollection: the migration asserted the iteration-scan catch-all was RngState-only, crashing any pipelined model with a non-trainable variable (e.g. theDeepSeek-V4hash-routing table). Non-circular now broadcastsnon_trainableas a loop-invariant constant (4-way state split); circular carries it viacarry_state.Unit Tests
Tests
Sheet combination of
set_remat_policy_on_layers_per_stageflag.Checklist
Before submitting this PR, please make sure (put X in square brackets):
gemini-reviewlabel.