Skip to content

Reject unset process-group fields instead of reading them as None - #8756

Closed
Connor-XY wants to merge 99 commits into
NVIDIA:mainfrom
Connor-XY:yx/pstate-c10-presence-checks
Closed

Connor-XY wants to merge 99 commits into
NVIDIA:mainfrom
Connor-XY:yx/pstate-c10-presence-checks

Conversation

@Connor-XY

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

Copy link
Copy Markdown
Contributor

Depends on #6303 (which contains #6293), #8017 (which contains #6259), #8021 and #8049. Please review the diff from their merge: scoped diff.

Land this after #8748, which passes the collection in the same TEFusedMLP.as_mlp_submodule call where this PR passes the checked tp_group. Whichever lands second keeps both arguments. Land it after #8038 too: TEDotProductAttention drops its cp assert and relies on #8038's check that CP attention has a CP group.

Why. ProcessGroupCollection returns None for a declared field that was never set. So hasattr(pgc, name) is always true, and getattr(pgc, name, None) cannot tell an unset field from an axis that is off. Core code used both as presence checks, so a collection missing a field silently led to:

  • an MLP sharded over the global TP group, with only a fallback warning;
  • TEDotProductAttention getting no TP group;
  • DSA attention running with TP and CP off;
  • refit describing TP shards as whole tensors, and a None group reaching torch.distributed as WORLD;
  • HybridModel choosing its logging ranks as if an unset group were an axis that is off;
  • MIMO checkpoint checks that vanish under python -O.

What.

  • TEDotProductAttention, MLP.as_mlp_submodule, TEFusedMLP.as_mlp_submodule, DSAttention, HybridModel's logging groups and refit's extract_param_metadata check the fields they read with require_process_groups. An unset field raises a ValueError that names it. None still means the axis is off.
  • DSAttention checks tp and cp once at construction instead of reading them with getattr in every forward. The cuDNN full-fusion hook still treats an omitted collection as TP off.
  • HybridModel's tp and dp_cp must still both be groups or both be None.
  • Refit also rejects a None dp, and a missing ep for expert-parallel parameters.
  • MIMO replica ids and sharded state dicts raise ValueError instead of asserting.
  • Optional reads use vars(), with no behavior change: dp_cp_gtp_remat in the GPT and Hybrid MTP metrics, tp_cp in MTP, and gtp_remat, expt_gtp_remat and DDP's expt_dp in the DDP and optimizer setup.
  • The allowlist does not change. vars() and require_process_groups are not counted reads.

Tests.

  • New tests/unit_tests/test_process_group_field_presence.py:
    • DSAttention rejects an unset tp or cp and accepts None;
    • the MLP builders reject a collection without tp;
    • on an even number of ranks, TEDotProductAttention and the MLP builder use a separate communicator over the same TP ranks with global reads forbidden, and an omitted collection still warns.
  • New tests/unit_tests/models/mimo/test_mimo_checkpoint_groups.py covers the replica ids and sharded state dicts.
  • test_model_metadata.py adds six refit metadata tests.
  • The HybridModel pairing test in test_hybrid_model.py builds real collections and expects unset fields to raise. The stand-in model in main's config_list_model_boundary fixture now sets tp and dp_cp to None next to pp, since HybridModel requires both.
  • Three DSAttention tests in test_attention_variant_dsa.py built the module from an empty namespace and now declare both axes off.

On 4 GPUs against its base, with no regressions:

  • test_process_group_field_presence.py passes all 9 new tests, including the 5 that need Transformer Engine;
  • test_mimo_checkpoint_groups.py passes all 17 new tests;
  • test_model_metadata.py has 50 passed here vs 41 on the base, and the six new tests run as 9 cases that all pass;
  • test_hybrid_model.py has 84 passed and 4 skipped on both sides;
  • test_attention_variant_dsa.py has 112 passed on both sides. The three TestIndexerTensorParallel tests fail on both sides because they need 8 ranks.

In the integration run, this PR's test files show no regressions against main. There the DSA file passes all 115 tests on 8 GPUs (two nodes), as on main, including the three TestIndexerTensorParallel tests. With all the #6307 changes applied, end-to-end training smokes on 4 GPUs give bit-identical losses to main and record no process-group fallback warning.

Before the sync with main, the branch ran on 4 CPU gloo ranks, one test file at a time, with no regressions:

  • test_model_metadata.py passes all 50 tests;
  • the new MIMO file passes all 17 tests;
  • the new field-presence file passes 4 tests and skips 5 that need Transformer Engine;
  • test_hybrid_model.py needs Transformer Engine to collect. Run on its own without it, the pairing test passes.

Compatibility.

  • Callers that pass a collection must set the fields above, to None where the axis may be off. In-repo callers already do, and so do Megatron-Bridge's MIMO collections. Other callers get a new ValueError from:
    • TEDotProductAttention and the MLP builders, for an unset tp. On main the MLP took the global TP group with a warning, and TE got tp_group=None;
    • DSAttention, for an unset tp or cp;
    • refit, for an unset tp, pp or dp, a None dp, or a missing ep on an expert-parallel parameter;
    • MIMO checkpointing, whose messages also change.
  • A duck-typed collection that keeps its groups as class attributes now reads None at the vars() sites. No in-repo caller passes one there.
  • The new comments in process_groups_config.py say that an unset gtp_remat or expt_gtp_remat means the axis is off for DDP and optimizer setup. resolve_gtp_remat_group instead falls back to the global GTP group for an unset field. That mismatch predates this PR; Warn before resolve_gtp_remat_group reads the global grid #8769 makes the fallback warn.
  • extract_param_metadata runs require_process_groups for every tensor, though once per component would do. The new Raises sections leave out the TypeError that require_process_groups raises for a MultiModuleProcessGroupCollection.
  • Needs determinism-exempt: the kernel coverage check flags the Transformer Engine wrappers and the DSA files. Only process-group checks change.

Synced with main 77391e7 on 2026-11-01 by a merge commit that lists each conflict and its resolution, so existing commits and review links stay valid.

Part of #6307.

🤖 Generated with Claude Code

Connor-XY and others added 30 commits October 7, 2026 16:02
Carry the optional dense and expert optimizer Gloo groups in explicit
collections and resolve the legacy global collection at the public optimizer
factory boundary. Preserve the disabled-Gloo path and permit collection
construction when Gloo groups were not created.

Refresh against current main, preserve the nested optimizer regressions,
and cover optional accessor checks plus Gloo presence and enablement.

Signed-off-by: Yan Xu <yxu1@nvidia.com>
…o groups

With use_gloo_process_groups=True, a process-group collection without Gloo
groups used to fall back silently to running the distributed optimizer without
Gloo. The global path raised in that case before this change. Raise ValueError
instead, except under GTP_remat, which never builds Gloo groups.

Signed-off-by: Yan Xu <yxu1@nvidia.com>
get_megatron_optimizer resolves the collection before calling its helpers, so a
missing collection there is a caller error. Report it with ValueError instead of
an assert, which is skipped under python -O. The previous hasattr(tp) check
could never fail, because ProcessGroupCollection returns None for unset fields,
so it is dropped.

Signed-off-by: Yan Xu <yxu1@nvidia.com>
get_megatron_optimizer now resolves a missing pg_collection with
ProcessGroupCollection.use_mpu_process_groups() up front, so the default path
goes through the explicit-collection branch of
setup_process_groups_for_optimizer. That branch returned no intra_dist_opt
group unless ddp_config.use_distributed_optimizer was set. Megatron-FSDP
leaves that flag False while OptimizerConfig enables the distributed
optimizer, so its gradient-statistics group became None
(TestMegatronFsdpGradStatsGroup in mfsdp_v1). Pass intra_dist_opt through
from the collection whenever it is present, as the parallel_state branch
always did.

The initializer-cleanup factory tests patch every optimizer helper and never
initialize model parallelism; stub the MPU fallback there instead of
reaching the global grid.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
With Muon and the distributed optimizer, the emerging-optimizer path builds
a separate DistributedOptimizer for the parameters Muon does not manage, and
it set up that optimizer's process groups with Gloo hard-coded off. The
collection now carries the Gloo groups, so honor the caller's
use_gloo_process_groups instead. Without a Gloo group, the first
memory-efficient fully reshardable checkpoint save failed the
data_parallel_group assert in get_parameter_state_dp_zero.

Tests: the layer-wise optimizer's DistributedOptimizer gets the collection's
Gloo group (and none when Gloo is off), and the Muon + Lion checkpoint
round trip also covers the memory-efficient fully reshardable format.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
Without the distributed optimizer, the emerging-optimizer path reduced the
gradient norm and zero count of expert parameters over tp_ep_pp, which
leaves out the EGTP_remat axis. EGTP_remat peers hold distinct shards of
each expert weight, so each peer computed the norm of its own shards only,
and with gradient clipping the peers applied different clip factors and
their replicas drifted apart. Use tp_ep_pp_with_egtp_remat, as the standard
optimizer path does, falling back to tp_ep_pp for collections without it
(the same group when EGTP_remat is off).

Test: Muon with EP=2 and EGTP_remat=2 reports the norm over every expert
shard on every rank. Before this change the two EGTP_remat peers reported
53.1 and 73.3 on 4 ranks, against a true norm of 89.1.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
The distributed optimizer shards its state over the collection's
data-parallel group (dp_cp, or intra_dp_cp with more than one optimizer
instance) and gathers that state over the matching Gloo group for checkpoint
I/O. Taking the Gloo groups from the collection means they can now disagree:
for example, a caller who starts from use_mpu_process_groups() and replaces
dp_cp with their own group keeps parallel_state's Gloo group, and the save
would then gather over other ranks, failing on a size mismatch or silently
scrambling the optimizer checkpoint. Compare the rank lists of each Gloo
group and the group it mirrors (dense and expert) and raise ValueError when
they differ.

Only expert parameters use the expert Gloo group, so require
intra_expt_dp_gloo only when the collection has an expert data-parallel
group. A dense model on its own grid can then enable Gloo by supplying
intra_dp_cp_gloo alone.

Tests: groups built by parallel_state pass with one and two optimizer
instances; a Gloo group over other ranks raises, dense and expert; replacing
dp_cp in an MPU collection with a same-size group over other ranks raises; a
dense collection with only intra_dp_cp_gloo is accepted.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
The Gloo-collection test built its collection with use_mpu_process_groups(),
so its Gloo groups were the parallel_state globals and the identity checks
also passed for code that read the globals directly. Swap freshly created
Gloo groups into the collection instead, and add an expert parameter so the
expert distributed optimizer's Gloo group is checked too. The case where Gloo
is requested in a job built without Gloo groups is now its own test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
The CI ratchet from NVIDIA#6258 landed on main after this branch was written, so
its allowlist still lists the two optimizer-helper fallbacks this change
removes and lacks the one boundary read it consolidates them into. Refresh
it: 210 -> 209 grandfathered reads.

The addition was reviewed by hand: get_megatron_optimizer resolves a
missing pg_collection with use_mpu_process_groups() once, at the entry
point, replacing the fallbacks in _get_megatron_emerging_optimizer and
_get_megatron_optimizer_based_on_param_groups that were already
grandfathered. After adding it, `python tools/check_process_group_usage.py
--update` dropped the two stale entries.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
A collection without an embd field, or tied pipeline embedding/output stages
without a usable embedding group, is a caller error. Report it with
ValueError instead of an assert, which is skipped under python -O.

Signed-off-by: Yan Xu <yxu1@nvidia.com>
Six sites in megatron/core called parallel_state accessors even though the
correct group was already stored on the object (self.tp_group / self.cp_group).
Pure substitution: no signature changes, no new arguments, no caller impact.

  SelfAttention.run_realtime_tests       3 sites -> self.tp_group
  SelfAttention.get_query_key_value_tensors  1 site -> self.tp_group
  MultiTokenPredictionLayer              1 site -> self.tp_group
  LLaVAModel                             1 site -> self.cp_group

Global process-group reads in megatron/core: 163 -> 157.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
…scope

Two more sites where the correct group was reachable without any API change.

transformer_engine.py: in the cp_comm_type == 'a2a+p2p' branch the code read the
global hierarchical CP groups, even though the caller-supplied branch directly
above asserts hasattr(pg_collection, 'hcp') for exactly that case. The assert
was already guaranteeing the group the code then declined to use.

language_module.py: the word-embedding replica id derived its DP-CP rank from
the global grid while the sharded tensor nine lines later is built against
metadata['dp_cp_group']. Now both use the same group, so replica id and
sharding agree under a caller-supplied grid.

Global process-group reads in megatron/core: 157 -> 155.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
megatron/core/models/multimodal/context_parallel.py hard-coded the global CP
grid in 11 places, across module-level helpers with no group parameter. This is
the vision-encoder path, so it is exactly where an independent encoder grid
silently used the LLM's groups instead.

cp_group is now a required argument on the public helpers and is threaded
through the private ones and the autograd Function. get_pg_size/get_pg_rank
preserve the previous behaviour when CP is disabled (size 1, rank 0).

The call graph is closed inside this repo: LLaVAModel already holds
self.cp_group, and the only other callers are unit tests. The module is not
re-exported and has no users in Megatron-Bridge or nemo-rl.

Global process-group reads in megatron/core: 155 -> 144.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
_gather_from_tensor_parallel_region read the global TP grid for its size, rank
and both collectives. Both callers are subclasses that already hold the group
(self.tp_group on ColumnParallelLinear, self._tp_group on the TE variant), and
the mapping helpers it calls already accept an explicit group.

Global process-group reads in megatron/core: 144 -> 141.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
… in tests

Caught by a distributed run: changing the autograd Function signature
missed one direct .apply() call site in the unit tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
hybrid_context_parallel_forward_backward read the global TP and DP-CP grids for
its broadcast source rank, broadcast group, rank checks and barrier. Its only
caller is forward_backward_no_pipelining, which already holds pg_collection.

dp_cp is asserted in the hybrid-CP branch rather than unconditionally, matching
how TEDotProductAttention asserts hcp only for cp_comm_type == 'a2a+p2p'; the
legacy schedule collection already requests dp_cp.

get_tensor_model_parallel_src_rank() becomes
torch.distributed.get_global_rank(tp_group, 0), which is its definition.

hybrid_cp_schedule.py no longer imports parallel_state at all.

Global process-group reads in megatron/core: 141 -> 136.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
run_realtime_tests compared Q/K layernorm parameters across the global DP grid.
The DP group is required only by this debug check, so it is asserted there
rather than in __init__, which asks use_mpu_process_groups for tp/cp only --
the same idiom TEDotProductAttention uses for hcp.

megatron/core/transformer/attention.py no longer references parallel_state.

Global process-group reads in megatron/core: 136 -> 133.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
mtp_on_this_rank read the global PP rank to decide whether MTP layers live on
this stage. Both model callers (GPTModel, HybridModel) already hold self.pp_group
from LanguageModule.__init__, which runs before the call.

Global process-group reads in megatron/core: 125 -> 124.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
RotaryEmbedding and MultimodalRotaryEmbedding fell back to the global CP grid
via a ternary when cp_group was not passed. 10 of the 22 constructor calls in
this repo already passed one; the remaining 12 (3 in core, 9 in tests) now do
too, so the fallback is gone rather than merely unused.

megatron/core/models/common/embeddings/rotary_pos_embedding.py no longer
references parallel_state.

Global process-group reads in megatron/core: 124 -> 122.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
…nd T5Model

Both classes fell back to ProcessGroupCollection.use_mpu_process_groups() when
no collection was passed. The fallback is now an assertion naming the missing
argument, which is the 'fails loudly rather than silently' half of the goal.

DotProductAttention: already dead in-repo -- Attention always passes
pg_collection when building core_attention, and the one direct test
construction passes it positionally.

T5Model: 4 of 5 constructors did not pass one (3 tests, 1 example); they now do.

This is a deliberate breaking change for external callers that construct these
directly without a collection, which is the intent of the migration.

Global process-group reads in megatron/core: 122 -> 120 (shims 20 -> 18).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
…lock

The use_mpu_process_groups fallback becomes an assertion naming the missing
argument. All 7 constructors without one live in a single test module and now
pass a collection.

Global process-group reads in megatron/core: 120 -> 119 (shims 18 -> 17).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
LLaVAModel is the case the migration exists for: a vision encoder and an LLM may
run on independent parallel grids, so falling back to the global grid silently
picks the wrong one. The fallback becomes an assertion that says exactly that.

All 8 constructors without a collection (7 tests, 1 example) now pass one.

Global process-group reads in megatron/core: 119 -> 118 (shims 17 -> 16).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
Attention, MultiLatentAttention (x2) and AbsorbedMLASelfAttention each fell back
to use_mpu_process_groups when no collection was passed. All are dead in-repo:
TransformerLayer sets attention_optional_kwargs['pg_collection'] unconditionally
before build_module, and every direct constructor already passes one (verified
by AST across megatron/, tests/ and examples/).

Global process-group reads in megatron/core: 118 -> 114 (shims 16 -> 12).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
Global process-group reads in megatron/core: 114 -> 113 (shims 12 -> 11).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
These were based on a faulty inventory. My constructor scan filtered on
len(node.args) < 2, which silently skipped every call site passing arguments
positionally, so 'no caller is missing pg_collection' was wrong.

A paired run showed 85 real failures:
  63x AssertionError: MultiLatentAttention requires an explicit pg_collection
  22x AssertionError: Attention requires an explicit pg_collection
  10x TypeError: DSAIndexer.__init__() got multiple values for 'pg_collection'
      (my inserted keyword duplicated an existing positional argument)

Reverting to the last validated state. The earlier fallback removals
(DotProductAttention, T5Model, MultiTokenPredictionBlock, LLaVAModel) were each
validated against a pristine baseline and are retained.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
Redo of the reverted attempt, this time with a signature-aware inventory: both
DSAIndexer constructors already pass pg_collection positionally, so the fallback
is dead and the tests need no change. The previous attempt added a redundant
keyword and produced 'got multiple values for argument pg_collection'.

Global process-group reads in megatron/core: 118 -> 117 (shims 16 -> 15).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
Second attempt, this time driven by the signature-aware scanner: 15 of 16
SelfAttention constructors were missing pg_collection (all in test modules) and
now pass one. The earlier attempt used a scan that skipped positional call
sites and had to be reverted.

Global process-group reads in megatron/core: 117 -> 116 (shims 15 -> 14).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
…sses

Second attempt, driven by the signature-aware scanner. MultiLatentAttention (x2)
and AbsorbedMLASelfAttention no longer fall back to use_mpu_process_groups.
18 of 19 MLASelfAttention constructors were missing pg_collection, all in one
test module, and now pass one; AbsorbedMLASelfAttention's single constructor
already did.

Global process-group reads in megatron/core: 116 -> 113 (shims 14 -> 11).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
Paired run: 24 failed / 43 passed against a pristine 20 / 57, with 110
'requires an explicit pg_collection' assertions. The failures are concentrated
in TestFusedMLA*, i.e. a MultiLatentAttention subclass my scan never covered --
I scanned MLASelfAttention and AbsorbedMLASelfAttention by name and assumed
those were the only constructors reaching the base __init__.

The Attention removal in the preceding commit validated clean (5F/121P on both
trees) and is retained.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
NVIDIA#6259 added the ProcessGroupCollection fields intra_dp_cp_gloo and
intra_expt_dp_gloo. NVIDIA#8017's build_test_pg_collection() predates them, so
with both PRs merged its collection lacks the two fields and
test_build_test_pg_collection_matches_initialize_model_parallel fails
its field-set check (8 of 8 with 4 gloo ranks on CPU).

The helper builds the NCCL groups model components use and no Gloo
groups. Set both fields to None, as in a job built without Gloo groups
(the documented meaning of None for these fields), and list them with
the other fields the helper does not build.

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

copy-pr-bot Bot commented Oct 30, 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 and others added 9 commits October 30, 2026 13:34
NVIDIA#6303 makes LanguageModule require an explicit process-group
collection, on top of NVIDIA#6293's deprecated global fallbacks, and imports
ProcessGroupCollection at module level in the hybrid model tests. The
HybridModel logging-group pairing test is to build
ProcessGroupCollection objects at module level, so merge NVIDIA#6303 here.
The merge is clean.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
NVIDIA#8017 adds test helpers that forbid global grid reads and build
process-group collections; its head also carries NVIDIA#6259's Gloo fields of
the collection. The process-group field presence tests use the
helpers, so merge NVIDIA#8017 here. The merge is clean.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
NVIDIA#8021 makes refit source collections carry dp instead of filling it
in, and its metadata tests import ProcessGroupCollection. The refit
metadata change requires the tp, pp and dp groups of that collection
and tests it with the same import, so merge NVIDIA#8021 here.
The merge is clean.

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


NVIDIA#8049 adds ProcessGroupFallbackWarning, resolve_process_groups and
require_process_groups, on top of NVIDIA#6293's deprecated global fallbacks.
The attention, MLP, MIMO, refit and HybridModel presence checks call
require_process_groups, so merge NVIDIA#8049 here. The merge is clean.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
The allowlist merged without conflict, but the merged file keeps a
total of 172 while the merged tree has 170 reads. Regenerate it from
the merged tree.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
ProcessGroupCollection returns None for a declared field that was never
set, so hasattr(pg_collection, name) is always true. TEDotProductAttention,
MLP.as_mlp_submodule and TEFusedMLP.as_mlp_submodule asserted the
presence of tp (and cp) with hasattr, which never fired:
- an MLP built from a collection without tp received tp_group=None and
  fell back to the global tensor-parallel group, reported only by a
  ProcessGroupFallbackWarning;
- TEDotProductAttention passed tp_group=None to Transformer Engine.
DSAttention read tp and cp with getattr(..., None) in its forward pass,
so a collection without them silently ran the attention with tensor
and context parallelism off.

The three builders now check tp with require_process_groups, so an
unset field raises ValueError. A tp of None is still accepted: TE then
waits for set_tensor_parallel_group(), and the MLP keeps its existing
TP-group default. TEDotProductAttention reads cp only with context
parallelism, where an unset or None cp group already raises, so the
cp assert is dropped rather than made stricter. DSAttention checks tp
and cp once at construction, where None still means that the axis is
off, and its forward pass reads the collection's fields directly. The
cuDNN split top-k hook, which requires a collection, reads its tp
directly as well. The cuDNN full-fusion hook keeps its default of no
collection, which still means no tensor parallelism.

Three DSAttention unit tests built the module from an empty namespace
and now declare both axes off.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
MimoOptimizer's replica ids and the sharded state dicts of
ModalitySubmodules and MimoModel checked their process groups with
"assert hasattr(pg_collection, name) and pg_collection.name is not
None". The hasattr half is always true for a ProcessGroupCollection,
and the assert disappears under python -O, so a module collection
without dp_cp, tp or pp would then elect checkpoint writers from None
groups.

The replica ids take tp, pp and dp_cp through require_process_groups
and also reject None, since each of them supplies a rank. The sharded
state dicts read dp_cp with vars() and reject an unset or None group.
All of them raise ValueError. gtp_remat and dp_cp_gtp_remat stay
optional and are read with vars(): without gtp_remat the module has no
GTP axis, and without dp_cp_gtp_remat the writer is elected over
dp_cp, as before.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
extract_param_metadata describes each tensor of a model for the
reshard planner with the rank lists of the model's groups. It read tp
and pp behind hasattr checks, which are always true for a
ProcessGroupCollection, and expt_tp with getattr(..., None). A
collection without tp therefore described every tensor-parallel shard
as a whole tensor, since the planner skips the TP descriptor when the
rank list is missing, and a collection without pp got every rank of
the job as its pipeline group. dp and ep went to
torch.distributed.get_process_group_ranks unchecked, where None stands
for the default group.

tp, pp and dp now go through require_process_groups, so an unset field
raises ValueError. tp and pp of None keep meaning that the axis is off,
as before. dp, and ep for an expert-parallel parameter, must be process
groups. A collection without an expert tensor-parallel group still
describes expert weights with tp; that read now uses vars().

The in-repo refit paths already pass collections that set these
fields: model collections (LanguageModule requires tp and pp), the
inference shard collections, and MIMO components, whose tp, pp and dp
refit_modules checks.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
HybridModel picks the ranks that log each pipeline stage's layers from
pg_collection.tp and pg_collection.dp_cp, read with getattr(..., None),
so a field the collection never set looked like an axis that is off.
Both fields now go through require_process_groups: an unset one raises
a ValueError that names it, and tp and dp_cp must still both be groups
or both be None. LanguageModule already requires tp to be set, so in
practice a missing dp_cp is now reported by name rather than as a
mismatched pair. Both None still drops the groups and leaves the log
to the global grid; HybridModel cannot get there with
torch.distributed initialized, because LanguageModule then rejects a
None tp. The existing pairing test now builds ProcessGroupCollection
objects and expects unset fields to raise.

The other getattr(pg_collection, name, None) reads in these files are
optional fields, where an unset field and None mean the same thing.
They now read vars() and say so:
- GPTModel and HybridModel average the MTP metrics over
  dp_cp_gtp_remat, or over dp_cp when the collection does not set it;
- MultiTokenPredictionBlock keeps tp_cp only for a sequence-parallel
  CP layout conversion, which raises without it;
- the DDP and optimizer process-group setup treat an unset or None
  gtp_remat, expt_gtp_remat or (for DDP) expt_dp as that axis being
  off.
None of these reads changes behavior.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
@Connor-XY
Connor-XY force-pushed the yx/pstate-c10-presence-checks branch from 39f2801 to 47750fe Compare November 1, 2026 01:38
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#8756 rejects unset tp
and cp fields in the attention and MLP builders, validates the MIMO
checkpoint groups and requires the refit metadata groups, instead of
probing the collection and falling back to the global grid.

megatron/core/extensions/transformer_engine.py conflicts on the
process_groups_config import: NVIDIA#8755 imports amax_reduction_group and
resolve_process_groups, and NVIDIA#8756 imports require_process_groups. The
import takes all three. The process-group usage allowlist 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 added a commit to Connor-XY/Megatron-LM that referenced this pull request Nov 1, 2026
The core changes that follow warn the last two reads that NVIDIA#8770's
checker classifies as unwarned (MoEMetricsTracker._sync_metrics and
mtp_on_this_rank), empty its UNWARNED_READS list, and extend NVIDIA#8770's
checker test and its test_moe_process_groups.py. NVIDIA#8770 brings the
fallback-warning series under it, NVIDIA#8767 and the deprecations below it.

Four files conflict. Each takes both sides:
- extensions/transformer_engine.py: the process_groups_config import
  adds NVIDIA#8755's resolve_process_groups (eac0b1c), and
  TEFusedMLP.as_mlp_submodule passes both NVIDIA#8756's tp_group from
  require_process_groups (c33f4ec) and NVIDIA#8748's
  pg_collection=pg_collection (8d0a49b).
- models/T5/t5_model.py: drop the self.tp_group line that NVIDIA#6293 kept;
  NVIDIA#6303's b732c67, which NVIDIA#8770 brings, removes it because
  LanguageModule already sets self.tp_group from the collection.
- multi_latent_attention.py and absorbed_mla.py, the q_proj linears:
  pass both NVIDIA#8023's tp_group=pg_collection.tp (e681f41, which NVIDIA#8770
  brings) and NVIDIA#8748's pg_collection=pg_collection (2a712de).
The process-group usage allowlist is regenerated on the merged tree with
NVIDIA#8770's checker (check_process_group_usage.py --update), which counts
the warned and unwarned reads inside the total; it equals NVIDIA#8770's.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
Connor-XY and others added 8 commits November 1, 2026 09:00
The process-group allowlist records the reads main added since this PR
was written.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
Conflicts:
- examples/t5/pretrain_t5.py: Main adds `cfg = get_run_config()` to
  model_provider and drops the `build_tokenizer` import in favour of
  `get_tokenizer()`. The PR moves that import for isort and passes a
  ProcessGroupCollection into T5Model. The result keeps main's `cfg`
  line and the PR's pg_collection fallback and argument. It drops the
  `build_tokenizer` import, which would otherwise be unused.
- megatron/core/models/common/embeddings/rotary_pos_embedding.py: Main
  imports `register_capture_constant` for its shared rotary cache. The
  PR imports `warn_global_process_group_fallback`. Both imports are
  kept, along with main's cache refactor and the PR's warning
  cp_group fallback.
- pretrain_gpt.py, pretrain_hybrid.py: Main (NVIDIA#8341, NVIDIA#8592) moves
  `is_dataset_built_on_rank` into megatron/training/datasets/providers.py
  and the dataset config builder into argument_utils. The PR adds
  `pp_group` to the `mtp_on_this_rank` call in the local predicate. The
  result takes main's layout and keeps the PR's `pp_group` in get_batch.

Adapted to main outside the conflicts:
- megatron/training/datasets/providers.py: The PR's predicate edit is
  re-applied in main's shared `is_dataset_built_on_rank_explicit`. It
  passes `pp_group=mpu.get_pipeline_model_parallel_group()` to
  `mtp_on_this_rank`, so middle pipeline stages do not hit the
  fallback warning. The function's signature is unchanged.

The process-group allowlist records the reads main added since this PR
was written.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
The process-group allowlist records the reads main added since this PR
was written.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
The process-group allowlist records the reads main added since this PR
was written.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
The process-group allowlist records the reads main added since this PR
was written.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
Conflicts:
- examples/bert/pretrain_bert.py: The PR adds a ProcessGroupCollection
  import for the pg_collection default in model_provider, and main's
  tokenizer refactor (NVIDIA#8391) swaps build_tokenizer for get_tokenizer.
  The result keeps the PR's import list, imports get_tokenizer from
  global_vars, and drops the unused build_tokenizer import.
- tests/unit_tests/pipeline_parallel/test_fine_grained_activation_offloading.py:
  The PR imports ProcessGroupCollection for its pg_collection arguments,
  and main splits the tensor_parallel.random import to add
  CheckpointWithoutOutput. The result keeps both imports.

Adapted to main outside the conflicts:
- tests/unit_tests/ssm/test_hybrid_block.py: Main's MTP overlap test
  now also requires the embd group. Without it, the PR's LanguageModule
  check raises before HybridModel reaches the guard the test expects.
- tests/unit_tests/inference/coordinator_pairwise_utils.py,
  tests/unit_tests/inference/engines/batch_invariant_test_utils.py,
  tests/unit_tests/inference/engines/test_batch_invariant_models.py,
  tests/unit_tests/models/test_pos_embedding_cuda_graph.py,
  tests/unit_tests/models/test_hybrid_mhc.py: The GPTModel and
  HybridModel constructions that main added now pass
  pg_collection=ProcessGroupCollection.use_mpu_process_groups(). This
  keeps them off the deprecated fallback, as the PR does elsewhere.

The process-group allowlist records the reads main added since this PR
was written.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
…ir update to main

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.com>
Adapted to main outside the conflicts:
- tests/unit_tests/models/test_hybrid_model.py: Main's new
  config_list_model_boundary fixture gives its stand-in model a
  pg_collection with only pp set. HybridModel now requires tp and dp_cp
  on that collection, so the stand-in sets both to None explicitly.
  Explicit None groups yield the same empty logging kwargs main got
  from getattr, so main's config-list tests still reach the
  kv_channels check.

The process-group allowlist records the reads main added since this PR
was written.

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 #8900, which carries this change as commit e8a55e6 under the same title, with "Originally #8756." 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants