fix(loss): stop masked losses from overwriting the caller's labels - #3918
Open
kabirvashisht4-glitch wants to merge 1 commit into
Open
kabirvashisht4-glitch wants to merge 1 commit into
kabirvashisht4-glitch wants to merge 1 commit into
Conversation
Contributor
|
Hi @kabirvashisht4-glitch can you take a look at the conflicts when you get the chance? thank you |
MaskedCrossEntropy, ChunkedCrossEntropy and TEParallelCrossEntropy applied `mask` with an in-place `labels.masked_fill_`. `labels.view(-1)` aliases the caller's storage, and in the TE path `labels.to_local()` aliases a DTensor's local shard, so the fill wrote `ignore_index` straight into the caller's batch rather than into a private copy. Reusing that tensor for a second loss term or a second mask then scores the intersection of the masks instead of the new one. With two disjoint masks the second call returns exactly 0.0 -- a silent zero gradient, no error and no warning. Use the out-of-place `masked_fill` in all three and document that labels are left intact. The existing mask test cloned `targets` after the loss call, so it re-applied an already-applied mask and could never observe the write; snapshot it beforehand instead. Signed-off-by: kabirvashisht4-glitch <kabirvashisht4@gmail.com>
kabirvashisht4-glitch
force-pushed
the
kabirvashisht4-glitch/fix/loss-label-mutation
branch
from
September 17, 2026 05:24
70bb3f9 to
2faa368
Compare
This branch has not been deployed
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 ?
Fixes #3917:
MaskedCrossEntropy,ChunkedCrossEntropyandTEParallelCrossEntropyapplied their optional
maskwith an in-placelabels.masked_fill_, writingignore_indexinto the caller's tensor.labels.view(-1)aliases the caller's storage rather than copying it, so the fill isvisible to whoever owns the batch.
TEParallelCrossEntropyis worse:labelshasalready been through
labels.to_local(), and that local shard aliases the DTensor'sstorage, so the write lands in the caller's distributed tensor.
The failure is silent and it zeroes a loss. Reusing the tensor for a second mask scores
the intersection of the two masks, not the second one:
mask_a= first halfmask_b= second half (same tensor)A loss of exactly
0.0backprops a zero gradient with no exception and no warning. Thesame applies to any second consumer of the batch: another loss term, a token-accuracy or
perplexity metric computed after the loss, or a cached dataset that returns the same
underlying tensor on the next epoch.
calculate_loss— the wrapper most callers go through — documents the oppositeguarantee: "The caller's mapping and tensors are not mutated."
Scope
No in-tree recipe passes
mask=today (calculate_lossforwards onlylogits,labelsand
num_label_tokens), so this is a latent defect on a public API rather than an activetraining-corruption bug. It is reachable by anyone calling these modules directly, which
is what the
maskparameter is for.Why the existing test could not catch it
test_masked_cross_entropy_with_maskbuilt its reference after the loss call:It re-applied an already-applied mask, so it passed with or without the bug. This PR
snapshots
targetsbeforehand, which makes the existing assertion able to fail.Changelog
labels.masked_fill_with the out-of-placemasked_fillinmasked_ce.py,chunked_ce.pyandte_parallel_ce.py.labelsis not modified in place.chunked_ce.pypreviously documented the opposite ("ignored positions are replaced with
ignore_indexin this tensor"); that line is now correct.targetsbefore the loss call intest_masked_cross_entropy_with_maskso itcan observe an in-place write.
MaskedCrossEntropyandChunkedCrossEntropy: a test that the caller'slabelsis unchanged, and a test that two disjoint masks over one tensor each scoretheir own positions. Add the corresponding no-mutation test for
TEParallelCrossEntropy, guarded onHAVE_TE_PARALLEL_CEand CUDA.Before your PR is "Ready for review"
Pre checks:
Verified locally:
pytest tests/unit_tests/loss/-> 313 passed, 37 skipped.mainand pass here. Reverting only thenemo_automodel/changes and keeping the tests reproduces:second mask scored 0.000000, expected 6.012943.ruff format --checkandruff checkclean on every changed file.Additional Information
labelsundernemo_automodel/components/loss/.