fix(loss): return zero, not NaN, when a batch has no supervised tokens - #3797
Open
kabirvashisht4-glitch wants to merge 1 commit into
Conversation
MaskedCrossEntropy guards `num_label_tokens == 0` and returns `loss * 0.0`, but FusedLinearCrossEntropy, ChunkedCrossEntropy and TEParallelCrossEntropy divided unconditionally. `num_label_tokens` is the global DP-reduced count of non-ignored labels, so it is 0 when every label in the batch is -100 -- a gradient-accumulation window where truncation cut the answer off every sample, for instance. The sum-reduced loss is then 0.0 and `0.0 / 0` is NaN, which propagates through backward() into every parameter: the run continues with a destroyed model rather than failing. Which loss was configured therefore decided whether the step survived, and `_maybe_downgrade_loss_fn` can swap a fused loss for MaskedCrossEntropy on its own, so one config could behave differently across two models. The recipe already treats zero as real on the validation path (`max(total_num_label_tokens, 1e-8)`). Apply the same guard to the three, and cover all four with CPU tests that stub the optional cut_cross_entropy / Transformer Engine kernels. Signed-off-by: kabirvashisht4-glitch <kabirvashisht4@gmail.com>
3 tasks
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 a batch with no supervised tokens contribute
0.0instead ofNaNin thethree losses that were missing the guard
MaskedCrossEntropyalready has.num_label_tokensis the global, DP-reduced count of non-ignored labels(
recipes/llm/train_ft.pyL1210-1213). It is0when every label in the globalbatch is
-100— for example a gradient-accumulation window where truncationcut the answer off every sample, or a bad shard. The sum-reduced loss is then
0.0, and0.0 / 0isNaN.NaNpropagates throughbackward()into everyparameter, so the run keeps going with a destroyed model rather than failing.
masked_ce.pyL84-88 already handles it:The other three divided unconditionally:
MaskedCrossEntropy0.00.0(unchanged)FusedLinearCrossEntropyNaN0.0ChunkedCrossEntropyNaN0.0TEParallelCrossEntropyNaN0.0So which loss you configured decided whether the step survived. That is
reachable without the user changing anything:
_maybe_downgrade_loss_fn(
train_ft.pyL159) swaps a fused loss forMaskedCrossEntropywhen the modeldoes not declare
logits_to_keep, so one config could behave differently acrosstwo models.
The recipe already treats zero as a real case on the validation path
(
train_ft.pyL1382,max(total_num_label_tokens, 1e-8)), so the training pathwas the inconsistent one.
loss * 0.0rather than a freshtorch.zeroskeeps the autograd graph intact,so the step contributes zero gradient instead of detaching.
Changelog
FusedLinearCrossEntropy,ChunkedCrossEntropyandTEParallelCrossEntropyreturn
loss * 0.0whennum_label_tokens == 0, matchingMaskedCrossEntropy.tests/unit_tests/loss/test_zero_label_tokens.pycovering all four lossesplus a normalization regression guard.
Before your PR is "Ready for review"
Pre checks:
Behaviour now matches the documented
num_label_tokenscontract, so no docchange was needed.
Verification (CPU, no GPU needed):
linear_ce/te_parallel_cetests stubcut_cross_entropyand theTransformer Engine kernel — following the pattern already in
test_linear_ce.py— so all four guards are actually exercised here ratherthan skipped, even though neither optional package is installed locally.
test_masked_ce_zero_label_tokens_is_zeroandtest_nonzero_label_tokens_still_normalizespass either way, which isintended — they pin the reference behaviour and the path this must not change.
pytest tests/unit_tests/loss/→ 299 passed, 36 skipped, no regressions.ruff format/ruff checkclean.Additional Information
I kept this as the minimal guard so it is easy to review. If you would rather
the normalization were factored into one shared helper so the four cannot drift
apart again, say so and I will restructure it that way.