Skip to content

[ckpt] fix: support vocab padding with Megatron-FSDP TP - #5353

Merged
yaoyu-33 merged 2 commits into
mainfrom
yuya/fix-mfsdp-tp-vocab-padding
Aug 6, 2026
Merged

[ckpt] fix: support vocab padding with Megatron-FSDP TP#5353
yaoyu-33 merged 2 commits into
mainfrom
yuya/fix-mfsdp-tp-vocab-padding

Conversation

@yaoyu-33

@yaoyu-33 yaoyu-33 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • compute the padded global vocabulary size correctly when the Megatron target is an MFSDP DTensor
  • add TP=2 coverage to the existing MFSDP Hugging Face round-trip functional test

Root cause

DTensor.shape already describes the global tensor across TP ranks. The vocabulary-padding path multiplied that dimension by tp_size again, so a TP=2 import padded a 151,936-token embedding to 303,872 rows and scattered 151,936 rows to a local 75,968-row buffer.

This fixes the regression introduced by #1473 and tracked internally as NVBug 6565261.

Validation

  • Before the fix, the Qwen3-0.6B MFSDP TP=2 round trip reproduced expected (75968, 1024), got (151936, 1024).
  • After the fix, the same real-checkpoint round trip completed successfully and wrote all 226 tensors.
  • uv run --no-sync python -m pytest tests/functional_tests/test_groups/converter/test_hf_fsdp_conversion.py -k FSDP_TP2 -v -s — 1 passed
  • uv run pre-commit run --all-files — passed

Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented Aug 5, 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.

@yaoyu-33 yaoyu-33 added the r0.6.0 Auto-cherrypick to release branch. Apply before merge; cherrypick happens after merge. label Aug 5, 2026
@yaoyu-33
yaoyu-33 marked this pull request as ready for review August 5, 2026 21:54
@claude

claude Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

LGTM.

The fix is correct. expect_dim0_size now accounts for DTensor.shape being the global shape across TP ranks in the Megatron-FSDP path, while a regular Megatron parameter stores only its local TP shard and still needs multiplication by tp_size. This matches the existing DTensor output_shape handling that uses orig_param.shape below it, and the global-size chunk along dim 0 produces per-rank shards matching each local shape. The new FSDP_TP2 parametrization gives the TP>1 path real coverage now that the Megatron-LM dependency is available.

No logic concerns, no typos, and no perf configs are touched.

Suggested test cases:

  • test_hf_fsdp_roundtrip 1-1-1-FSDP_base
  • test_hf_fsdp_roundtrip 2-1-2-FSDP_TP2

No perf tests impacted.

@yaoyu-33 yaoyu-33 added area:ckpt Checkpoint conversion, loading, export, and save paths bug Something isn't working full-test-suite needs-more-tests Requires additional L0 and L1 test coverage before merge needs-review PR is ready for code review and waiting on a reviewer labels Aug 5, 2026
@yaoyu-33
yaoyu-33 merged commit 250710b into main Aug 6, 2026
10 checks passed
@yaoyu-33
yaoyu-33 deleted the yuya/fix-mfsdp-tp-vocab-padding branch August 6, 2026 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:ckpt Checkpoint conversion, loading, export, and save paths bug Something isn't working full-test-suite needs-more-tests Requires additional L0 and L1 test coverage before merge needs-review PR is ready for code review and waiting on a reviewer r0.6.0 Auto-cherrypick to release branch. Apply before merge; cherrypick happens after merge.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant