Skip to content

fix: reset dataloader index when dataset changes on resume#2519

Merged
yuki-97 merged 3 commits into
NVIDIA-NeMo:mainfrom
jinglinglingling:fix/dataset-swap-skip-load
May 21, 2026
Merged

fix: reset dataloader index when dataset changes on resume#2519
yuki-97 merged 3 commits into
NVIDIA-NeMo:mainfrom
jinglinglingling:fix/dataset-swap-skip-load

Conversation

@jinglinglingling

Copy link
Copy Markdown
Contributor

What does this PR do ?

When resuming a single-dataloader run with a different data.train.dataset_name, StatefulDataLoader.load_state_dict() silently skips the first N samples of the new dataset (where N = samples_yielded from the previous run), or hard-crashes with StopIteration during iter() if len(new_dataset) <= N.

This PR tags each train_dataloader.pt save with the active dataset_name and skips the state restore when it differs from the current config, printing a warning so the swap is visible. Legacy saves (raw state dict) are still loaded as before for backward compatibility.

Applied to GRPO (sync + async), SFT, DPO, RM, Distillation. The multi-dataloader save path is intentionally left untouched in this PR.

Issues

N/A

Usage

No new API. Behavior is automatic:

# step 1: train on dataset A and checkpoint
uv run examples/run_grpo_math.py --config ... data.train.dataset_name=OpenMathInstruct-2

# step 2: resume from the checkpoint with a *different* dataset
uv run examples/run_grpo_math.py --config ... data.train.dataset_name=gsm8k checkpointing.checkpoint_must_exist=true

Previously, step 2 would silently skip the first N samples of gsm8k (or crash if N >= len(gsm8k)). Now the new dataset starts from index 0 and a warning is printed.

Before your PR is "Ready for review"

Pre checks:

  • Make sure you read and followed Contributor guidelines
  • Wrote standalone empirical + identity-logic tests locally (not upstreamed)
  • Ran existing unit tests locally:
    • tests/unit/algorithms/test_sft.py: 5 passed
    • tests/unit/algorithms/test_dpo.py: 4 passed
    • tests/unit/algorithms/test_rm.py: 3 passed
    • tests/unit/algorithms/test_distillation.py: 10 passed
    • tests/unit/algorithms/test_grpo.py: running at PR time, relying on CI
  • No new documentation required

Additional Information

  • Backward compatible: old checkpoints with the legacy raw-state format still load via an isinstance fallback.
  • The save payload is now {"state": <state>, "dataset_name": <name>}.
  • Multi-dataloader save path is unchanged in this PR; per-dataset state semantics can be added in a follow-up.

@jinglinglingling
jinglinglingling requested review from a team as code owners May 18, 2026 09:37
@copy-pr-bot

copy-pr-bot Bot commented May 18, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@jinglinglingling

Copy link
Copy Markdown
Contributor Author

/ok to test 410b0f0

@jinglinglingling
jinglinglingling force-pushed the fix/dataset-swap-skip-load branch from 410b0f0 to 9627592 Compare May 18, 2026 09:54
@jinglinglingling

Copy link
Copy Markdown
Contributor Author

/ok to test 9627592

@jinglinglingling
jinglinglingling force-pushed the fix/dataset-swap-skip-load branch from 9627592 to a6fe6ba Compare May 18, 2026 10:04
@jinglinglingling jinglinglingling added the CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version) label May 18, 2026
@jinglinglingling

Copy link
Copy Markdown
Contributor Author

/ok to test a6fe6ba

@jinglinglingling

Copy link
Copy Markdown
Contributor Author

/ok to test e68e7bc

@jinglinglingling

Copy link
Copy Markdown
Contributor Author

/ok to test 6401a47

@yuki-97 yuki-97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for helping fix! some comments.

Comment thread nemo_rl/algorithms/distillation.py Outdated
Comment thread nemo_rl/data/utils.py Outdated
@jinglinglingling
jinglinglingling force-pushed the fix/dataset-swap-skip-load branch from f748553 to 3f26781 Compare May 20, 2026 09:49
When resuming a single-dataloader run with a different
data.train.dataset_name, StatefulDataLoader.load_state_dict() silently
skips the first N samples of the new dataset (N = samples_yielded from
the previous run), or hard-crashes with StopIteration during iter() if
len(new_dataset) <= N.

Detect this in load_dataloader_state by reading the previous run's
dataset_name out of the sibling config.yaml that the checkpointer
already writes alongside train_dataloader.pt. When it differs from the
current config, skip the state restore and print a warning so the swap
is visible. The on-disk format of train_dataloader.pt is unchanged, so
every existing checkpoint is automatically compatible.

Applied to GRPO (sync + async), SFT, DPO, RM, Distillation. The
multi-dataloader save path is intentionally left untouched in this PR.

Signed-off-by: Linglin Jing <linglinj@nvidia.com>
@jinglinglingling
jinglinglingling force-pushed the fix/dataset-swap-skip-load branch from 3f26781 to eaa5a40 Compare May 20, 2026 09:50
@jinglinglingling

Copy link
Copy Markdown
Contributor Author

/ok to test eaa5a40

@yuki-97 yuki-97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @jinglinglingling . change to run L1 to make sure resume works well.

@yuki-97 yuki-97 added CI:L1 Run doctests, unit tests, and functional tests and removed CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version) labels May 21, 2026
@yuki-97

yuki-97 commented May 21, 2026

Copy link
Copy Markdown
Contributor

/ok to test fc0138c

@yuki-97
yuki-97 enabled auto-merge (squash) May 21, 2026 05:23
@yuki-97
yuki-97 merged commit 6afb047 into NVIDIA-NeMo:main May 21, 2026
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI:L1 Run doctests, unit tests, and functional tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants