Skip to content

Support latent MoE shared expert overlap - #5135

Open
Victarry wants to merge 7 commits into
NVIDIA:mainfrom
Victarry:denliu/latent-moe-shared-expert-overlap-main
Open

Victarry wants to merge 7 commits into
NVIDIA:mainfrom
Victarry:denliu/latent-moe-shared-expert-overlap-main

Conversation

@Victarry

@Victarry Victarry commented Jun 3, 2026

Copy link
Copy Markdown

Summary

  • Enable latent MoE to use shared expert overlap with A2A/Flex dispatchers by running shared-expert pre-comm on full hidden states in MoELayer, while keeping shared-expert FC launches inside dispatcher overlap windows.
  • Keep dispatcher-owned shared-expert pre/post handling for the non-latent path only, and add the latent shared-expert output after fc2_latent_proj.
  • Add forward/backward parity coverage comparing shared-expert overlap vs no-overlap for latent A2A and HybridEP.

Test plan

  • PASS: uv run unit-test run tests/unit_tests/transformer/moe/test_shared_experts.py --cluster cw --platform h100 --environment dev --time-limit 0:45:00 (SLURM 12467070, 11 passed; includes latent A2A parity and test_latent_shared_expert_hybridep_forward_backward).
  • PASS: CHECK_ONLY=true BASE_REF=main UV_CACHE_DIR=/project/agentic-mcore-dev/.uv_cache uv run --project /project/agentic-mcore-dev/.claude/worktrees/latent-moe-shared-expert-overlap --extra mcore-lint -- bash tools/autoformat.sh (Black/isort/pylint/ruff clean; wrapper exits 0; mypy still prints existing missing-dependency/type issues from this local lint env).
  • PASS: tools/check_copyright.py on changed Python files.

Notes

  • A separate main-base replay in a temporary CW worktree reached import before test collection but was blocked by the local mcore-ci-dev image having nvidia-resiliency-ext < 0.6.0, while current origin/main requires nvidia-resiliency-ext==0.6.0 (SLURM 12467214).

@Victarry
Victarry requested review from a team as code owners June 3, 2026 14:02
@copy-pr-bot

copy-pr-bot Bot commented Jun 3, 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.

@svcnvidia-nemo-ci
svcnvidia-nemo-ci marked this pull request as draft June 3, 2026 14:03
@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

This PR has been automatically converted to draft because all PRs must start as drafts.

When you are ready for review, click Ready for Review to begin the review process. This will:

  1. Add the oncall reviewer (optional reviewer)
  2. Add required review teams based on your changes

See the contribution guide for more details.

@kvareddy
kvareddy requested a review from sidsingh-nvidia June 3, 2026 14:59
@kvareddy

kvareddy commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

@sidsingh-nvidia can you take a look at this MR. You added this support on inference side.

@Victarry
Victarry force-pushed the denliu/latent-moe-shared-expert-overlap-main branch from ef1db8f to df7e3d4 Compare June 10, 2026 15:26
@Victarry

Copy link
Copy Markdown
Author

Folded the FSDP shared-expert stream synchronization fix into this latent MoE shared expert overlap PR. I also adjusted the implementation to use an opt-in parameter marker from shared expert overlap, so Megatron FSDP handles a generic side-stream grad processing requirement instead of importing MoE shared expert classes or matching parameter names.

@Phlip79
Phlip79 marked this pull request as ready for review September 21, 2026 21:56
@Phlip79

Phlip79 commented Sep 21, 2026

Copy link
Copy Markdown
Member

/claude fix

@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

🛠️ Claude fix commit 6b1d53cdb18d (attempt 1)

⚠️ This explanation is AI-generated and may be inaccurate; the exact commit is authoritative.

What changed
Resolved the only merge conflict (3 hunks) in tests/unit_tests/transformer/moe/test_shared_experts.py as a union: kept the PR's is_hybrid_ep_available and its **config_kwargs helper signature, plus main's TE fakes and new tests, while adopting main's _set_random_seed seeding call. All four test methods retained under one teardown_method; removed the unused model_parallel_cuda_manual_seed import. Markers gone, ast.parse passes, 14 tests, no over-length lines. Not committed or staged.

Files changed by Claude

  • tests/unit_tests/transformer/moe/test_shared_experts.py

Why
Steering was empty and the CI log dir was empty, so the conflict was the only actionable failure. The PR hoisted the inlined test body into a **config_kwargs helper; main independently migrated seeding to _set_random_seed and added fixtures/tests. Either raw side of the middle hunk breaks the file: main's references an out-of-scope dispatcher_type inside the helper, and the PR's calls a seed helper whose import main deleted.

DCO
@Victarry, please fix any DCO failures on your commits before merge. DCO does not block this workflow.

View exact commit

Sanitized and posted by svcnvidia-nemo-ci.

@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

/ok to test 6b1d53c

svcnvidia-nemo-ci added a commit to Victarry/Megatron-LM that referenced this pull request Sep 21, 2026
Signed-off-by: svcnvidia-nemo-ci <svcnvidia-nemo-ci@nvidia.com>
svcnvidia-nemo-ci added a commit to Victarry/Megatron-LM that referenced this pull request Sep 21, 2026
Signed-off-by: svcnvidia-nemo-ci <svcnvidia-nemo-ci@nvidia.com>
@svcnvidia-nemo-ci
svcnvidia-nemo-ci force-pushed the denliu/latent-moe-shared-expert-overlap-main branch from 6b1d53c to 3edbd80 Compare September 21, 2026 22:16
@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

🛠️ Claude fix commit 3edbd80c1bf3 (attempt 2)

⚠️ This explanation is AI-generated and may be inaccurate; the exact commit is authoritative.

What changed
Edited tests/unit_tests/transformer/moe/test_shared_experts.py line 540: replaced pre_forward_comm(hidden_states) with register_input(hidden_states) plus pre_forward_comm(). One file, 2 added, 1 removed, uncommitted. Base merge was already clean, so no conflicts. Left the determinism-coverage lint alone; it needs a non-PR file or a maintainer label. Verified statically only, as torch and pytest are absent.

Files changed by Claude

  • tests/unit_tests/transformer/moe/test_shared_experts.py

Why
PR commit 93fcd2f dropped the input parameter from SharedExpertMLP.pre_forward_comm, moving capture into a new register_input(). It fixed both token_dispatcher.py callers but missed this test, which the PR also edits. The call passed hidden_states positionally, binding it to wait_current_stream and tripping the new assert. CI had not shown it, as unit tests were skipped behind the lint failure.

DCO
@Victarry, please fix any DCO failures on your commits before merge. DCO does not block this workflow.

View exact commit

Sanitized and posted by svcnvidia-nemo-ci.

@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

/ok to test 3edbd80

Signed-off-by: svcnvidia-nemo-ci <svcnvidia-nemo-ci@nvidia.com>
@svcnvidia-nemo-ci
svcnvidia-nemo-ci force-pushed the denliu/latent-moe-shared-expert-overlap-main branch from 3edbd80 to d96f0b9 Compare September 21, 2026 22:28
@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

🛠️ Claude fix commit d96f0b9723c4 (attempt 3)

⚠️ This explanation is AI-generated and may be inaccurate; the exact commit is authoritative.

What changed
Bumped the copyright header in pr-head/megatron/core/transformer/moe/shared_experts.py from 2025 to 2026 (one line, line 1). This clears the failing tools/check_copyright.py check, which now passes on all four files the PR changes. Left the kernel determinism coverage gate failing and made no other edits. Changes are uncommitted in pr-head/.

Files changed by Claude

  • megatron/core/transformer/moe/shared_experts.py

Why
The base merge was already committed and clean (no conflicts/markers; call sites consistent). Unit tests were "skipped", so the CICD failure was only downstream of linting. The reported determinism-coverage failure is out of policy to fix: its remedies are editing a determinism test the PR never touched (broadening scope) or adding a PR label (needs network). The fixable failure was check_copyright.py: the PR's own commit b4076ab bumped 3 of its 4 changed files but missed shared_experts.py.

DCO
@Victarry, please fix any DCO failures on your commits before merge. DCO does not block this workflow.

View exact commit

Sanitized and posted by svcnvidia-nemo-ci.

@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

/ok to test d96f0b9

@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

❌ Claude fix stopped after attempt 3; supported lint or unit tests still fail. View exact-SHA CI.

@svcnvidia-nemo-ci svcnvidia-nemo-ci added the waiting-on-customer Waiting on the original author to respond label Sep 22, 2026
@NVSkySC NVSkySC closed this Oct 5, 2026
@NVSkySC NVSkySC reopened this Oct 5, 2026
@svcnvidia-nemo-ci svcnvidia-nemo-ci added waiting-on-maintainers Waiting on maintainers to respond and removed waiting-on-customer Waiting on the original author to respond labels Oct 29, 2026

This branch was previously deployed

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

Labels

community-request waiting-on-maintainers Waiting on maintainers to respond

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants