fix: cache a copy of the GPT dataset loss mask - #6981
Open
ZhiyuLi-Nvidia wants to merge 1 commit into
Open
Conversation
GPTDataset.__getitem__ caches the loss mask computed for the first sample it serves, then applies that sample's padding to it in place. Because the cache stores the tensor itself rather than a copy, the in-place masking mutates the cached mask, and every subsequent sample clones an already-padded base. A dataset instance whose first served sample is padded therefore returns wrong masks from then on. With the mock dataset's validation split and drop_last_partial_validation_sequence=False, serving the padded trailing sequence first takes sample 0's loss mask from 1 masked position to 771. Cache a clone, mirroring the clone already made on the cache-hit path. Signed-off-by: Zhiyu Li <zhiyul@nvidia.com>
ZhiyuLi-Nvidia
force-pushed
the
zhiyul/gpt-dataset-loss-mask-cache
branch
from
August 30, 2026 10:06
bba70c6 to
3217bc0
Compare
ZhiyuLi-Nvidia
marked this pull request as ready for review
August 30, 2026 11:02
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?
The bug
GPTDataset.__getitem__caches the loss mask, then masks in place:Line 303 stores the tensor itself rather than a copy, so line 312 writes through that reference
into the cache. Only the first sample served can do this — FILL runs once per dataset
(
masks_and_position_ids_are_cachedis setTruethere and never reset) and every later samplemasks a clone.
Step 3 does not reuse "the cache from step 2" — step 2 never wrote one. Line 308
always clones the slot, which has held
Asince step 1.Impact
The cached mask is masked in place, so the first sample's padding poisons every batch after
it. Line 303 stores the tensor itself, then line 312 writes into that same object — so whatever
the first sample masks (real padding, or the pad id among its tokens) is written straight into the
cache and silently inherited by every following batch. Later samples are harmless by comparison:
they clone the cache first, so their own padding lands on a throwaway copy.
Two ways a sample gets masked positions:
sequence_length. Training never hits this(
drop_last_partial_sequenceis hardcodedTrueat line 248), but validation can, withdrop_last_partial_validation_sequence=False._pad_token_idistokenizer.padverbatim(
megatron_dataset.py:75), so any occurrence in the token stream is masked, on any sample.MockGPTLowLevelDatasetgenerates(arange(length - 1) + 1) % vocab_size— a ramp over the wholevocabulary — so under
--mock-datathis fires on essentially every sample.It breaks bitwise checkpoint resume, silently. Which sample runs FILL is a property of the
process: a fresh run fills from its first training sample, a resumed run from whatever sample its
step maps to. The two then serve different masks for the same sample, so gradients diverge from the
first step after the resume even with checkpoint state,
tokens,labelsandposition_idsallbitwise identical. Nothing errors — the loss stays plausible because it is normalised by the mask sum.
MockGPTDatasetandGPTFIMDatasetinherit this__getitem__.SFTDatasetdefines its own and isunaffected.
The fix
Cache a clone at 303, mirroring the clone already made at 308. One line — the first
sample's in-place write then lands on a throwaway and the slot stays clean.
Test
test_mask_cache_does_not_leak_paddingbuilds two dataset instances, serves themasked sample first in one, and asserts both return the same
loss_maskforindex 0. It first asserts that sample really is masked, so it fails loudly rather
than passing vacuously if the fixture stops producing one. It reproduces the defect
via trigger 1, which is the one constructible from an in-tree tokenizer; both
triggers exercise the same aliasing.
Fails before the change (
AssertionError: padding from the first served sample leaked into a later sample's loss_mask), passes after.cached_attention_maskandcached_position_idsare stored by reference too.Neither is mutated today so neither is buggy; left unchanged to keep the diff
minimal.
Issue tracking
For PRs from open-source community contributors:
Linked issue:
Contribution process
Pre-checks
Code review
Feel free to message or comment @NVIDIA/mcore-oncall to help accelerate your merge into main. The less complex your PR is, the faster it will be approved and merged!
All PRs start as draft. If you open a non-draft PR, it will be automatically converted to draft.
Step 1: Mark PR as "Ready for Review"
.github/CODEOWNERS.Final Review might get declined if these requirements are not fulfilled.
Step 2: Final Review
For PRs that change
megatron/core, once all expert reviewers have approved, theFinal Reviewlabel is applied automatically and final reviewers are assigned.For PRs outside
megatron/core, this step is skipped.Step 3: Approved
Once all required reviewers have approved, the
Approvedlabel is applied automatically.Merge
Any member of mcore-engineers will be able to merge your PR.