Skip to content

[model] fix: support MCore FSDP factory wrappers - #5322

Closed
cuichenx wants to merge 1 commit into
mainfrom
agent/fix-conversion-unwrap-mfsdp-v2
Closed

[model] fix: support MCore FSDP factory wrappers#5322
cuichenx wants to merge 1 commit into
mainfrom
agent/fix-conversion-unwrap-mfsdp-v2

Conversation

@cuichenx

@cuichenx cuichenx commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • keep conversion unwrap_model() compatible with both the Bridge-pinned legacy MCore FSDP class API and the newer MCore factory API
  • unwrap both concrete FullyShardedDataParallelV1 and FullyShardedDataParallelV2 wrappers without passing the factory function to isinstance()
  • add regression coverage for both API generations, including the legacy case where the V1/V2 symbols do not exist

Root cause

Newer MCore versions expose mcore_fsdp_adapter.FullyShardedDataParallel as a factory function and provide V1/V2 as the concrete wrapper classes. Bridge conversion code included the factory in an isinstance() tuple, which raises before MFSDP conversion reaches weight loading.

This is separate from the TP=2 vocabulary-scatter defect fixed by #5353. That fix is present on current main; this PR removes the newer MCore compatibility blocker that prevented QA from re-verifying the three MFSDP conversion examples.

Compatibility

  • legacy API: use FullyShardedDataParallel when it is a class; V1/V2 symbols may be absent
  • factory API: use the explicit, independent V1 and V2 wrapper classes
  • no training-path, dependency, CI, public API, or MCore submodule changes

Validation

  • Red on clean origin/main c62be0886a:
    • uv run python -m pytest tests/unit_tests/models/test_conversion_utils.py -k unwrap_model -vv
    • factory case failed with TypeError: isinstance() arg 2 must be a type, a tuple of types, or a union; legacy case passed
  • Green on ea19acb7f:
    • uv run python -m pytest tests/unit_tests/models/test_conversion_utils.py -vv — 7 passed
    • uv run pre-commit run --all-files — passed

@copy-pr-bot

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

@cuichenx

cuichenx commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 62097ac

Signed-off-by: Chen Cui <chcui@nvidia.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
@yaoyu-33
yaoyu-33 force-pushed the agent/fix-conversion-unwrap-mfsdp-v2 branch from 62097ac to ea19acb Compare August 7, 2026 23:20
@copy-pr-bot

copy-pr-bot Bot commented Aug 7, 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 marked this pull request as ready for review August 7, 2026 23:21
@yaoyu-33

yaoyu-33 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

/ok to test ea19acb

@claude

claude Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

LGTM — the change correctly handles the Megatron-Core FullyShardedDataParallel transition from a class to a factory function.

The isinstance(megatron_fsdp, type) guard is right: a factory function is not a type, so it falls through to the V1/V2 branch, while a real class is used directly. Both paths are covered by the two new unit tests (test_unwrap_model_supports_mcore_fsdp_factory and test_unwrap_model_supports_legacy_mcore_fsdp_class). Test coverage for the changed logic is good.

Minor note (non-blocking): in the factory branch, FullyShardedDataParallelV1/V2 are assumed to exist. If a future MCore ships a factory FullyShardedDataParallel without those two attributes, this raises AttributeError rather than a clear message — acceptable for now, since the factory contract implies they exist.

Suggested test cases

  • No perf tests impacted.

@yaoyu-33 yaoyu-33 added area:ckpt Checkpoint conversion, loading, export, and save paths bug Something isn't working waiting-on-customer Waiting on the original author to respond labels Aug 8, 2026
@yaoyu-33

yaoyu-33 commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Superseded by #5431, which has now merged the MCore FSDP factory-wrapper compatibility fix into main together with focused coverage in tests/unit_tests/models/test_conversion_unwrap_utils.py. Rebasing this PR now conflicts on the same implementation and would only duplicate the code/tests, so closing this PR rather than resolving it as a duplicate.

@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

area:ckpt Checkpoint conversion, loading, export, and save paths bug Something isn't working waiting-on-customer Waiting on the original author to respond

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants