Skip to content

fix: use explicit Megatron-FSDP V1/V2 classes in isinstance checks - #5357

Closed
shanmugamr1992 wants to merge 1 commit into
mainfrom
mcore-5382-fix
Closed

fix: use explicit Megatron-FSDP V1/V2 classes in isinstance checks#5357
shanmugamr1992 wants to merge 1 commit into
mainfrom
mcore-5382-fix

Conversation

@shanmugamr1992

Copy link
Copy Markdown

Found by the nemo-rl-testing-agent while validating
NVIDIA/Megatron-LM#5382
against the NeMo-RL Megatron functional suites. The break is not that PR's
fault — it reproduces on megatron-core main, so every NeMo-RL Megatron L1
test is currently failing at import time.

Failure

All 7 tests in L1_Functional_Tests_Megatron_4 failed with an identical
signature, reached via build_conversion_tasks ->
_get_pg_collection_from_model -> unwrap_model:

TypeError: isinstance() arg 2 must be a type, a tuple of types, or a union

Root cause

Megatron-LM #5865 split the
FSDP adapter into explicit FullyShardedDataParallelV1 / FullyShardedDataParallelV2
classes and turned FullyShardedDataParallel into a factory function that
dispatches between them. The old name is therefore no longer a type, and any
isinstance() against it raises. The factory's own docstring says so:

This is a factory function, not a wrapper type. Use the explicit V1 or V2
implementation classes for type checks.

Bridge has three isinstance sites against that name, in three different states:

Site State against megatron-core main
models/conversion/utils.py (unwrap_model) No shim — this is the crash above
training/train.py (FSDP manual buffer registration) No shim — same crash, on a path this suite does not reach
training/setup.py Already shimmed, but imports V1 only

Fix

Type-check against the explicit classes at the two crash sites, using the same
try/except ImportError idiom training/setup.py already uses so older
megatron-core (where FullyShardedDataParallel is still a class) keeps working.
V1 and V2 were both introduced by #5865, so the two-name import succeeds or
fails together.

One thing left for a maintainer rather than folded in here: training/setup.py
imports FullyShardedDataParallelV1 only, so its isinstance check silently
misses a V2-wrapped model instead of crashing. That looked like a behavioural
question the functional suite does not exercise, so I did not touch it.

Validation

L1_Functional_Tests_Megatron_4 on oci-hsg (GB200, 4 GPUs), NeMo-RL
86774472, megatron-core at Megatron-LM#5382 head eb01b689:

  • Before: 7/7 fail, all with the TypeError above.
  • After: 6/7 pass. The 7th, grpo_megatron_generation_topp_topk, no longer
    crashes — it runs to completion and misses a metric threshold
    (max(train/token_mult_prob_error) 1.078 vs < 1.06). That is a separate
    question being baselined against megatron-core main independently, and is
    unrelated to this change.

Draft: raised by an agent, needs a human review before merge.

Signed-off-by: shanmugamr1992 <shanmugamr1992@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 commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Superseded by merged #5431. Current main now covers both parts of this PR: unwrap_model() filters the concrete FSDP wrapper types and training/train.py uses _get_megatron_fsdp_types() for manual buffer registration. #5431 also added focused regression coverage in test_conversion_unwrap_utils.py and tests/unit_tests/training/test_train.py. Rebasing this draft would duplicate the implementation and tests, so no additional CI run is needed here.

@yaoyu-33 yaoyu-33 closed this Aug 8, 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.

2 participants