Skip to content

fix(peft): correct dense MultiLoRALinear on replicated base linears - #5156

Merged
yaoyu-33 merged 12 commits into
NVIDIA-NeMo:mainfrom
radixark:yusheng/multilora-replicated-base
Jul 29, 2026
Merged

fix(peft): correct dense MultiLoRALinear on replicated base linears#5156
yaoyu-33 merged 12 commits into
NVIDIA-NeMo:mainfrom
radixark:yusheng/multilora-replicated-base

Conversation

@yushengsu-thu

Copy link
Copy Markdown
Contributor

What does this PR do ?

Fixes the dense MultiLoRALinear (#4218) on replicated base linears — TELinear parallel_mode='duplicated', i.e. the MLA q/kv down-projections of GLM-/DeepSeek-style models.

Stacked on #4218 and #5155 — this branch includes their commits as its base, so the Files tab shows all three until they merge. Only the last commit is new here. Ported from radixark#25, where it runs in production on the bridge branch.

Why

A GLM-5.2 (5-layer) multi-LoRA e2e smoke — the first MLA model through the multi-LoRA path — crashed in the first training forward. Qwen3-style models never reach these paths (all their LoRA targets are TP-parallel linears), which is why the existing e2e coverage missed them.

Two defects, both specific to replicated bases

  1. Per-slot token spans covered the full micro-batch, but a replicated base under sequence parallelism consumes the SP shard directly (TE stores parallel_mode=None for duplicated linears, so the adapter attribute derivation disables the SP gather). The grouped GEMM's offsets then exceed the row count — an out-of-bounds read surfacing as CUDA error: device-side assert triggered, not a shape error. The spans are now intersected with this rank's contiguous token window (_narrow_token_counts_to_window, the same invariant the MoE slot routing uses for its SP narrow). The host-side total is cached once per micro-batch in set_tokens_per_adapter_slot, so the forward needs no device sync.

  2. _gather_output keyed only on input_is_parallel, leaving the adapter output as a [tokens, out/tp] shard while a replicated base produces the full [tokens, out]. It now mirrors ParallelLinearAdapter's lin_out_gather_output: gather for row-parallel and replicated bases.

Deliberately unchanged: the TP collective between the two grouped GEMMs stays unconditional. ParallelLinearAdapter shards A on the rank axis whenever input_is_parallel=False — replicated bases included — so the mid all-gather is required there too (gating it on base_linear_is_parallel was tried and breaks GEMM2 with a contraction mismatch).

Testing

  • 4 new CPU tests for the window-narrowing math (slot-boundary-spanning windows, identity, single-slot, many-small-slots); the three multi-LoRA test files pass 74/74 on an H200 devbox (radixark fork, same file contents as this PR).
  • GLM-5.2 (5-layer) multi-LoRA e2e (TP=EP=4, ETP=1, SP, thd): with this fix the run completes 3 training steps end-to-end with per-step distributed adapter upserts and (tp, pp, ep) checkpoint shards, executed under CUDA_LAUNCH_BLOCKING=1 so no async assert hides.
  • Without the fix, the identical run dies in the first wrapped MLA down-projection (torch._grouped_mm device-side assert).

Changelog

  • MultiLoRALinear: narrow per-slot token spans to this rank's SP shard for replicated bases (_narrow_token_counts_to_window); cache the host-side span total in set_tokens_per_adapter_slot
  • MultiLoRALinear: gather the column-sharded adapter output for replicated bases (mirror ParallelLinearAdapter.lin_out_gather_output)
  • 4 new CPU unit tests for the window narrowing

GitHub Actions CI

See the CI section in the Contributing doc. 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

mathewjhan and others added 10 commits June 8, 2026 16:21
Signed-off-by: Mathew Han <mathewjhan@gmail.com>
Signed-off-by: Mathew Han <mathewjhan@gmail.com>
Signed-off-by: Mathew Han <mathewjhan@gmail.com>
Signed-off-by: Mathew Han <mathewjhan@gmail.com>
Signed-off-by: Mathew Han <mathewjhan@gmail.com>
Signed-off-by: Mathew Han <mathewjhan@gmail.com>
Signed-off-by: Mathew Han <mathewjhan@gmail.com>
MultiLoRA previously skipped every MoE expert linear, so on an MoE model the
experts — most of the trainable capacity — silently received no adapter.

Add MultiLoRAGroupedExpertLinear for grouped expert linears
(mlp.experts.linear_fc{1,2} of a TEGroupedMLP). Each slot is a
GroupedExpertLinearAdapter, so the packed [num_local_experts, ...] weight layout
that peft_bridge's grouped-expert export and distributed checkpointing already
understand is reused unchanged. It subclasses MultiLoRALinear so isinstance-based
multi-LoRA discovery (slot lifecycle here, per-slot optimizers and adapter-state
zeroing downstream) picks expert layers up with no changes.

The hard part is that inside the experts the row order is the dispatcher's
expert-major permutation of tokens from every EP rank, so tokens_per_adapter no
longer segments it. install_moe_slot_routing registers a forward pre-hook on each
MoE layer's experts module — the only point with both the dispatcher's
permutation metadata and the final row order — which co-permutes an int32
slot-id vector through the same stages the dispatcher applied and stable-sorts by
(slot, local expert) so one grouped GEMM covers every pair. The companion
all-to-all carries one int32 per dispatched token.

Every slot's weights stay in the autograd graph (via the stacked weights, and an
explicit zero term on the empty-batch path) because Megatron's grad buffers
expect one grad hook per trainable parameter; a slot missing from the graph would
leave its bucket incomplete.

Unsupported configurations are rejected at layer construction, not in the hook:
a hook that raised on only some ranks would leave its peers waiting in the
companion all-to-all. Rejected: expert TP > 1, fp8/fp4 experts, permute fusion
(the fused permute records TE's row_id_map rather than a token gather index),
capacity padding, and non-alltoall dispatchers. Sequential (SequentialMLP)
expert linears are still skipped, with the warning rewritten to say so.

reset_adapter now forks the expert RNG tracker instead of the dense one: the
dense seed varies with the dense TP rank, and expert-data-parallel peers differ
in exactly that rank whenever tensor and expert tensor parallel sizes differ, so
replicas of a reused slot would diverge.

Signed-off-by: Ethan (Yusheng) Su <yushengsu.thu@gmail.com>
Found by a GLM-5.2 multi-LoRA e2e smoke (MLA q/kv down-projections are
TELinear parallel_mode='duplicated'); Qwen3-style models never exercise
this case because all their LoRA targets are TP-parallel linears.

Two defects, both specific to replicated bases:

* The per-slot token spans covered the full micro-batch, but a replicated
  base under sequence parallelism consumes the SP shard directly (no
  gather), so the grouped GEMM read out of bounds — a device-side assert,
  not a shape error. The spans are now intersected with this rank's
  contiguous token window (the same invariant the MoE slot routing uses
  for its SP narrow), with the host-side total cached once per micro-batch
  in set_tokens_per_adapter_slot so the forward needs no device sync.

* _gather_output keyed only on input_is_parallel, leaving the adapter
  output as a [tokens, out/tp] shard while a replicated base produces the
  full [tokens, out]. It now mirrors ParallelLinearAdapter's
  lin_out_gather_output: gather for row-parallel AND replicated bases.

The TP collective between the two grouped GEMMs stays unconditional:
ParallelLinearAdapter shards A on the rank axis whenever
input_is_parallel=False, replicated bases included, so the mid all-gather
is required there too.

Verified: 74 unit tests pass (4 new for the window narrowing), and a
GLM-5.2_5layer multi-LoRA e2e (TP=EP=4, ETP=1, SP, thd, dp-attention
rollout) completes 3 training steps with per-step adapter upserts and
(tp, pp, ep) checkpoint shards, run with CUDA_LAUNCH_BLOCKING=1.

Signed-off-by: Ethan (Yusheng) Su <yushengsu.thu@gmail.com>
Copilot AI review requested due to automatic review settings July 28, 2026 23:18
@copy-pr-bot

copy-pr-bot Bot commented Jul 28, 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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@yaoyu-33 yaoyu-33 added area:peft Parameter-efficient fine-tuning (LoRA, adapters) bug Something isn't working needs-review PR is ready for code review and waiting on a reviewer x-radixark labels Jul 29, 2026
@yaoyu-33

Copy link
Copy Markdown
Contributor

Hi @yushengsu-thu — this needs a rebase too, and it is stacked on #5155, so please rebase that one first.

Why it conflicts: #4218 was squash-merged into main as 1f12931e2. This branch still carries #4218's six original (unsquashed) commits plus #5155's commit as its base, so git conflicts with the squashed version on main.

Fix: after rebasing #5155 (yusheng/multilora-moe-experts) per my comment there, restack this branch on it. I verified locally that this cherry-picks cleanly on top of the rebased #5155:

git checkout yusheng/multilora-replicated-base
git reset --hard yusheng/multilora-moe-experts   # already rebased onto upstream/main
git cherry-pick c78e227f795bdcb4f66ef14a3489b4f3028edd1b
git push --force-with-lease

FYI this commit does not apply directly onto main — it genuinely depends on the MultiLoRAGroupedExpertLinear code from #5155, so the stack has to be preserved.

I would have pushed the rebase for you, but the fork is owned by the radixark org and GitHub's "allow edits by maintainers" does not apply to org-owned forks, so my push is rejected.

@yaoyu-33 yaoyu-33 added waiting-on-customer Waiting on the original author to respond ready-to-merge PR is approved, current, and only waiting for CI to pass before merge and removed needs-review PR is ready for code review and waiting on a reviewer labels Jul 29, 2026
yaoyu-33 added 2 commits July 29, 2026 15:17
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>

# Conflicts:
#	src/megatron/bridge/peft/multi_lora.py
#	src/megatron/bridge/peft/multi_lora_layers.py
#	tests/unit_tests/peft/test_multi_lora.py
#	tests/unit_tests/peft/test_multi_lora_layers.py
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>

# Conflicts:
#	src/megatron/bridge/peft/multi_lora_layers.py
@yaoyu-33
yaoyu-33 merged commit 3df4fd5 into NVIDIA-NeMo:main Jul 29, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:peft Parameter-efficient fine-tuning (LoRA, adapters) bug Something isn't working community-request ready-to-merge PR is approved, current, and only waiting for CI to pass before merge waiting-on-customer Waiting on the original author to respond x-radixark

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants