Match Tunix peft_trainer_v2 performance in MaxTextTrainingEngine - #5088
Match Tunix peft_trainer_v2 performance in MaxTextTrainingEngine#5088NuojCheng wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces significant performance optimizations and fixes for gradient accumulation and training engine overhead in MaxText. Key changes include caching the pure-state representation of the NNX model to avoid costly graph re-splitting, accumulating unreduced gradients to divide by the accumulated denominator once during updates, bounding the metrics history to prevent HBM bloat, and properly handling PartitionSpecs with reduced/unreduced tags across layers and sharding helpers. The review feedback highlights potential AttributeError issues in moe.py and normalizations.py when partition specs are None in single-device or unsharded environments, providing actionable code suggestions to guard these accesses.
| if weight_gather: | ||
| # Read `.partitions`: a kernel spec carrying a reduced axis refuses direct indexing. | ||
| wi_partitions = w0_pspec.partitions | ||
| # weight_gather implies either exp or embed_moe is sharded. | ||
| if self.config.shard_exp_on_fsdp: | ||
| # wi [Experts, In, Hidden] -> Gather Exp(0) | ||
| wi_gather_axes.extend(get_active_sharding_axes(w0_pspec[0], 0)) | ||
| wi_gather_axes.extend(get_active_sharding_axes(wi_partitions[0], 0)) | ||
| else: | ||
| # Gather In(1) where embed_moe is sharded. | ||
| wi_gather_axes.extend(get_active_sharding_axes(w0_pspec[1], 1)) | ||
| wi_gather_axes.extend(get_active_sharding_axes(wi_partitions[1], 1)) | ||
| # Gather Hidden(2) | ||
| wi_gather_axes.extend(get_active_sharding_axes(w0_pspec[2], 2)) | ||
| wi_gather_axes.extend(get_active_sharding_axes(wi_partitions[2], 2)) |
There was a problem hiding this comment.
If self.mesh is None (e.g., in single-device or local test environments), w0_pspec will be None. In this case, accessing w0_pspec.partitions will raise an AttributeError. We should guard this block with a check to ensure w0_pspec is not None before accessing its partitions.
| if weight_gather: | |
| # Read `.partitions`: a kernel spec carrying a reduced axis refuses direct indexing. | |
| wi_partitions = w0_pspec.partitions | |
| # weight_gather implies either exp or embed_moe is sharded. | |
| if self.config.shard_exp_on_fsdp: | |
| # wi [Experts, In, Hidden] -> Gather Exp(0) | |
| wi_gather_axes.extend(get_active_sharding_axes(w0_pspec[0], 0)) | |
| wi_gather_axes.extend(get_active_sharding_axes(wi_partitions[0], 0)) | |
| else: | |
| # Gather In(1) where embed_moe is sharded. | |
| wi_gather_axes.extend(get_active_sharding_axes(w0_pspec[1], 1)) | |
| wi_gather_axes.extend(get_active_sharding_axes(wi_partitions[1], 1)) | |
| # Gather Hidden(2) | |
| wi_gather_axes.extend(get_active_sharding_axes(w0_pspec[2], 2)) | |
| wi_gather_axes.extend(get_active_sharding_axes(wi_partitions[2], 2)) | |
| if weight_gather and w0_pspec is not None: | |
| # Read `.partitions`: a kernel spec carrying a reduced axis refuses direct indexing. | |
| wi_partitions = w0_pspec.partitions | |
| # weight_gather implies either exp or embed_moe is sharded. | |
| if self.config.shard_exp_on_fsdp: | |
| # wi [Experts, In, Hidden] -> Gather Exp(0) | |
| wi_gather_axes.extend(get_active_sharding_axes(wi_partitions[0], 0)) | |
| else: | |
| # Gather In(1) where embed_moe is sharded. | |
| wi_gather_axes.extend(get_active_sharding_axes(wi_partitions[1], 1)) | |
| # Gather Hidden(2) | |
| wi_gather_axes.extend(get_active_sharding_axes(wi_partitions[2], 2)) |
| if weight_gather: | ||
| # Read `.partitions`: a kernel spec carrying a reduced axis refuses direct indexing. | ||
| wo_partitions = wo_pspec.partitions | ||
| # weight_gather implies either exp or embed_moe is sharded. | ||
| if self.config.shard_exp_on_fsdp: | ||
| # wo [Experts, Hidden, Out] -> Gather Exp(0) | ||
| wo_gather_axes.extend(get_active_sharding_axes(wo_pspec[0], 0)) | ||
| wo_gather_axes.extend(get_active_sharding_axes(wo_partitions[0], 0)) | ||
| else: | ||
| # Gather Out(2) where embed_moe is sharded. | ||
| wo_gather_axes.extend(get_active_sharding_axes(wo_pspec[2], 2)) | ||
| wo_gather_axes.extend(get_active_sharding_axes(wo_partitions[2], 2)) | ||
| # Gather Hidden(1) | ||
| wo_gather_axes.extend(get_active_sharding_axes(wo_pspec[1], 1)) | ||
| wo_gather_axes.extend(get_active_sharding_axes(wo_partitions[1], 1)) |
There was a problem hiding this comment.
Similarly to w0_pspec, wo_pspec can be None if self.mesh is None. We should guard this block to ensure wo_pspec is not None before accessing its partitions.
| if weight_gather: | |
| # Read `.partitions`: a kernel spec carrying a reduced axis refuses direct indexing. | |
| wo_partitions = wo_pspec.partitions | |
| # weight_gather implies either exp or embed_moe is sharded. | |
| if self.config.shard_exp_on_fsdp: | |
| # wo [Experts, Hidden, Out] -> Gather Exp(0) | |
| wo_gather_axes.extend(get_active_sharding_axes(wo_pspec[0], 0)) | |
| wo_gather_axes.extend(get_active_sharding_axes(wo_partitions[0], 0)) | |
| else: | |
| # Gather Out(2) where embed_moe is sharded. | |
| wo_gather_axes.extend(get_active_sharding_axes(wo_pspec[2], 2)) | |
| wo_gather_axes.extend(get_active_sharding_axes(wo_partitions[2], 2)) | |
| # Gather Hidden(1) | |
| wo_gather_axes.extend(get_active_sharding_axes(wo_pspec[1], 1)) | |
| wo_gather_axes.extend(get_active_sharding_axes(wo_partitions[1], 1)) | |
| if weight_gather and wo_pspec is not None: | |
| # Read `.partitions`: a kernel spec carrying a reduced axis refuses direct indexing. | |
| wo_partitions = wo_pspec.partitions | |
| # weight_gather implies either exp or embed_moe is sharded. | |
| if self.config.shard_exp_on_fsdp: | |
| # wo [Experts, Hidden, Out] -> Gather Exp(0) | |
| wo_gather_axes.extend(get_active_sharding_axes(wo_partitions[0], 0)) | |
| else: | |
| # Gather Out(2) where embed_moe is sharded. | |
| wo_gather_axes.extend(get_active_sharding_axes(wo_partitions[2], 2)) | |
| # Gather Hidden(1) | |
| wo_gather_axes.extend(get_active_sharding_axes(wo_partitions[1], 1)) |
| scale_spec = jax.typeof(scale).sharding.spec | ||
| if scale_spec[-1] == activation_spec[-1]: | ||
| if scale_spec.partitions[-1] == activation_axis: | ||
| return scale | ||
| return jax.sharding.reshard(scale, jax.sharding.PartitionSpec(activation_spec[-1])) | ||
| return jax.sharding.reshard( | ||
| scale, | ||
| jax.sharding.PartitionSpec(activation_axis, unreduced=scale_spec.unreduced, reduced=scale_spec.reduced), | ||
| ) |
There was a problem hiding this comment.
If scale is not sharded (e.g., in single-device mode or if auto-sharding is used and no sharding is assigned yet), scale_spec can be None. In this case, accessing scale_spec.partitions will raise an AttributeError. We should guard against scale_spec is None and return scale directly.
| scale_spec = jax.typeof(scale).sharding.spec | |
| if scale_spec[-1] == activation_spec[-1]: | |
| if scale_spec.partitions[-1] == activation_axis: | |
| return scale | |
| return jax.sharding.reshard(scale, jax.sharding.PartitionSpec(activation_spec[-1])) | |
| return jax.sharding.reshard( | |
| scale, | |
| jax.sharding.PartitionSpec(activation_axis, unreduced=scale_spec.unreduced, reduced=scale_spec.reduced), | |
| ) | |
| scale_spec = jax.typeof(scale).sharding.spec | |
| if scale_spec is None: | |
| return scale | |
| if scale_spec.partitions[-1] == activation_axis: | |
| return scale | |
| return jax.sharding.reshard( | |
| scale, | |
| jax.sharding.PartitionSpec(activation_axis, unreduced=scale_spec.unreduced, reduced=scale_spec.reduced), | |
| ) |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
a980351 to
0548159
Compare
`MaxTextTrainingEngine` was slower than Tunix's `PeftTrainer` v2 on the same model, the same loss and the same optimizer, and the gap was entirely host-side. Three changes to the step path close it, plus two correctness fixes the measurement turned up. Performance: - Trace the accumulation and update kernels under `nn_partitioning.axis_rules`, so gradient accumulation is fused into the compiled program instead of running as eager `jax.tree.map(add)` over every leaf, every micro-batch. - Carry a pure `nnx.State` mirror across steps in place of a per-step `nnx.split` of the module graph. The split was two traversals per step, scaling with node count -- 21.3 ms on an unscanned qwen3-0.6b's 310 parameter leaves -- and it allocated a large short-lived object graph twice a step, whose GC pauses landed on whichever step was unlucky (a 464 ms worst step against a 161.6 ms median). - Defer the metrics write until after the next dispatch, so its device-to-host read overlaps live work rather than blocking on it. Correctness: - Accumulate gradients unreduced and divide once by the summed denominator, rather than averaging per-micro-batch means. The two agree only when every micro-batch has the same token count; on ragged batches the mean-of-means gradient is simply the wrong one. The denominator rides along in checkpoint metadata so a mid-step resume can finish the division, and is backfilled from the restored per-micro-batch losses for checkpoints written before it was tracked. - Compute the gradient norm on every update, not only under `skip_step_on_spikes`. `learning/grad_norm` is a metric `trainers/pre_train/train.py` reports each step and `metrics._METRICS_TO_LOG` already lists, but with spike-skipping off -- base.yml's default -- it read NaN. Tunix computes the same quantity unconditionally in `peft_trainer_v2._update_step`. Taken after clipping, in float32, it now matches Tunix bit for bit: 59.77279281616211 at GA=1 and 18.25168800354004 at GA=8. The norm also gives the inflight throttler a scalar to block on, so it no longer holds the whole train state alive between steps -- three parameter trees pinned per queued entry, whose buffers a later donating update would delete out from under `jax.block_until_ready`. Measured on qwen3-0.6b, batch 8 x seq 1024, f32, `optax.sgd(1e-5)`, no clipping, as an A/B on these files alone. Per optimizer step, medians of 19 steps after warmup: at GA=1 on 8 x v7x, 161.6 -> 89.8 ms (1.80x); at GA=8 on 4 x v6e, 3458.9 -> 589.6 ms (5.87x), against 1225.9 ms for the same MaxText model under `PeftTrainer` (2.08x, at 96% device utilization against 50%). TPU-busy time is identical across the A/B to one part in 340,000 and the accumulation kernel keeps the same XLA module hash, so all of it is host time. Tests: `tests/post_training/unit/maxtext_engine_test.py` covers the cached pure-state path end to end on a model with non-`Param` state, the unreduced accumulation and its denominator, the denominator's checkpoint round-trip, the throttler's scalar handle, and the gradient norm with spike-skipping off. 59 passed across the engine unit tests.
The engine now traces its kernels under `nn_partitioning.axis_rules`, which is what makes MaxText's logical constraints on the activations real. That turns the test's hard-coded batch of 2 into a problem on any host with more than 2 data x fsdp devices: XLA pads the batch out, the padded lanes are all-zero sequences that mask themselves out of attention entirely, and their contribution comes back as NaN on the pad token's embedding row -- a finite loss with unusable gradients, so the "a parameter moved" assertion saw nan. The splash kernel asserts on exactly this ratio; dot_product does not, which is why it surfaced as a NaN rather than an error. Derive the batch from the mesh instead, and pin matmul_precision. The KL assertion compares log-probs from two code paths -- tunix's compute_per_token_logps for the reference and the engine's sharded forward for the policy -- and at bf16 they disagree by ~2e-2 on values near -12.6, which low_var_kl squares into ~1e-4.
52e6981 to
655811b
Compare
Same code, 131 fewer lines of prose. Keeps the load-bearing ones -- why params are not donated, why the throttler queues a scalar, why the batch has to be shardable -- and drops the measurements and narrative that belong in the write-up rather than the source.
Description
MaxTextTrainingEnginewas slower than Tunix'sPeftTrainerv2 on the same model, the same loss function and the sameoptaxtransformation, and the gap turned out to be entirely host-side. This PR closes it, and fixes two correctness problems the measurement turned up along the way.Five files, no new scripts:
src/maxtext/training_engine/maxtext_engine.pysrc/maxtext/training_engine/inflight_throttler.pysrc/maxtext/training_engine/metrics.pytests/post_training/unit/maxtext_engine_test.pytests/post_training/integration/maxtext_engine_grpo_loss_test.pyPerformance
Trace the accumulation and update kernels under
nn_partitioning.axis_rules, so gradient accumulation is fused into the compiled program instead of running as an eagerjax.tree.map(add)over every gradient leaf on every micro-batch.This is the one behaviour change in the PR, as opposed to a timing change. Those rules live in a context variable, so a kernel traced without them saw an empty rule set and every
maybe_shard_with_logicalinside the MaxText layers was silently a no-op; making them real meansmicro_batch_size_to_train_onmust now be a multiple ofdata x fsdp. A batch those devices cannot split gets padded out by XLA, and the padded lanes are all-zero sequences that mask themselves out of attention entirely — their contribution comes back as NaN on the pad token's embedding row, giving unusable gradients under a finite loss. MaxText's splash kernel already asserts on exactly this ratio ("Batch dimension should be shardable among the devices in data and fsdp axis",attention_op.py);dot_productdoes not, so there it surfaces as a NaN rather than an error. Callers relying on an undersized micro-batch are the group affected, and in practice such a batch was already leaving devices idle. This is how it showed up: the GRPO integration test hard-coded a batch of 2 and ran on a 4-device host, hence the fifth file above.Carry a pure
nnx.Statemirror across steps in place of a per-stepnnx.splitof the module graph. The split was two graph traversals per step, scaling with node count — 21.3 ms on an unscanned qwen3-0.6b's 310 parameter leaves — and it allocated a large short-lived object graph twice per step, whose GC pauses landed on whichever step was unlucky (a 464 ms worst step against a 161.6 ms median).Defer the metrics write until after the next dispatch, so its device-to-host read overlaps live work rather than blocking on it.
Correctness
Accumulate gradients unreduced and divide once by the summed denominator, rather than averaging per-micro-batch means. The two agree only when every micro-batch carries the same number of tokens; on ragged batches, mean-of-means is simply not the gradient of the batch loss. The denominator rides along in checkpoint metadata so a mid-step resume can finish the division, and is backfilled from the restored per-micro-batch losses for checkpoints written before it was tracked.
Compute the gradient norm on every update, not only under
skip_step_on_spikes.learning/grad_normis a metrictrainers/pre_train/train.pyreports every step andmetrics._METRICS_TO_LOGalready lists, but with spike-skipping off — base.yml's default — it read NaN. Tunix computes the same quantity unconditionally insidepeft_trainer_v2._update_step.202a89ab8landed the same fix on main while this was in review; rebasing onto it, I kept main's placement — before clipping — over this branch's post-clipping one, because that is where Tunix'soptax.global_normsits (its clipping, when a caller configures any, is a later link in the optax chain). Intrain.py's vocabulary that makes itraw_grad_normrather thanlearning/grad_norm. What this PR still adds on top of main's version is the float32 reduction — a sum of squares over bf16 leaves overflows on production-size models — and the norm doubling as the throttler's handle. It agrees with Tunix bit for bit:gradient_normgrad_normIt costs ~0.6 ms per update — 0.7% of a GA=1 step, 0.1% of a GA=8 one, and below the noise floor on device (568.0 vs 570.7 ms TPU-busy per optimizer step at GA=8), because XLA fuses the reduction into an update kernel that already streams every gradient leaf.
The norm also gives
InflightThrottlera scalar to block on. It previously queued the train state itself, which pinned three parameter trees alive for as long as the entry sat in the queue, and — once the update kernel donates its state argument — could raiseArray has been deletedout ofjax.block_until_readywhen an entry was popped after a later step had reused those buffers.Results
qwen3-0.6b, batch 8 × seq 1024, f32,
optax.sgd(1e-5), no clipping, measured as an A/B on these four files alone — the "before" arm isgit checkout origin/main -- src/maxtext/training_engine/on this same commit, so nothing else differs. 4 × v6e,fsdp=4, unscanned, median of 19 steps after warmup, no profiler attached:main)Gradient accumulation multiplies the win, because the removed
nnx.splitsat infwd_bwd, which runs once per micro-batch rather than once per optimizer step. The process-wall-clock rows are lower than the rest because ~25 s of interpreter start, imports, config and engine build is fixed cost this PR does not touch.The saving is a fixed quantity of host time, so what it is worth also depends on how big the NNX module graph is —
scan_layerscollapses a decoder stack into one stacked node set, so a scanned qwen3.5-35b-a3b (70 parameter leaves) sees 2322.5 → 2314.0 ms/step, a 1.004x median, while its worst step still drops 2641.8 → 2314.6 ms as the GC pauses go away — and on how much device time there is to hide host work behind.Against Tunix's trainer at GA=8, driving the same MaxText model, so only the trainer differs (measured on an earlier base, where this PR's arm read 589.6 ms rather than 613.0):
MaxTextTrainingEnginePeftTrainer, MaxText modelTwo independent checks that the win is host-only: TPU-busy time across the GA=8 A/B is 3408.12 vs 3408.13 ms over the six optimizer steps each trace covers — agreement to one part in 340,000 — and
jit_accum_kernelcarries the same XLA module hash (13160937502384084184) on both sides.Full write-up, methodology and xplane traces: #5060.
Tests
tests/post_training/unit/maxtext_engine_test.py, extended here to cover:Paramstate and over two steps, which is where a stale or wrongly-partitioned cache would show up;gradient_normbeing recorded withskip_step_on_spikesoff — the configuration that used to produce nothing.On TPU,
tests/post_training/integration/maxtext_engine_grpo_loss_test.pypasses against the real GCS checkpoint (1 passed in 51.67 s), and the numerical parity harness in #5060 shows identical loss and gradients agreeing torel_l23.0e-4 against Tunix at GA=1. Two things in that test are now load-bearing: its batch comes frommesh.shape["data"] * mesh.shape["fsdp"]rather than a hard-coded 2, for the reason above; andmatmul_precision=highestis pinned, because its KL assertion compares log-probs from two code paths — Tunix'scompute_per_token_logpsfor the reference against the engine's sharded forward for the policy — which at bf16 disagree by ~2e-2 on values near -12.6, andlow_var_klsquares that into a KL of ~1.2e-4, an order of magnitude above the tolerance the assertion is meant to police.Checklist
Before submitting this PR, please make sure (put X in square brackets):
gemini-reviewlabel.