fix(loss): make ChunkedCrossEntropy reductions independent of chunk_len - #3804
Open
kabirvashisht4-glitch wants to merge 1 commit into
Conversation
The fallback loop accumulated per-chunk results with `+=`. That is valid for reduction="sum" -- which takes the separate _ChunkedCrossEntropySum kernel anyway -- but wrong for the two reductions that actually use the loop: - "mean" summed per-chunk means, returning num_chunks x the loss. At the default chunk_len=32 over a 4096-token sequence that is 128x, and the gradients scale with it, behaving like a silently multiplied learning rate. - "none" element-wise added the per-chunk vectors instead of concatenating, returning chunk_len values rather than one per token. Neither raised. Both are hidden when chunk_len >= seq_len, since a single chunk makes accumulation a no-op -- so the error only appears once the sequence is long enough to chunk, which is what this loss is for. Accumulate the sum for "mean" and divide once by the non-ignored token count, and concatenate for "none", so both match F.cross_entropy exactly. Tests cover all three reductions across chunk_len values that do and do not divide the sequence evenly, with and without ignored positions. Signed-off-by: kabirvashisht4-glitch <kabirvashisht4@gmail.com>
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.
What does this PR do ?
Makes
ChunkedCrossEntropyreturn the same loss regardless ofchunk_len.chunk_lenis a memory knob; it was changing the answer.The fallback loop accumulated per-chunk results with
+=(
components/loss/chunked_ce.pyL199-216). That is valid forreduction="sum"— which takes the separate_ChunkedCrossEntropySumkernelanyway — but wrong for the two reductions that actually reach the loop:
"mean"summed per-chunk means, returningnum_chunks xthe loss."none"element-wise added the per-chunk vectors instead ofconcatenating, returning
chunk_lenvalues instead of one per token.Neither raised. Measured against
F.cross_entropyon 4096 tokens:chunk_len"mean"before"mean"after"none"returned(32,)instead of(4096,)at the defaultchunk_len.Note the last row: with
chunk_len >= seq_lenthere is exactly one chunk, soaccumulation is a no-op and the result is correct. The error only appears once
the sequence is long enough to chunk — which is the case this loss exists for —
so a short smoke test passes.
Because the gradients scale with the loss, a config using
reduction: meantrained with an effectively multiplied learning rate.
Changelog
"mean"accumulates the per-chunk sum and divides once by thenon-ignored token count, which matches
F.cross_entropy(..., reduction="mean")exactly rather than approximately.
"none"concatenates the per-chunk vectors."sum"is untouched — it never used this loop.TestReductionIsChunkInvariantintests/unit_tests/loss/test_chunked_ce.pycomparing all three reductions against
F.cross_entropyacrosschunk_lenin{1, 32, 96, 100, 128, 512}— including 96 and 100, which donot divide the 512-token sequence evenly — with and without ignored positions
scattered across chunk boundaries.
Before your PR is "Ready for review"
Pre checks:
The class docstring already describes the fallback loop; its behaviour now
matches the description, so no doc change was needed.
Verification (CPU, no GPU needed):
"mean"and"none"case with more than one chunk); the"sum"cases and thesingle-chunk cases pass either way, which is intended — they pin the paths
this must not change.
pytest tests/unit_tests/loss/→ 318 passed, 36 skipped, no regressions.ruff format/ruff checkclean.Scope note.
"mean"over a batch where every label is ignored divides byzero and yields
NaN, exactly asF.cross_entropydoes — I kept that identicalrather than changing it here, since this PR is only about chunk-invariance. That
separate question is #3796 / #3797.
Additional Information