Skip to content

fix(peft): preserve grouped expert LoRA routing semantics - #3902

Open
hasuoshenyun wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
hasuoshenyun:hasuoshenyun/fix/expert-lora-routing
Open

hasuoshenyun wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
hasuoshenyun:hasuoshenyun/fix/expert-lora-routing

Conversation

@hasuoshenyun

@hasuoshenyun hasuoshenyun commented Sep 16, 2026

Copy link
Copy Markdown

What does this PR do ?

Fix ordinary grouped-expert LoRA so that adding an initially zero adapter does not unexpectedly change the parent expert's numerical behavior. Preserve its routing-weight placement, reduction order, dtype and backend settings, and handle empty routing safely. This PR does not add QAT.

Why this fix is independent of QAT

Wrapping an expert module with ordinary LoRA should preserve the original expert's configured behavior when the adapter update is zero. The previous wrapper did not consistently preserve projection dtype/backend settings or apply_router_weight_after_down. In particular, a parent configured to apply routing probabilities after the down projection could instead apply them before that projection in the LoRA wrapper. These formulas are algebraically equivalent for a linear down projection, but BF16 rounding and top-k reduction order differ.

Concretely, if the parent asks for route_weight * down(activation), attaching LoRA should not silently switch it to down(route_weight * activation). This fix follows the parent's setting; it does not force all models to use one routing placement. V4's default pre-down placement remains intact, while GLM's configured post-down placement is honored. It also preserves existing projection storage and handles genuinely empty inputs without dereferencing x[0].

This PR fixes that ordinary-LoRA baseline without adding quantizers. It also handles zero-token/fully-masked expert ranks, where the previous dummy path could index nonexistent x[0] or leave expected gradient connections absent. The graph-connected zero returns zero numerical contribution while retaining explicit zero gradients; it does not add a loss or train unused experts with real data. Whether such connections are needed depends on the surrounding autograd/distributed contract, not a claim that every disconnected rank necessarily hangs.

The routing, wrapper-storage and empty-route cases are separate regression-test groups. The empty-route expression is correctness-first rather than a claimed performance optimization; its full-tensor reductions and readability warrant review independently of the routing fix.

Changelog

  • Honor apply_router_weight_after_down in additive expert LoRA without merging its weights or enabling QAT.
  • Preserve source projection dtype and backend settings when constructing expert LoRA wrappers.
  • Keep empty/fully masked routes connected to autograd without indexing absent tokens.
  • Add standalone CPU numerical regression coverage for ordinary expert LoRA. These tests do not import the new QAT implementation.

Before your PR is "Ready for review"

  • Read the contributor guidelines and signed off commits.
  • Added focused tests.
  • Documented changed tensor/routing contracts in code.

Additional Information

This is the first prerequisite for targeted weight QAT. It intentionally fixes ordinary LoRA behavior and is separated from QAT to make that compatibility impact reviewable.

Before/after regression evidence

The same 164 newly added focused regression cases were run against the old implementation and the extracted standalone fix:

Implementation under test Passed Failed
Before this fix (original implementation; negative control) 86 78
After this fix (standalone fixed implementation) 164 0

The 78 failures are reproduced defects in the old code, not failures remaining after the fix. The 86 cases that already passed continue to pass. These are targeted regression cases, not a claim that the upstream project's existing test suite previously had 78 failures.

The test groups cover parent-equivalent behavior with zero adapters, configured routing placement and reduction order, preservation of projection dtype/backend settings, and empty/fully masked inputs with zero-gradient connections.

Grouped-MM/DeepEP tests use CPU doubles and are not native-kernel validation. Broader integrated regression after rebasing passed 1454 tests with 53 conditional skips; that broader run also includes the dependent QAT work and is not evidence for this commit alone.

Native backend CI should run before merge. The author has marked this PR ready for review; the dependent QAT feature remains draft.

Signed-off-by: hasuoshenyun <1225541074@qq.com>
@copy-pr-bot

copy-pr-bot Bot commented Sep 16, 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.

@hasuoshenyun
hasuoshenyun marked this pull request as ready for review September 16, 2026 07:58
@hasuoshenyun
hasuoshenyun requested a review from a team as a code owner September 16, 2026 07:58
@svcnvidia-nemo-ci svcnvidia-nemo-ci added the waiting-on-maintainers Waiting on maintainers to respond label Sep 18, 2026
@hasuoshenyun

Copy link
Copy Markdown
Author

Thanks @akoumpa for arranging the CI proxy in #3929! The CI run for the current head commit is now green. Could you please take a look when you have a chance? Happy to address any feedback.

@svcnvidia-nemo-ci svcnvidia-nemo-ci removed the waiting-on-maintainers Waiting on maintainers to respond label Sep 21, 2026

This branch was successfully deployed

3 active deployments
public ba7a25ba Deployed Sep 17, 2026 by copy-pr-bot[bot] via release / finalize / notify #4643
test ba7a25ba Deployed Sep 17, 2026 by copy-pr-bot[bot] via cicd-wait-in-queue #10810
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants