Skip to content

Require an explicit process-group collection in LanguageModule - #6303

Draft
Connor-XY wants to merge 1 commit into
NVIDIA:mainfrom
Connor-XY:yx/pstate-d1-require-pg-collection
Draft

Require an explicit process-group collection in LanguageModule#6303
Connor-XY wants to merge 1 commit into
NVIDIA:mainfrom
Connor-XY:yx/pstate-d1-require-pg-collection

Conversation

@Connor-XY

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

What does this PR do?

Makes an explicit pg_collection required for the model family rooted at LanguageModuleGPTModel, BertModel, T5Model, HybridModel, MambaModel.

Today:

class LanguageModule(MegatronModule):
    def __init__(self, config, pg_collection: Optional[ProcessGroupCollection] = None):
        if pg_collection is None:
            pg_collection = ProcessGroupCollection.use_mpu_process_groups()   # global grid

Pass nothing and you silently get the global parallel grid. For a model built on independent grids — a vision encoder and an LLM with different parallelism, GTP, MIMO — that is the wrong grid, and nothing raises. This is the same mechanism behind three gradient-counting bugs: #5916 (merged), #6080, #6099.

After:

        assert pg_collection is not None, (
            "LanguageModule requires an explicit pg_collection. The global parallel grid is not a "
            "safe default: a model built on independent grids would silently get the wrong one."
        )

Breaking change

This breaks external code that constructs these models without a collection. That is the intent — a silent wrong-grid default is worse than a loud failure.

Downstream impact is small. An AST scan of Megatron-Bridge finds 2 of its 9 GPTModel-family constructions without a collection (examples/megatron_mimo/llava/verify_llama_conversion.py, training/mlm_compat/model.py); its real model-construction paths — gpt_builder.py, gpt_provider.py — already thread pg_collection. nemo-rl-internal has none.

All 126 in-repo constructions are updated here: 119 tests, 6 examples, 1 in megatron/elastification.

Also fixed

T5Model accepted pg_collection and then dropped it:

- super(T5Model, self).__init__(config=config)
+ super(T5Model, self).__init__(config=config, pg_collection=pg_collection)

So a caller that did pass one still got the global grid. This was a live bug, not just a migration artefact.

Verification

Paired run against unmodified main at the same commit, same container, 8×GPU:

Tree Result
this PR 1 failed, 97 passed, 3 skipped
main 1 failed, 97 passed, 3 skipped

Identical, same single pre-existing failure; none of the added assertions fire. Suites: test_gpt_model.py, test_bert_model.py, test_hybrid_model.py, test_t5_model.py.

Contribution process

Pre-checks

  • I have added relevant unit tests
  • I have added relevant functional tests
  • I have added proper typing to my code
  • I have added relevant documentation
  • I have run the autoformatter on my PR

LanguageModule fell back to the global parallel grid when no collection was
passed, so GPTModel/BertModel/T5Model/HybridModel/MambaModel silently inherited
it. For a model on independent grids that is the wrong grid, and nothing raises.
The fallback becomes an assertion naming the missing argument.

All 126 in-repo constructors across the family now pass a collection: 119 in
tests, 6 in examples, 1 in megatron/elastification.

Downstream impact is small: Megatron-Bridge already threads pg_collection
through its real model-construction paths (gpt_builder, gpt_provider); an AST
scan finds only 2 of its 9 GPTModel-family constructions without one, and
nemo-rl-internal has none.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Yan Xu <yxu1@nvidia.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.

@Connor-XY

Copy link
Copy Markdown
Contributor Author

Part of #6307 — tracking issue for the parallel_state deprecation, with the landing order, the decisions needed, and what remains.

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