Skip to content

fsdp: avoid double sharding of MoE experts when EP is enabled - #2833

Closed
CodersAcademy006 wants to merge 15 commits into
NVIDIA:mainfrom
CodersAcademy006:fix/fsdp-ep-double-sharding
Closed

fsdp: avoid double sharding of MoE experts when EP is enabled#2833
CodersAcademy006 wants to merge 15 commits into
NVIDIA:mainfrom
CodersAcademy006:fix/fsdp-ep-double-sharding

Conversation

@CodersAcademy006

Copy link
Copy Markdown
Contributor

When Expert Parallelism (EP) is enabled, MoE expert parameters are already
partitioned across ranks. However, current FSDP auto-wrapping logic still
wraps these expert modules, causing parameters to be effectively tracked
and sharded twice.

This results in:

  • Increased peak GPU memory usage
  • Duplicated optimizer state
  • Additional communication overhead
  • Reduced throughput compared to EP-only runs

What this PR does

  • Skips FSDP auto-wrapping for MoE expert modules that are already managed
    by Expert Parallelism
  • Adds an explicit ownership signal (expert_parallel_enabled) to MoE layers
    to avoid heuristic or name-based checks
  • Preserves existing FSDP behavior for all non-expert parameters

Impact

  • Restores expected memory behavior (EP + FSDP ≤ EP-only)
  • Reduces redundant parameter metadata and optimizer state
  • Improves training throughput
  • No behavior change for non-MoE or non-EP models

Reproduction

This issue is reproducible on large MoE models and is reported in #2772.
The change removes the redundant sharding path responsible for the memory
regression.

Fixes #2772

@CodersAcademy006
CodersAcademy006 requested review from a team as code owners January 6, 2026 16:56
@copy-pr-bot

copy-pr-bot Bot commented Jan 6, 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.

@yaox12 yaox12 added Expert Review [deprecated] Apply this label to indicate that your PR is ready for expert review. module: megatron-fsdp labels Jan 9, 2026
@chtruong814 chtruong814 added the needs-follow-up Issue needs follow-up label Jan 11, 2026
@zhujian19891203

zhujian19891203 commented Jan 13, 2026

Copy link
Copy Markdown

Hi, thank you for this work.
But I tried this PR, it can not run successfully. Because there are multiple code snippets like the following in megatron_fsdp.py and param_and_grad_buffer.py file, it will not filter expert params. For example, function _replace_param_with_distributed_if_needed in megatron_fsdp.py file, the assert will triggered.

image

@CodersAcademy006

Copy link
Copy Markdown
Contributor Author

@zhujian19891203 Thanks for testing this and for the detailed trace.

You’re right: while the current change correctly skips expert parameters during FSDP parameter grouping to avoid double sharding, the same exclusion needs to be applied in the FSDP replacement path.

In particular, _replace_param_with_distributed_if_needed still assumes all parameters are registered with FSDP, which is not true for EP-managed expert parameters. I will update the replacement logic to explicitly bypass expert parameters when expert_parallel_enabled=True, ensuring consistency across all FSDP code paths.

I’ll push a follow-up fix shortly

CodersAcademy006 and others added 7 commits January 14, 2026 11:36
- Move nv-grouped-gemm from dev/lts extras to new moe extra
- Users can now install megatron-core[dev] without build failures
- Add comprehensive error messages with installation instructions
- Update README with MoE dependencies documentation

This resolves the installation failure when nv-grouped-gemm cannot build
due to missing CUTLASS headers. Users who need MoE with grouped GEMM can
now explicitly install it with megatron-core[moe], while others can
install dev/lts extras without encountering build errors.

Fixes NVIDIA#2541
…r RL workflows. Fixed imports, ensured native RL loop support, and validated no errors in touched files. Ready for review and CI on supported platforms.
…lag, gate all logic on --rl-amem-offload-during-rollout, encapsulate env setup, and update docs/examples per review
@CodersAcademy006
CodersAcademy006 requested review from a team and pablo-garay as code owners January 16, 2026 06:40
@CodersAcademy006

Copy link
Copy Markdown
Contributor Author

I can split AMem NCCL into a follow-up PR if reviewers prefer

@shjwudp

shjwudp commented Jan 16, 2026

Copy link
Copy Markdown
Contributor

partitioned across ranks. However, current FSDP auto-wrapping logic still

Hi @CodersAcademy006 — that's awesome! 🎉 Please open a separate PR for AMem NCCL integration. We’d also really appreciate it if you could include a brief design document outlining how this feature work in RL training and its expected performance impact.

@HaochenYuan is our RL expert here can help review your code.

…m-nccl flag, gate all logic on --rl-amem-offload-during-rollout, encapsulate env setup, and update docs/examples per review"

This reverts commit ee9962c.
Remove all AMem NCCL-related code and documentation from this PR:
- Delete megatron/core/amem_nccl.py
- Delete examples/rl/train_with_amem.sh
- Revert megatron/rl/rl_utils.py to upstream (remove AMem hooks)
- Revert megatron/training/arguments.py to upstream (remove AMem flags)
- Revert megatron/training/initialize.py to upstream (remove AMem setup)
- Revert pyproject.toml to upstream (remove AMem dependencies)
- Revert scripts/check_api_backwards_compatibility.py to upstream
- Remove AMem documentation section from README.md

This PR should focus only on FSDP/EP double-sharding fixes.
AMem NCCL integration will be submitted as a separate PR as requested
by reviewers.
@CodersAcademy006

Copy link
Copy Markdown
Contributor Author

I have made the separate pr's please review. #2979 #2980

- Add assert_not_fsdp_wrapped_ep_param() helper to enforce the invariant
- Call assertion in _replace_param_with_distributed_if_needed()
- Document invariant in MoE layer where expert_parallel_enabled is set

This prevents future double-sharding regressions by making violations loud
instead of silent.
@CodersAcademy006

Copy link
Copy Markdown
Contributor Author

Added a small invariant guard to ensure EP-owned parameters are never wrapped or replaced by FSDP in any code path. This should prevent future double-sharding regressions.

# Exclude expert-parallel params from FSDP bookkeeping
self.param_to_name = {}
self.raw_param = {}
from megatron.core.utils import is_ep_owned_param

@cspades cspades Jan 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Megatron-FSDP is also standalone pip-installable, mind guarding this with a try/except MCore import, or simply having these utilities live natively under Megatron-FSDP? (After all, this is an Megatron-FSDP-specific interaction with Megatron-LM.)

@cspades
cspades requested review from a team and xuwchen January 23, 2026 20:33

@cspades cspades left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CodersAcademy006 Can you elaborate a bit more on the "double sharding"? Megatron-FSDP should be able to shard expert parameters per local expert per Expert TP rank. Are you suggesting that FSDP sharding is useless here due to the memory inflation of using FSDP with EP? (I also wonder about the loss curves you're seeing - since I believe if you detach from FSDP, no reductions would be happening on the DP-Shard / intra-DP domain right?)

#2772 seems to be resolved by a TE issue as well, looking like some bug that un-distributed the heavyweight optimizer state (which coincidentally, becomes a major issue as model size increases), so wanted clarity on whether that problem has already been solved in a less radical way.

Could you provide more technical justification on the problem between the interaction of MoE layers in MLM with Megatron-FSDP? 🙏🏻 Or maybe this PR is just out-dated and the root cause of the memory issue was resolved.

@CodersAcademy006

Copy link
Copy Markdown
Contributor Author

hey @cspades Thanks for the review. This PR is not addressing the TE FusedAdam regression from #2772 (which was correctly fixed upstream), but instead enforces a correctness invariant between Expert Parallelism and Megatron-FSDP. EP-owned expert parameters are already partitioned and communicated by EP and must never be wrapped, replaced, or sharded again by FSDP. Without an explicit exclusion, FSDP still attempts to manage these parameters in multiple code paths, which is architecturally incorrect and can reintroduce double-sharding or invalid reductions in the future. This change makes EP ownership explicit and consistently respected across all FSDP paths, independent of optimizer implementation or current memory behavior.

Please correct me otherwise.

@shjwudp

shjwudp commented Jan 27, 2026

Copy link
Copy Markdown
Contributor

Hi @CodersAcademy006,

When enabling EP, expert parameters may still have replicas. If the DP world size > the EP world size, some expert parameters will have data-parallel neighbors—this setup is known as Expert Data Parallelism (EDP). Applying FSDP on top of EDP can still yield meaningful memory savings.

Conceptually, FSDP is transparent to all parameters. Each parameter maintains its original copy; we generally don't modify it directly—instead, we create shadow shards and unshard them as needed. The function that restores module parameters to their original form is located here: megatron_fsdp.py#L1159.

Regarding your point, I believe our aim should be to maintain broad compatibility between FSDP and DTensor—fixing issues in the ecosystem where needed—rather than avoiding either FSDP or DTensor altogether. This PR unintentionally cancels out FSDP’s effect on expert parameters, which isn’t ideal.

@chtruong814 chtruong814 removed the needs-follow-up Issue needs follow-up label Jan 30, 2026
@cspades

cspades commented Mar 4, 2026

Copy link
Copy Markdown
Member

@CodersAcademy006 Just to conclude, maybe the ask is for better documentation on how to use EP-EDP-ETP distribution for MoE layers, or support for specific layers, with appropriate advice for how to actually reduce memory utilization? We definitely don't want users to try this code and come to the initial conclusion that the easiest way to get it to work is just to get rid of FSDP for MoE.

Otherwise, good to close this PR? I'll also try to improve the user experience with EP outside of Megatron-LM or custom training FW's, so hopefully all of this is better documented.

@CodersAcademy006

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review @cspades @shjwudp.

You're right — I was reasoning about the pure EP case and missed the
EDP scenario where DP world size > EP world size. In that setup, FSDP
over expert params is still meaningful and my exclusion logic would
incorrectly cancel those memory savings.

Given that #2772 was resolved upstream and the broader fix requires
DTensor/FSDP compatibility work rather than exclusion logic, I'll close
this PR. Happy to help with documentation on EP-EDP-ETP memory
behaviour if that's useful.

Also thank you for correcting my reasoning and helping me understand better with real exposure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-request Expert Review [deprecated] Apply this label to indicate that your PR is ready for expert review. module: megatron-fsdp

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Using Megatron-FSDP + EP consumes more GPU memory than using EP alone

8 participants