Skip to content

Fix NNX throughput regression with scan_layers=False - #5027

Merged
copybara-service[bot] merged 3 commits into
mainfrom
fix/nnx-quant-bridge-rng-state
Sep 4, 2026
Merged

Fix NNX throughput regression with scan_layers=False#5027
copybara-service[bot] merged 3 commits into
mainfrom
fix/nnx-quant-bridge-rng-state

Conversation

@ecnal-cienet

@ecnal-cienet ecnal-cienet commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Description

NNX modules that hold an nnx.Rngs put it in the model state: a (key, count) pair per stream, which the host marshals into the jitted step on every call. Scanned, the decoder body is traced once and it is paid once. Unrolled, it is paid per layer. On llama3-8b with te_fp8_currentscaling that took the entry parameter count from 881 to 1277 and cost ~8% step time. All 396 extra parameters are RNG state.

Three places hold RNGs they never draw from:

  • The quantization bridge. ToNNX forks the caller's Rngs and keeps it, one wrapper per quantized DenseGeneral, so llama3-8b ends up with 672 streams and 673 extra scalar counter kernels per step. Kernel time is unchanged, but GPU utilization falls from 86.0% to 76.7% as those launches serialize into the dependency chain. Adds Quantization.needs_apply_rngs, default True so a backend only opts out deliberately, and ToNNX.release_rngs() to drop the fork after init. AQT keeps its RNGs: its config sets rng_type="jax.uniform", which does draw at apply time.
  • Dropout. It forked even at rate 0, where nnx.Dropout.__call__ returns its input before touching self.rngs. It now keeps the fork only when the rate can draw. It still forks either way, because fork() advances the caller's streams and skipping it would change parameter initialization.
  • AttentionOp. It held rngs only for the cudnn_flash_te bridge, which draws only with attention dropout on. It now holds them only in that case.

Also drops the lazy_init in cudnn_flash_attention, which ran a full max_target_length dummy forward pass on every call, except on the attention-sinks path. That is the one case where TE declares a variable, softmax_offset, which _inject_te_softmax_offset then grafts sinks into. Sinks therefore still lazy_init, and get a call-local Rngs when AttentionOp holds none, since the drawn value is overwritten by the graft and the wrapper never reaches the model state. Everywhere else TE declares nothing and the output is bit-identical without priming.

The moe.py hunk is unreachable today, since MoE with TransformerEngine fails earlier on 'TransformerEngineQuantization' object has no attribute 'quant_dg'. It is there so GateLogit is not left behind when that is fixed.

NVFP4 needs its RNGs. Opting the whole TransformerEngine backend out was too broad, and te_nvfp4 / te_nvfp4_no_rht died before the first step with InvalidRngError: None needs PRNG for "sr_rng". NVFP4ScalingQuantizeConfig calls make_rng("sr_rng") for the DGRAD quantizer unless stochastic rounding is disabled, and it defaults to on. With the fork released the Linen apply has no RNG collection at all, so not even Flax's fallback to params is available. needs_apply_rngs is now derived from the recipe on that backend.

For review

  • Reference HLO is regenerated. The Dropout and AttentionOp fixes apply to every NNX model. After normalizing instruction ids the only difference is the dropped u32 RNG arrays (30 llama3_8b, 54 deepseek3, 18 qwen3_1.7b) and the renumbering that follows. No operation, shape or layout changes.
  • Existing checkpoints still resume. split_for_checkpoint routes RNG state to nnx_aux, so an older checkpoint carries entries the model no longer holds and nnx.replace_by_pure_dict raises on them. train_state_nnx.apply_checkpoint_aux now skips those, logging what it skips. Only nnx_aux is filtered, so a genuinely missing weight still raises.
  • NVFP4 throughput needs GB300. On H100 the RNG failure is gone and the step compiles and launches, then dies in create_2D_tensor_map because the kernels need Blackwell. origin/main dies there identically.

FIXES: b/552606153

Testing

8xH100-80GB via NVIDIA's test-maxtext.sh, cudnn_flash_te, 21 steps, -b 1 --fsdp=8. Two images from the same tree differing only by this PR, run concurrently on separate nodes. Median step time over the last 15 steps:

model / quantization run before after
llama3-8b te_fp8_currentscaling enable_nnx=false 0.6530 0.6580
llama3-8b te_fp8_currentscaling NNX 0.7050 (+8.0%) 0.6620 (+0.6%)
llama3-8b fp8 enable_nnx=false 0.6940 0.6950
llama3-8b fp8 NNX 0.7640 (+10.1%) 0.7050 (+1.4%)

The fp8 gap is the scan-on versus scan-off difference NVIDIA reported: unrolled NNX was paying the per-layer cost that scanned NNX pays once.

Logs from the first measurement, on an earlier base: before Linen, before NNX, after Linen, after NNX.

gpt-oss-20b with attention sinks, scanned, runs on both: 21 steps, loss 6.310 before and 6.318 after. Unrolled it OOMs on both, at 43.9 GiB. te_nvfp4_no_rht no longer fails on sr_rng.

JAX_PLATFORMS=cpu pytest tests/unit/nnx_quant_bridge_rng_test.py tests/unit/attention_test.py

Each case fails if the matching fix is reverted. The bridge tests use stub backends so they run without TE, and cover the TE recipe table when it is present. The attention cases pin both directions of the lazy_init split: skipped without sinks, and still run with them, with an Rngs to initialize from.

JAX_PLATFORMS=cpu pytest tests/unit matches main: same pre-existing environment failures, none new. Losses unchanged on llama2-7b (te_fp8_currentscaling, int8, unquantized), gemma2-2b, and mixtral-8x7b / qwen3-30b-a3b for the moe.py hunk.

Checklist

Before submitting this PR, please make sure (put X in square brackets):

  • I have performed a self-review of my code. For an optional AI review, add the gemini-review label.
  • I have necessary comments in my code, particularly in hard-to-understand areas.
  • I have run end-to-end tests tests and provided workload links above if applicable.
  • I have made or will make corresponding changes to the doc if needed, including adding new documentation pages to the relevant Table of Contents (toctree directive) as explained in our documentation.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request addresses a throughput regression (b/552606153) by optimizing the NNX bridge wrapper. It removes a redundant lazy_init call on the TransformerEngine DotProductAttention bridge, which previously caused an extra forward trace per layer. Additionally, it introduces a release_rngs mechanism to drop forked Rngs after initialization for quantization backends that do not require RNGs at apply time (such as TransformerEngineQuantization), preventing unnecessary RNG state from being carried and incremented on device. Comprehensive unit tests have been added to verify these optimizations and guard against future regressions. I have no further feedback to provide as there are no review comments.

@ecnal-cienet
ecnal-cienet force-pushed the fix/nnx-quant-bridge-rng-state branch 3 times, most recently from 05cb230 to e8bfabd Compare August 27, 2026 15:45
@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.67347% with 8 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/maxtext/layers/quantizations.py 66.66% 4 Missing ⚠️
src/maxtext/layers/attention_op.py 85.71% 2 Missing ⚠️
src/maxtext/layers/linears.py 80.00% 0 Missing and 1 partial ⚠️
...trainers/diloco/utils/spmd_diloco_checkpointing.py 0.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

NNX modules that hold an nnx.Rngs put it in the model state: a (key, count) pair
per stream, which the host marshals into the jitted step on every call. Unrolled,
that is paid once per layer. On llama3-8b with te_fp8_currentscaling it took the
entry parameter count from 881 (enable_nnx=false) to 1277, and cost ~8% step time.
All 396 extra parameters are RNG state; the model arrays are identical at 291 on
both sides. Three places contribute.

1. ToNNX forks the caller's Rngs and keeps it, and MaxText bridges one wrapper per
quantized DenseGeneral: 7 TE dot_general sites x 3 streams x 32 layers = 672
streams, and 673 extra scalar counter kernels per step. Kernel time was unchanged
(+0.3%) but GPU utilization fell from 86.0% to 76.7% as those launches serialized
into the dependency chain.

Add Quantization.needs_apply_rngs, default True so a backend only opts out
deliberately, and set it False for TransformerEngineQuantization, whose dense()
takes its scales from the tensors and never calls make_rng at apply time.
ToNNX.release_rngs() then drops the fork once init is done. AQT keeps its RNGs on
purpose: its config sets rng_type="jax.uniform" and stochastic rounding does draw
at apply time, so releasing there would be a correctness bug.

2. Dropout forked the whole Rngs even at rate 0, where nnx.Dropout.__call__
returns its input before touching self.rngs. It now keeps the fork only when the
rate can draw. It still forks either way, because fork() advances the caller's
streams and skipping it would shift every later draw and change parameter
initialization.

3. AttentionOp held rngs solely for the cudnn_flash_te bridge, which only draws
when attention dropout is on. It now holds them only in that case.

Measured with NVIDIA's test-maxtext.sh on 8xH100, two images differing only by
this commit: 0.709 s/step before against 0.656 for enable_nnx=false, and 0.663
against 0.663 after. NNX now matches the Linen path exactly.

Because 2 and 3 apply to every NNX model, not just quantized ones, the reference
HLO baselines are regenerated. The diff is the dropped u32 RNG arrays -- 30 for
llama3_8b, 54 for deepseek3, 18 for qwen3_1.7b -- and the instruction renumbering
that follows. No operation, shape or layout changes.

split_for_checkpoint routes RNG state to nnx_aux, so checkpoints written before
this change carry entries the model no longer holds, and
nnx.replace_by_pure_dict raises on them. Restore was already tolerant the other
way -- an entry the checkpoint lacks keeps its fresh init value -- so
train_state_nnx.apply_checkpoint_aux now makes it tolerant both ways, logging what
it skips. Only nnx_aux is filtered; weights still go through replace_by_pure_dict,
so a genuinely missing weight keeps raising.

Also drops a lazy_init in cudnn_flash_attention that primed the TE attention
bridge with a full max_target_length dummy forward pass on every call. TE's
DotProductAttention declares no variables, Linen init() on it returns an empty
collection dict, and the output is bit-identical without it. The descriptor-count
assertions in the packing test drop from 2 to 1 and from 4 to 2 because the
second, identical SequenceDescriptor existed only to feed that lazy_init.

The moe.py hunk is unreachable today, since MoE with TransformerEngine already
fails on "'TransformerEngineQuantization' object has no attribute 'quant_dg'". It
is included so GateLogit is not the one bridged call site left behind when that
is fixed.

Fixes: b/552606153
TransformerEngine only draws at apply time for NVFP4, where it calls
make_rng("sr_rng") for the DGRAD quantizer unless stochastic rounding is
disabled. Opting the whole backend out of needs_apply_rngs released the
bridge's forked Rngs, leaving the Linen apply with no RNG collection, so
te_nvfp4 and te_nvfp4_no_rht failed before the first step with
InvalidRngError.

Derive the flag from the recipe instead. The other recipes still opt out,
so the throughput fix is unchanged: llama3-8b te_fp8_currentscaling
unrolled stays at parity with enable_nnx=false on 8xH100.

Reported by NVIDIA on 4xGB300.
Flax's fp8 ops scale from amax history and never call make_rng at apply
time, but Fp8Quantization and NANOOFp8Quantization were left at the safe
default, so every bridged DenseGeneral kept its forked Rngs. Scanned that
costs one layer body; unrolled it is paid per layer, which is the same
per-layer cost the previous commit removed for TransformerEngine.

llama3-8b, quantization=fp8, 8xH100, median step time over 15 steps:

                  NNX unrolled  NNX scan  Linen scan
    without this        0.7930    0.7000      0.6990
    with this           0.6960    0.6960      0.6960

NNX unrolled was 13% behind both NNX scanned and Linen, and is now level
with them. This is what NVIDIA reported as NNX helping much more with
scan on than with scan off.

Measured on top of the overwrite-with-gradient fix in 951086c, without
which quantization=fp8 does not train under NNX at all and the comparison
would not be meaningful.
@ecnal-cienet
ecnal-cienet force-pushed the fix/nnx-quant-bridge-rng-state branch from 597cf58 to e3a71e3 Compare September 3, 2026 23:16
@copybara-service
copybara-service Bot merged commit 1430d1a into main Sep 4, 2026
75 of 76 checks passed
@copybara-service
copybara-service Bot deleted the fix/nnx-quant-bridge-rng-state branch September 4, 2026 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants