Repository navigation
feat(sft): pad to a multiple of cp and remove fp8 indexer dynamism - #3948
Merged
Merged
Conversation
CatDataset padded every row to seq_len, and the trainer counted those pads as trained tokens. Rows now pad to the next multiple of cp (as RL does), and throughput and progress/num_tokens count the tokens each rank actually trained on. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Temporary: removed before merge. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The autotune key used the exact per-rank query count, so every new packed row length re-benchmarked all six configs: 0.9 to 4.1 s per step on H200 at DeepSeek-V4-Flash shapes. Key on next_power_of_2(S_q), like S_K_BUCKET. The selected top-k indices are bitwise identical across all six configs. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…mpare.py Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… config Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Autotuning picked a config per rank from whichever row arrived first, so ranks could run different kernels and a bad first row locked in a slow one. Most of the per-layout config preference came from an unaligned key axis: with S_k padded to a multiple of 16, BLOCK_M=64 BLOCK_N=128 num_warps=4 num_stages=2 is within 1.05x of the per-shape best on 90 measured shapes, kernel time drops 9%, and tuning disappears. The padded keys lie outside every query window, so the top-k indices are bitwise identical to before. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…g.patch Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ument Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
RL throughput and progress/num_tokens multiplied the first micro batch's length by the number of micro batches, which is wrong when lengths differ. Sum each rank's row lengths and all-reduce over dp_cp, with the same code as SFT. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ng out cp Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
CP shards each row into cp contiguous chunks (ring and ulysses), so rows only need to be divisible by cp, which the SFT config already validates. The 2 * cp rule came from PyTorch's load-balanced context parallel, which prime-rl does not use. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Results and scripts stay reachable at 0ed0e78. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
garrett361
marked this pull request as ready for review
October 8, 2026 21:23
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
artzhuravel
added a commit
that referenced
this pull request
Oct 12, 2026
…ding - The objective validator refuses `model.offload = "full"` (the field that replaced `full_offload`, #3988). - Tests pass `pad_to_multiple_of` to `CatDataset` (#3948): packs are now padded to a multiple of cp, not to seq_len, so the padding test uses an explicit multiple. - `render()` carries upstream's `message_loss_mask` column (#3950), which the rebase placed there. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
artzhuravel
added a commit
that referenced
this pull request
Oct 14, 2026
…ding - The objective validator refuses `model.offload = "full"` (the field that replaced `full_offload`, #3988). - Tests pass `pad_to_multiple_of` to `CatDataset` (#3948): packs are now padded to a multiple of cp, not to seq_len, so the padding test uses an explicit multiple. - `render()` carries upstream's `message_loss_mask` column (#3950), which the rebase placed there. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
MarioSieg
added a commit
that referenced
this pull request
Oct 20, 2026
…amism (#3948)" This reverts commit 8532083. Unpadded SFT rows give each micro batch a variable token count. Inductor's sort_stable lowering in the MoE routing guards on 8 * tokens < 32767, so every block compiles twice (rows under / over ~4k tokens), and each rank compiles the second variant the first time its data crosses that size: ~8 s stalls on random steps (6, 9, 17, ... on Qwen3-30B, 2x8 H200) while the other ranks wait in the next collective. Padding packed rows to seq_len again keeps one static shape. Conflicts resolved against later commits: fp8_indexer.py keeps the eps=amax_floor argument from #3956; sft/train.py keeps the simplified defer_vlm_cp_to_model condition and restores the "pads every pack to seq_len" comment. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MarioSieg
added a commit
that referenced
this pull request
Oct 20, 2026
…able row lengths (#4160) ## Summary Since #3948, SFT pads each packed row only to a multiple of `model.cp` instead of to `seq_len`, so rows have variable lengths (2413 to 16384 tokens on the run below). With `torch.compile`, a rank recompiles its block whenever a row length breaks a guard of the compiled graph, e.g. the routing sort's `8*args[0].size()[1] >= 32767` (`sort_stable`) or the FSDP view-stride guards. The recompiling rank stalls for 6-15 s, its expert-parallel peers wait in the all-to-all, and every rank then waits in the inter-node FSDP collectives. This adds `data.pad_to_seq_len` (default `false`, so main's behaviour is unchanged). With `true`, every packed row is padded to `seq_len * micro_batch_size` again (the pre-#3948 behaviour), so rows have one shape and nothing recompiles. It also adds the config that reproduces the regression, under `benchmarks/scripts/sft_pad_to_seq_len/`. ## Results Qwen3-235B-A22B SFT, 8x8 H200, vanilla FP8 (DeepGEMM FP8 dense + `deepgemm_fp8` experts, torch all-to-all), `torch.compile` fullgraph, full AC, no offload, 64 x 16k tokens/step, 30 steps. Both runs used this branch with the same config and ran at the same time on separate node sets; only the flag differs. | `data.pad_to_seq_len` | Steps 6-30 mean | Slow (>20 s) steps | MFU | Loss @ 30 | Recompiles (all ranks) | |---|---|---|---|---|---| | `false` (main) | 20.2 s | 15 of 25, ~25 s each | 21.5% | 0.2417 | 24 | | `true` | **12.7 s** | 0 of 25, steady 12.5-13.2 s | **36.7%** | 0.2417 | 0 | Per-step times with `false` (the same steps are slow on every run): ``` 2:29.3 3:26.9 4:26.0 5:25.8 6:25.8 7:27.9 8:25.9 9:25.3 10:12.0 11:25.5 12:25.0 13:12.0 14:12.1 15:12.3 16:11.9 17:12.1 18:25.1 19:25.8 20:26.3 21:25.0 22:24.7 23:12.0 24:24.8 25:25.5 26:25.2 27:26.4 28:12.0 29:12.2 30:12.2 ``` Ruled out while bisecting: the NCCL flight recorder from #4019 (same pattern with `TORCH_FR_BUFFER_SIZE=0`), the `deep_gemm` build (same pattern with the public 2.5.0 wheel), nodes (same steps slow on three node sets), and NCCL/torch versions (unchanged). FSDP message sizes and dtypes are identical between the slow and fast configurations. A profile of a slow step shows one rank in dynamo/inductor compile for 6.5 s mid-step (`entire_frame_compile`, AOT + inductor backward compile) while its 7 expert-parallel peers spend ~20 s in the all-to-all (`ncclDevKernel_SendRecv`), and the inter-node all-gathers stretch from ~8 ms to ~66 ms per call waiting for it. ## How to benchmark it yourself Needs 8 H200 nodes (64 GPUs) with SLURM. The config is `benchmarks/scripts/sft_pad_to_seq_len/qwen3_235b_fp8.toml`; set `[slurm] partition` for your cluster. ```bash git fetch origin && git switch mario/sft-pad-to-seq-len-recompiles uv sync --all-extras # main's padding (rows padded to a multiple of cp) uv run sft @ benchmarks/scripts/sft_pad_to_seq_len/qwen3_235b_fp8.toml \ --output-dir outputs/pad-cp --run.name pad-cp # rows padded to seq_len uv run sft @ benchmarks/scripts/sft_pad_to_seq_len/qwen3_235b_fp8.toml \ --output-dir outputs/pad-seq-len --run.name pad-seq-len --data.pad_to_seq_len true ``` What to look at: - Step times in `outputs/<run>/.../logs/attempt_1/trainer/node_0.log` (`Step N | <time>s`): with `pad_to_seq_len=false`, about half the steps take ~25 s instead of ~12.6 s. - To see the recompiles, add `TORCH_LOGS = "recompiles"` under `[env_vars]` in the config and grep every node's trainer log for `Recompiling function` and the `triggered by` guard failures (`size mismatch at index 1`, `8*args[0].size()[1] >= 32767`, `args[0]._base.stride()`). Check all `node_*.log` files: the recompiles happen on whichever rank first sees a new row length, not only on rank 0. - To profile, add `trace_path = "<dir>"` at the top level and set `max_steps` to at most 9 (the trainer refuses longer traces); steps 2-9 are all slow on this data. Running both configs at the same time on separate node sets keeps cluster noise out of the comparison. If the Hugging Face Hub is flaky, point `data.name` at a local snapshot of `PrimeIntellect/INTELLECT-3-SFT-10K`. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Opt-in dataloader padding change with default false; no change to training math beyond padding length when enabled. > > **Overview** > Adds an opt-in **`data.pad_to_seq_len`** flag (default **`false`**) on SFT data config so packed training rows can use a **fixed token length** again. > > When enabled, **`setup_dataloader`** pads each packed row to **`seq_len * micro_batch_size`** instead of only to a multiple of **`model.cp`**. That restores one static sequence shape per step, which the PR targets at **`torch.compile`** / DeepGEMM JIT recompiles when row lengths vary across steps. The tradeoff is **extra padding**; leaving the flag off keeps current padding behavior unchanged. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 7c4bfb5. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
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.
SFT now pads packed rows only to a multiple of cp, as RL does, instead of to
seq_len. The fp8 indexer drops its autotuning, which re-benchmarked configs on every new row length and made unpadded rows slower than padded ones on DeepSeek-V4 (#3726).seq_len× rows or the first row × count.seq_len % (2 * cp)check is gone: CP only needsseq_len % cp, which configs enforce.Same samples per arm, time summed over steps 5 to 20, one H200 node:
On DeepSeek-V4, the 1.60x exceeds the 1.25x that removing 19.8% of tokens predicts because two effects multiply: main's token-0 pads crowd onto the same experts, and the padding itself costs compute. The DeepSeek-V4 ablation section below separates the two.
On GLM with scaleswe, removing 19.7% of the tokens would predict about 1.25x, but it measured 1.13x. Its 4 data-parallel ranks now get rows of different lengths, and each step likely waits for the rank with the longest rows.
Loss matches main to within 1e-3 per step. This overlaps #3932 in
CatDataset._finalize_pack. Harness:benchmarks/scripts/sft_pad_to_cp/(removed before merge).DeepSeek-V4 ablation
Each row changes one thing on the DeepSeek-V4 workload above, to separate the cost of the padding length, of token-0 pads crowding onto the same experts, and of indexer re-tuning:
seq_lenwith token 0Alternatives considered
next_power_of_2(S_q)reached 1.47x on DeepSeek-V4. Each bucket kept its first row's config (up to 1.44x slower later), short rows paid extra tunes, and each rank still tuned on its own rows, so ranks could run different kernels.🤖 Generated with Claude Code