Skip to content

fix(model): pass pp_rank to callable transformer layer specs - #5752

Open
pruprakash wants to merge 1 commit into
mainfrom
pruprakash/fix_model_gpt_provider_pp_rank_spec
Open

fix(model): pass pp_rank to callable transformer layer specs#5752
pruprakash wants to merge 1 commit into
mainfrom
pruprakash/fix_model_gpt_provider_pp_rank_spec

Conversation

@pruprakash

Copy link
Copy Markdown
Contributor

What does this PR do ?

Fixes Qwen3-Next/GDN, Qwen3.5, GLM-5 and DeepSeek-V4 models failing to build with AssertionError: pipeline_model parallel group is not initialized under use_decentralized_pg=True, by passing pp_rank to callable transformer layer specs.

Changelog

  • src/megatron/bridge/models/gpt_provider.py: new _callable_spec_kwargs() resolves pp_rank from _pg_collection.pp; used by provide() and mtp_block_spec().
  • tests/unit_tests/models/test_gpt_provider.py: 4 tests covering both added branches.

GitHub Actions CI

See the CI section in the Contributing doc for how to trigger the CI. A Nvidia developer will need to approve and trigger the CI for external contributors.

Before your PR is "Ready for review"

Pre checks:

  • Make sure you read and followed Contributor guidelines
  • Did you write any new necessary tests?
  • Did you add or update any necessary documentation?
  • Does the PR affect components that are optional to install? (Ex: Numba, Pynini, Apex etc)
    • Reviewer: Does the PR have correct import guards for all optional libraries?

Additional Information

  • Root cause: gpt_provider.py:257-263 introspected the spec callable for vp_stage only, so MCore block-spec builders fell back to parallel_state.get_pipeline_model_parallel_rank(), which decentralized runs never initialize. PP-degree independent: PP=1 fails identically.
  • Blast radius: every provider installing a block-spec builder as transformer_layer_spec; blocks eval-CP ([training, perf] feat: Add eval-time context parallelism via decentralized PG rebinding #3755, [training] fix: refresh GDN metadata for eval CP #5624), which requires use_decentralized_pg=True.
  • Regression? No — the vp_stage-only call predates the repo rename (958fb9b7, 2025-07-16); reachable since M4: Model providers update to use pg_collection #1326.
  • Verification: red-green in nvcr.io/nvidian/nemo:nightly — RED 2 failed with the fix reverted; GREEN 32 passed / 1 skipped for the whole file. 2-GPU e2e: the GDN model now builds on both ranks and reaches the training forward, where it hits an unrelated gdn.py:206 activation assert (separate issue, not addressed here).
  • Not included: get_gpt_mtp_block_spec is not given pp_rank — MCore's get_mtp_num_layers_to_build ignores it and uses the global anyway.

GPTModelProvider.provide() forwarded only vp_stage to a callable
transformer_layer_spec, so Megatron-Core block-spec builders fell back to
parallel_state.get_pipeline_model_parallel_rank(). Under
use_decentralized_pg=True the MPU globals are never initialized, so building
any block-spec model asserted with "pipeline_model parallel group is not
initialized" before the first forward. mtp_block_spec() had the same omission.

Adds unit tests covering the change (red-green verified).
Detected by: megatron-bridge QA (test_eval_cp_gdn_metadata_e2e)

Signed-off-by: Pruthviraj Prakash <pruprakash@nvidia.com>
@pruprakash pruprakash added bug Something isn't working area:model Model implementations and HF bridge logic needs-review PR is ready for code review and waiting on a reviewer qa_rcca_done labels Aug 23, 2026
@copy-pr-bot

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

@claude

claude Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

LGTM - clean consolidation of the duplicated inspect.signature spec-invocation logic into _callable_spec_kwargs, with pp_rank resolved from the provider's own _pg_collection.pp instead of MPU globals (correct for decentralized runs).

Verified:

  • New imports (get_pg_rank, Any/Callable/Optional) are all present.
  • _pg_collection defaults to None; the helper guards with getattr(..., None) plus 'is not None', so the no-collection path safely leaves the spec's own default (matches the regression test).
  • Both call sites (provide and mtp_block_spec) now route through the shared helper - no stale copies of the old pattern remain in models/.
  • Tests cover the four relevant paths: pp_rank forwarded, spec without pp_rank, no _pg_collection, and the MTP re-invocation.

Minor (non-blocking): a spec that collects args via **kwargs (rather than a named pp_rank/vp_stage parameter) will not receive these kwargs, since the helper keys off signature parameter names. This matches the pre-existing vp_stage behavior, so it is consistent.

Suggested test cases

  • No perf tests impacted.

@yaoyu-33 yaoyu-33 added the needs-more-tests Requires additional L0 and L1 test coverage before merge label Aug 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:model Model implementations and HF bridge logic bug Something isn't working 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 qa_rcca_done

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants