Skip to content

Reject unsafe sharded cyclic dataloader resumes - #7648

Open
Silas-Zeng wants to merge 2 commits into
NVIDIA:mainfrom
Silas-Zeng:fix/cyclic-sharded-dp-resume
Open

Silas-Zeng wants to merge 2 commits into
NVIDIA:mainfrom
Silas-Zeng:fix/cyclic-sharded-dp-resume

Conversation

@Silas-Zeng

@Silas-Zeng Silas-Zeng commented Sep 24, 2026 •

Copy link
Copy Markdown

Problem

With cyclic dataloading and data sharding enabled, resuming after a data-parallel size change can silently repeat and skip samples because the sampler's buckets and resume offset depend on the effective DP size.

Fix

Persist explicit cyclic dataloader metadata in checkpoint arguments alongside the consumed sample count: the effective dataloader DP size and the sharding mode. On a normal resume, validate that metadata before adopting the checkpoint's training progress and fail clearly when the sharded layout cannot be preserved. Older checkpoints fall back to their existing DP arguments for compatibility; finetune and release loads keep their existing behavior.

Validation

  • python -m py_compile megatron/training/checkpointing.py tests/unit_tests/test_checkpointing.py
  • git diff --check
  • Parameterized tests cover mismatch rejection, matching layouts, missing legacy metadata, sharding-mode changes, non-target modes, finetune/release, and equivalent effective DP sizes.
  • pytest -q tests/unit_tests/test_checkpointing.py — 47 passed on NVIDIA RTX 5090 32GB, CUDA 12.8, PyTorch 2.8.0+cu128, Triton 3.4.0, Python 3.12.3.

Fixes #7645

@copy-pr-bot

copy-pr-bot Bot commented Sep 24, 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.

@Silas-Zeng
Silas-Zeng marked this pull request as ready for review September 24, 2026 18:29
@Silas-Zeng
Silas-Zeng requested a review from a team as a code owner September 24, 2026 18:29
@svcnvidia-nemo-ci
svcnvidia-nemo-ci requested a review from a team September 24, 2026 18:29
@Silas-Zeng
Silas-Zeng marked this pull request as draft September 24, 2026 19:13
@Silas-Zeng
Silas-Zeng marked this pull request as ready for review September 24, 2026 19:28
@Silas-Zeng
Silas-Zeng marked this pull request as draft September 25, 2026 05:07
@Silas-Zeng
Silas-Zeng marked this pull request as ready for review September 25, 2026 06:45
@svcnvidia-nemo-ci svcnvidia-nemo-ci added the waiting-on-maintainers Waiting on maintainers to respond label Sep 26, 2026
@Silas-Zeng
Silas-Zeng requested review from a team as code owners September 28, 2026 17:24
Silas-Zeng added a commit to Silas-Zeng/Megatron-LM that referenced this pull request Sep 28, 2026
Refresh the PR branch onto the current mainline and preserve the cyclic
dataloader resume validation, metadata compatibility, and regression tests.

Signed-off-by: HR-zengzzz <hrzeng123456@gmail.com>
Silas-Zeng added a commit to Silas-Zeng/Megatron-LM that referenced this pull request Sep 28, 2026
Refresh the PR tree to the current mainline and preserve the cyclic
dataloader resume validation, metadata compatibility, and regression tests.

Signed-off-by: HR-zengzzz <hrzeng123456@gmail.com>
@Silas-Zeng
Silas-Zeng force-pushed the fix/cyclic-sharded-dp-resume branch from a1a582a to 6688a16 Compare September 28, 2026 17:28
@Silas-Zeng
Silas-Zeng requested review from a team as code owners September 28, 2026 17:28
@Silas-Zeng
Silas-Zeng force-pushed the fix/cyclic-sharded-dp-resume branch from 6688a16 to a1a582a Compare September 28, 2026 17:30
@guihong-nv

Copy link
Copy Markdown
Contributor

@Silas-Zeng The branch includes ~9 unrelated upstream commits (biggest structural issue). Of the 14 commits in this PR, only 5 are from you. The rest are merged mainline commits: MXFP8 via MFSDP adapter, HybridStack residuals, MIMO fixes, DCP V2, SSM fusion tests, GTP teardown fix, Docker parallelization. The total diff is +3,537/−279 lines, but the actual fix is almost certainly <100 lines touching checkpointing.py and test_checkpointing.py. This inflates the review surface dramatically and makes it hard to audit what the PR actually changes. The branch should be rebased onto current main with only the fix commits, or at minimum the maintainers should review only the your commits directly.

Signed-off-by: svcnvidia-nemo-ci <svcnvidia-nemo-ci@nvidia.com>
@svcnvidia-nemo-ci
svcnvidia-nemo-ci force-pushed the fix/cyclic-sharded-dp-resume branch from 9808899 to 7483a59 Compare October 1, 2026 11:41
@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

🛠️ Claude fix commit 7483a59d3bd6 (attempt 2)

⚠️ This explanation is AI-generated and may be inaccurate; the exact commit is authoritative.

What changed
Reformatted two SimpleNamespace(...) call sites in pr-head/tests/unit_tests/test_checkpointing.py to match black's expected output: collapsed the multi-line argument lists in test_validate_cyclic_dataloader_resume_uses_effective_dp_size (checkpoint_args) and test_validate_cyclic_dataloader_resume_rejects_sharding_mode_change (args) onto single lines, dropping trailing commas. Whitespace only; no logic change, no new/deleted/renamed files, nothing committed.

Files changed by Claude

  • tests/unit_tests/test_checkpointing.py

Why
Only the linting job failed: black --check --diff said "would reformat tests/unit_tests/test_checkpointing.py", exit 1. With skip_magic_trailing_comma=true, black joins those arg lists onto one line as both fit in line_length=100. I applied black's two printed hunks verbatim; my line deltas match the log hunk headers. black and pytest are unavailable (no network), so I verified the AST is identical pre/post, confirming a cosmetic edit. No unit failures in the log.

DCO
@Silas-Zeng, please fix any DCO failures on your commits before merge. DCO does not block this workflow.

View exact commit

Sanitized and posted by svcnvidia-nemo-ci.

@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

/ok to test 7483a59

@svcnvidia-nemo-ci svcnvidia-nemo-ci added waiting-on-customer Waiting on the original author to respond and removed waiting-on-maintainers Waiting on maintainers to respond labels Oct 1, 2026
@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

❌ Claude fix stopped because exact-SHA CI did not complete in time. View exact-SHA CI.

@Silas-Zeng

Copy link
Copy Markdown
Author

Thanks for the fixes. The Claude workflow timed out waiting for CI on 7483a59. Run 36857019494 is still waiting at cicd-wait-in-queue, with a pending deployment for the test environment; lint has passed. @dimapihtar, could you check what is needed for the CI run to proceed?

@svcnvidia-nemo-ci svcnvidia-nemo-ci added waiting-on-maintainers Waiting on maintainers to respond and removed waiting-on-customer Waiting on the original author to respond labels Oct 1, 2026
@Silas-Zeng

Copy link
Copy Markdown
Author

Hi @dimapihtar @guihong-nv @wujingyue, thanks for enabling CI for these fixes. The CICD runs for #7648 and #7649 have now passed.

Could you help route these two PRs to the appropriate code owner for review? They address repeated or skipped samples when resuming cyclic dataloaders after a DP-size change. I'm happy to provide any additional validation needed.

Thanks for your time!

@svcnvidia-nemo-ci svcnvidia-nemo-ci removed the waiting-on-maintainers Waiting on maintainers to respond label Oct 5, 2026
@wujingyue
wujingyue removed request for a team October 5, 2026 20:10
@svcnvidia-nemo-ci svcnvidia-nemo-ci added the waiting-on-maintainers Waiting on maintainers to respond label Oct 7, 2026

This branch was previously deployed

2 inactive deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-request Expert Review [deprecated] Apply this label to indicate that your PR is ready for expert review. waiting-on-maintainers Waiting on maintainers to respond

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cyclic dataloader with data sharding repeats and skips samples when resumed at a different data-parallel size

4 participants