Do the chunked-CE lm_head projection on tensor cores instead of in fp32 - #6863
Open
qgallouedec wants to merge 4 commits into
Open
Do the chunked-CE lm_head projection on tensor cores instead of in fp32#6863qgallouedec wants to merge 4 commits into
lm_head projection on tensor cores instead of in fp32#6863qgallouedec wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00c9b2d569
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
qgallouedec
force-pushed
the
sft-chunked-ce-tensorcore
branch
4 times, most recently
from
August 24, 2026 20:30
0078e2d to
f3db7b0
Compare
`DistillationTrainer._chunk` runs the same upcast-then-fp32-GEMM twice, once for the student and once for the teacher, and the experimental async distillation trainer has a third copy. Same change, same reasoning: both operands are already the model dtype, so upcasting them buys nothing and costs the tensor cores. The `grad_logits @ w_chunk.float()` in utils.py is deliberately left alone -- there `grad_logits` really is fp32, so that upcast is load-bearing.
Under FSDP2 mixed precision the patched forward reads the fp32 sharded master weight directly while the hidden states come out bf16, so the tensor-core projection failed with a dtype mismatch (tests/distributed test_sft_peft[fsdp2]). Cast the weight to the hidden-states dtype, which is what lm_head's own forward computes in. Same fix in all three copies: SFT, distillation, async distillation.
qgallouedec
force-pushed
the
sft-chunked-ce-tensorcore
branch
from
August 25, 2026 17:12
f3db7b0 to
f99ca55
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.
_chunkinsft_trainer.py, the inner loop of the defaultloss_type="chunked_nll"doestrl/trl/trainer/sft_trainer.py
Line 101 in 6d484ba
Both operands are already the model dtype (bf16), so upcasting them buys no information; it just moves the GEMM off the tensor cores onto the fp32 SIMT path and materialises an fp32 copy of the entire
lm_headweight. For a 248,320-token vocabulary that copy is 2.03 GB, rebuilt for every chunk and again on every gradient-checkpoint recompute.is what
loss_type="nll"already gets (the model applieslm_headin bf16 andForCausalLMLossupcasts afterwards), and what the docstring forchunked_nllalready claims ("same math asnll").The same pattern is in the distillation trainers
DistillationTrainer._chunkpays it twice per chunk: once for the student and once for the teacher. And the experimental async distillation trainer has a third copy. All four are fixed here.trl/trainer/utils.py:1446looks similar and is deliberately not touched:there
grad_logitsgenuinely is fp32, so that upcast is important.Only the SFT path is benchmarked below; the distillation change is mechanically identical and numerically free for the same reason, but I have not measured its speedup. Its test suite is green:
Numbers
One chunk, 256 tokens × vocab 248,320 × hidden 2048, 1×H100, bf16, fwd+bwd):
h.float() @ w.float().t())In an 8×H100 profile of
trl sfton Qwen3.6-35B-A3B (FSDP2, seq 4096, per-device batch 4, LoRA), the two fp32 SIMT GEMM kernels (sm80_xmma_gemm_f32f32_f32f32_f32_tn_n_...ffma,cutlass_80_simt_sgemm_...) account for 985 ms of a 4.57 s step (21.6% of all GPU kernel time) in 192 launches averaging 5.1 ms each.End to end, on four released models, 16384 tokens per step,
loss_type="chunked_nll"throughout:tokens/s/GPU, at 16k tokens per step. The win grows with
hidden_size, because that is what sets the fp32 GEMM's cost relative to the rest of the step. Which is why thehidden=2048projection, gains least.Numerically it changes nothing
This is not a speed-for-accuracy trade. One step at
learning_rate=0.0onQwen3-0.6B, gradients captured beforezero_gradand compared in fp32:That is expected once stated:
handware already bf16, so upcasting them is lossless, and a bf16 tensor-core GEMM accumulates in fp32 regardless.Both paths compute the same products at the same accumulation precision, the fp32 version just does it on
the SIMT path and materialises an fp32 copy of the
lm_headweight to get there.Why this matters for the
ccePR#6859
When measured against this "fixed" baseline instead of today's default,
loss_type="cce"adds only 1.06× on Qwen3-8B (both full finetune and LoRA), against the 1.80×/1.66× it shows versus the unfixed path.Nearly all of that apparent gain is this bug!
ccestill earns its place on models whose vocabulary is large relative to their size (1.76× on gemma-3-270m over the fixed baseline)Note
Low Risk
Localized numeric-path change in loss helpers; PR reports identical loss and zero gradient drift when operands are already bf16, with comments documenting the FSDP2 dtype edge case.
Overview
Chunked
lm_headprojections in SFT (loss_type="chunked_nll"), DistillationTrainer, and async distillation no longer upcast hidden states and weights to fp32 before the matmul. They now compute(hidden @ weight.to(hidden.dtype).T)and only then cast logits to fp32 for softmax/NLL or JSD—aligned with"nll",ForCausalLMLoss, and the intended “same math as nll” behavior.This keeps the large vocabulary GEMM on tensor cores (bf16/fp16) instead of fp32 SIMT, avoiding repeated fp32 copies of the full
lm_headweight each chunk (and on gradient-checkpoint recomputation). Weights are explicitly cast to the hidden-state dtype so FSDP2 mixed precision still matches whatlm_head.forwardwould do when master weights are fp32.Reviewed by Cursor Bugbot for commit f99ca55. Bugbot is set up for automated code reviews on this repo. Configure here.