Skip to content

Fix broken remove_sharded_tensors public API and re-enable its unit test - #5759

Merged
dimapihtar merged 2 commits into
NVIDIA:mainfrom
huthvincent:fix/remove-sharded-tensors
Jul 19, 2026
Merged

Fix broken remove_sharded_tensors public API and re-enable its unit test#5759
dimapihtar merged 2 commits into
NVIDIA:mainfrom
huthvincent:fix/remove-sharded-tensors

Conversation

@huthvincent

Copy link
Copy Markdown
Contributor
  • I, the PR author, have personally reviewed every line of this PR.

What does this PR do?

Repairs megatron.core.dist_checkpointing.remove_sharded_tensors, which is currently broken in three independent ways, and re-enables the unit test that would have caught this.

  1. serialization.py: the wrapper called the method unbound on TorchDistSaveShardedStrategy, but it is defined only on TorchDistLoadShardedStrategy → every call raised AttributeError. Now instantiates the load strategy, restoring the pre-2beb593b4 behavior (sharded_strategy.remove_sharded_tensors(...) on an instance).
  2. strategies/torch.py: the method read fs_writer.metadata_path, which no longer exists on FileSystemWriter in current PyTorch. It now uses the metadata_filename computed two lines above (identical path, works across torch versions).
  3. strategies/torch.py: fs_writer.fs.rm_file(checkpoint_dir / f) raised TypeError for the documented str argument type; now Path(checkpoint_dir) / f.

Test changes (tests/unit_tests/dist_checkpointing/test_serialization.py::test_remove_sharded_tensors):

  • Removed the @pytest.mark.flaky / @pytest.mark.flaky_in_dev markers. With both markers the test runs in no CI environment, which is precisely how the API could break unnoticed (timeline in the linked issue).
  • Updated the post-removal metadata assertion to include common_state/shard_0_1: commit 40b1fd3 ("deprecate common strategy") updated the pre-removal assertion for the new common-state entry but could not update the post-removal one, since the test no longer reached it.

Issue tracking

Linked issue: Fixes #5758

Contribution process

Pre-checks

  • I have added relevant unit tests (re-enabled and corrected the existing one)
  • I have added relevant functional tests (not applicable)
  • I have added proper typing to my code (no signature changes)
  • I have added relevant documentation (behavior unchanged; restores documented behavior)
  • I have run the autoformatter on the changed files (black 24.10, isort)

Local verification (the CI unit test needs 8 GPUs; verified end-to-end at world_size=1 on a single GPU with the same save/remove/inspect sequence as the unit test, passing str paths to exercise fix 3):

files before: ['.metadata', '__0_0.distcp', 'metadata.json', 'prefix__0_1.distcp']
files after: ['.metadata', '__0_0.distcp', 'metadata.json']
metadata keys after: ['common_state/shard_0_1', 'keyA']
PASS: remove_sharded_tensors works end-to-end

On unpatched main the same script fails with AttributeError: type object 'TorchDistSaveShardedStrategy' has no attribute 'remove_sharded_tensors'; with only fix 1 applied it fails with AttributeError: 'FileSystemWriter' object has no attribute 'metadata_path'.

@copy-pr-bot

copy-pr-bot Bot commented Jul 12, 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.

@huthvincent
huthvincent marked this pull request as ready for review July 12, 2026 17:58
@huthvincent
huthvincent requested review from a team as code owners July 12, 2026 17:58
@svcnvidia-nemo-ci
svcnvidia-nemo-ci requested a review from a team July 12, 2026 17:58
@huthvincent
huthvincent marked this pull request as draft July 12, 2026 17:59
@cspades

cspades commented Jul 13, 2026

Copy link
Copy Markdown
Member

/ok to test 5218f35

Every call to megatron.core.dist_checkpointing.remove_sharded_tensors
raised AttributeError: since commit 2beb593 the wrapper invokes the
method unbound on TorchDistSaveShardedStrategy, while it is defined only
on TorchDistLoadShardedStrategy. Restore the previous behavior by calling
it on a load-strategy instance.

Repair two further breakages inside the method itself: replace reads of
FileSystemWriter.metadata_path, which no longer exists in current
PyTorch, with the equivalent metadata_filename computed in the same
function, and convert the str checkpoint_dir to Path before joining file
names so removal does not raise TypeError.

Re-enable the covering unit test by dropping its flaky markers (with both
markers it ran in no CI environment, which is how the API broke
unnoticed) and update its post-removal metadata assertion for the
common_state entry introduced by commit 40b1fd3, which updated only
the pre-removal assertion.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Rui Zhu <rui.zhu.rz399@yale.edu>
@huthvincent
huthvincent force-pushed the fix/remove-sharded-tensors branch from 5218f35 to bb92537 Compare July 13, 2026 22:41
@huthvincent

Copy link
Copy Markdown
Contributor Author

@cspades Same situation as #5756. After your /ok to test, I pushed a small follow-up commit (bb92537c5) that bumps the NVIDIA copyright headers on the changed files to the current year so the copyright-check job passes. That moved the PR head, so the earlier approval is tied to the previous commit and CI did not start on the new one. Could you re-run /ok to test bb92537c5 when you get a chance? Thanks a lot, and sorry for the extra round.

@cspades

cspades commented Jul 13, 2026

Copy link
Copy Markdown
Member

/ok to test bb92537

@svcnvidia-nemo-ci svcnvidia-nemo-ci added the waiting-on-maintainers Waiting on maintainers to respond label Jul 16, 2026
@svcnvidia-nemo-ci svcnvidia-nemo-ci added the Final Review PR is in the "final review" stage label Jul 16, 2026
@dimapihtar

Copy link
Copy Markdown
Contributor

/ok to test c656b65

@svcnvidia-nemo-ci svcnvidia-nemo-ci added Approved All necessary approvals have been made and removed Final Review PR is in the "final review" stage labels Jul 17, 2026
@dimapihtar
dimapihtar enabled auto-merge July 17, 2026 16:56
@dimapihtar
dimapihtar added this pull request to the merge queue Jul 19, 2026
@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

🔄 Merge queue validation started!

You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/29677835075

Merged via the queue into NVIDIA:main with commit 3219f38 Jul 19, 2026
86 of 88 checks passed
chochowski pushed a commit to chochowski/Megatron-LM that referenced this pull request Jul 20, 2026
…est (NVIDIA#5759)

Signed-off-by: Rui Zhu <rui.zhu.rz399@yale.edu>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Dmytro Pykhtar <37850217+dimapihtar@users.noreply.github.com>
Signed-off-by: mchochowski <mchochowski@nvidia.com>
terminator123 pushed a commit to 021ai/Megatron-LM that referenced this pull request Aug 3, 2026
…est (NVIDIA#5759)

Signed-off-by: Rui Zhu <rui.zhu.rz399@yale.edu>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Dmytro Pykhtar <37850217+dimapihtar@users.noreply.github.com>
svcnvidia-nemo-ci pushed a commit to dimapihtar/Megatron-LM that referenced this pull request Aug 4, 2026
…est (NVIDIA#5759)

Signed-off-by: Rui Zhu <rui.zhu.rz399@yale.edu>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Dmytro Pykhtar <37850217+dimapihtar@users.noreply.github.com>
Signed-off-by: Dmytro Pykhtar <dpykhtar@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Approved All necessary approvals have been made community-request complexity: low

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Public API dist_checkpointing.remove_sharded_tensors is broken: any call raises AttributeError

7 participants