Skip to content

Create Megatron-FSDP v1 TP groups collectively and validate pg_collection - #8051

Closed
Connor-XY wants to merge 3 commits into
NVIDIA:mainfrom
Connor-XY:yx/pstate-f3-fsdp-v1-dist-index
Closed

Connor-XY wants to merge 3 commits into
NVIDIA:mainfrom
Connor-XY:yx/pstate-f3-fsdp-v1-dist-index

Conversation

@Connor-XY

@Connor-XY Connor-XY commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Why. FullyShardedDataParallelV1 fills an unset tp or expt_tp in pg_collection with
dist.new_group(ranks=[rank]), a different rank list on every rank. Group creation must be identical on
all ranks: this works only while every rank takes the same branch, and otherwise later groups mismatch or
hang. Dense models also create an expert TP group they never use, and a collection that lacks a needed
group fails with an AttributeError on None deep in mesh construction.

What.

  • Create single-rank TP groups once, with new_subgroups_by_enumeration over every rank, and only for
    the axes the model uses.
  • Check the groups the configuration needs (dp_cp; intra_dp_cp, inter_dist_opt for HSDP; ep,
    expt_dp, intra_expt_dp for MoE) and raise ValueError naming the missing ones.
  • Read collection fields directly. Comment the global branch as the migration fallback; its groups are
    unchanged.

The collection path keeps the replicate data-parallel fields. With GTP weight remat off they hold the
same ranks as the fallback's GTP-inclusive groups.

Tests. New test_mcore_fsdp_dist_index.py (4 GPUs): a HyperCommGrid collection laid out unlike the global grid,
with every parallel_state accessor patched to raise; identical group creation on every rank;
ValueError for missing groups; explicit-vs-fallback rank parity on eight standard-grid layouts. Before the
2026-11-01 sync, 10 of the 20 cases failed on main, and the existing MFSDP v1 tests matched main on 4 and 2 GPUs.
In the integration run, this PR's test file shows no regressions against main: all 20 cases of
test_mcore_fsdp_dist_index.py pass on 4 GPUs, and main does not have the file.

Compatibility. No change for the standard grid or for collections that set the needed groups. A collection missing one
now raises ValueError instead of AttributeError.

The branch was synced with main 77391e7 on 2026-11-01 by a merge commit (no conflicts), so existing commits

Later on 2026-11-01 the branch also merged main bfab98f. Its #8471 moves FullyShardedDataParallelV1 and FullyShardedDataParallelV2 from mcore_fsdp_adapter.py into mcore_adapter_v1.py and mcore_adapter_v2.py, and keeps mcore_fsdp_adapter.py as the factory that re-exports them. This PR's edits to those classes are applied unchanged where the code moved, and the process-group allowlist follows the move. On 4 GPUs against its new base, the Megatron-FSDP adapter tests and this PR's own tests (7 files) show no regressions: 78 passed here, 58 on the base. The two CUDA-graph end-to-end cases of test_mcore_fully_sharded_data_parallel.py fail on both sides, as they do on main.
and review links stay valid.

Part of #6307.

🤖 Generated with Claude Code

FullyShardedDataParallelV1._init_dist_index filled a missing tp or
expt_tp field of pg_collection with dist.new_group(ranks=[rank]). Each
rank passed a different rank list, which breaks the rule that every
rank creates the same groups in the same order. This works only while
each group has one member and every rank takes the same branch. If
ranks ever disagree, their group counters drift apart and later group
creation mismatches or hangs. Dense models also created an expert TP
group they never use.

- Create the single-rank groups once, with new_subgroups_by_enumeration
  over every rank, and only for the axes the model uses.
- Check the fields the configuration needs (dp_cp; intra_dp_cp and
  inter_dist_opt with several optimizer instances; ep, expt_dp and
  intra_expt_dp for MoE) and raise ValueError naming the missing ones,
  instead of an AttributeError on None deep in mesh construction.
- Read collection fields directly instead of getattr with a default.
- Comment the global-grid branch as the migration fallback for callers
  without a collection. Its accessors and groups are unchanged.

The collection path keeps the replicate data-parallel fields (dp_cp,
expt_dp, intra_dp_cp, intra_expt_dp). With GTP weight remat off they
hold the same ranks as the GTP-inclusive groups that the fallback's
accessors return. A new parity test checks this on eight standard-grid
layouts, and a forbid-globals test runs the adapter on a HyperCommGrid
collection laid out unlike the global grid.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
@Connor-XY

Copy link
Copy Markdown
Contributor Author

/ok to test 528b9f6

@copy-pr-bot

copy-pr-bot Bot commented Oct 10, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

Connor-XY added a commit to Connor-XY/Megatron-LM that referenced this pull request Nov 1, 2026


NVIDIA#8051 creates the Megatron-FSDP v1 TP groups collectively and validates
the pg_collection it is given. The Megatron-FSDP v1 fallback warning
changes the same adapter code and the dist-index tests that NVIDIA#8051
creates, so merge NVIDIA#8051's head. The merge is clean.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
Connor-XY added a commit to Connor-XY/Megatron-LM that referenced this pull request Nov 1, 2026
The checker classification that follows requires every allowlisted
read in megatron/core to be warned or guarded. NVIDIA#8749 warns the global
fallbacks of Megatron-FSDP v1, Torch FSDP and the FSDP-DTensor expert
offsets, and brings NVIDIA#8017, NVIDIA#8045 and NVIDIA#8051. The guard tests that follow
edit the mfsdp_v1 test_mcore_fsdp_dist_index.py that NVIDIA#8749 extends.

The merge conflicts only in the process-group usage allowlist, which is
regenerated on the merged tree.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
Connor-XY and others added 2 commits October 31, 2026 23:00
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
Main's NVIDIA#8471 moves FullyShardedDataParallelV1 and FullyShardedDataParallelV2 out of
mcore_fsdp_adapter.py into mcore_adapter_v1.py and mcore_adapter_v2.py, and keeps
mcore_fsdp_adapter.py as the factory that re-exports them. This branch's edits to the
two classes and their helpers are applied unchanged where the code moved, and the
process-group allowlist follows the move.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
@Connor-XY

Copy link
Copy Markdown
Contributor Author

Superseded by #8894, which carries this change as commit c2dbd1d under the same title, with "Originally #8051." in its message. Closing so review happens in one place. The branch stays on the fork.

@Connor-XY Connor-XY closed this Nov 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant