[TRTLLM-12507][feat] Per-expert lora support with Cutlass backend - #14801
Conversation
0a959ff to
00cae91
Compare
00cae91 to
71eec3c
Compare
📝 WalkthroughWalkthroughImplemented routed-expert MoE LoRA support by extending the C++ ChangesRouted-expert MoE LoRA implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/unittest/_torch/lora/test_moe_layout.py (1)
18-20: ⚡ Quick winVerify consistency with validation module's constant.
The test comment claims to check that the module names "match the validator's set," but it compares against a hardcoded set instead of importing
MOE_LORA_MODULE_NAMESfromvalidation.py. Import both constants and verify they match to prevent drift.♻️ Proposed fix
+from tensorrt_llm._torch.peft.lora.validation import MOE_LORA_MODULE_NAMES from tensorrt_llm._torch.peft.lora.moe_layout import ( MOE_LORA_MODULES, make_per_expert_lora, reference_moe_lora_delta, ) def test_module_list_complete(): - # Sanity check: the canonical module names match the validator's set. - assert set(MOE_LORA_MODULES) == {"moe_h_to_4h", "moe_4h_to_h", "moe_gate"} + """Verify that moe_layout.MOE_LORA_MODULES and validation.MOE_LORA_MODULE_NAMES + contain the same module names.""" + assert set(MOE_LORA_MODULES) == MOE_LORA_MODULE_NAMES🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/_torch/lora/test_moe_layout.py` around lines 18 - 20, Replace the hardcoded set assertion with a comparison against the validator's constant: import MOE_LORA_MODULE_NAMES from the validation module and assert that MOE_LORA_MODULES == MOE_LORA_MODULE_NAMES (using the existing symbol names MOE_LORA_MODULES and MOE_LORA_MODULE_NAMES) so the test verifies the two canonical lists stay consistent rather than comparing to a literal set.tensorrt_llm/_torch/peft/lora/moe_layout.py (2)
165-165: 💤 Low valuePrefix unused unpacked variable with underscore.
The
num_expertsvariable is unpacked fromw3_w1.shapebut never referenced in the function body. Prefix it with_to signal it's intentionally unused.♻️ Proposed fix
- num_experts, two_inter, hidden_check = w3_w1.shape + _num_experts, two_inter, hidden_check = w3_w1.shape🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tensorrt_llm/_torch/peft/lora/moe_layout.py` at line 165, The tuple unpacking for w3_w1 currently assigns num_experts but it's never used; change the unpack to prefix the unused variable with an underscore (e.g., _num_experts, two_inter, hidden_check = w3_w1.shape) so the intent is clear and linters won't flag it—update the unpacking where w3_w1.shape is consumed in the moe_layout logic.
26-26: 💤 Low valueConsider moving device initialization inside the function.
While
torch.device("cpu")is immutable and won't cause the classic mutable-default bug, the coding guidelines prefer avoiding function calls in default arguments. Move the default initialization inside the function body for consistency.♻️ Proposed refactor
*, dtype: torch.dtype = torch.bfloat16, - device: torch.device = torch.device("cpu"), + device: torch.device | None = None, seed: Optional[int] = None, ) -> Dict[str, torch.Tensor]: """Generate a per-expert (A, B) LoRA tensor pair for an MoE module. Returns shapes `A: [E, rank, in_dim]` and `B: [E, out_dim, rank]`, with an independent low-rank pair per expert, suitable for `torch.stack(...)` in `tensorrt_llm/lora_manager.py`. Args: num_experts: number of experts in this MoE layer. rank: LoRA rank for this module. in_dim: input hidden size (e.g. `hidden_size` for moe_h_to_4h). out_dim: output hidden size (e.g. `intermediate_size` for moe_h_to_4h). dtype: tensor dtype. - device: tensor device. + device: tensor device (defaults to CPU). seed: optional torch RNG seed for deterministic generation. Returns: Dict with keys "A" (shape [E, rank, in_dim]) and "B" (shape [E, out_dim, rank]). """ + if device is None: + device = torch.device("cpu") if seed is not None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tensorrt_llm/_torch/peft/lora/moe_layout.py` at line 26, The parameter currently uses a call in its default value: device: torch.device = torch.device("cpu"); change the signature to accept device: Optional[torch.device] = None (or device=None) and inside the function body set device = device if device is not None else torch.device("cpu"); update any type hints/imports as needed and keep the variable name 'device' so usages in the function (e.g., in moe_layout-related logic) remain unchanged.tensorrt_llm/_torch/modules/fused_moe/fused_moe_cutlass.py (1)
383-474: 💤 Low valueConsider defensive access for
num_seqskey.Line 452 accesses
lora_params["num_seqs"]directly. If the LoRA infrastructure ever passeslora_paramswithout this key, the resultingKeyErrorwould be less informative than a targeted error message. This is a minor robustness concern.🛡️ Optional defensive fix
- num_seqs = lora_params["num_seqs"] + num_seqs = lora_params.get("num_seqs") + if num_seqs is None: + raise ValueError( + "lora_params must contain 'num_seqs' when MoE LoRA tensors are active" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tensorrt_llm/_torch/modules/fused_moe/fused_moe_cutlass.py` around lines 383 - 474, The _extract_moe_lora_tensors method uses lora_params["num_seqs"] directly which can raise an unhelpful KeyError; update _extract_moe_lora_tensors to defensively check for the presence and validity of "num_seqs" (e.g., use lora_params.get("num_seqs") and verify it's an int > 0) and raise a clear ValueError if missing/invalid, so subsequent slices like kernel_ranks["fc1"][:num_seqs] and lora_params["host_request_types"][:num_seqs] are safe and the error message names the missing/invalid "num_seqs" key and the layer (reference self.layer_idx and function name _extract_moe_lora_tensors).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/source/features/lora.md`:
- Around line 193-199: The example snippet is missing the torch import required
for torch.bfloat16; add an import statement for torch at the top of the example
so the call to make_per_expert_lora (and the variable fc1_adapter) can use
torch.bfloat16 without error. Ensure the snippet includes "import torch" before
the call to make_per_expert_lora.
---
Nitpick comments:
In `@tensorrt_llm/_torch/modules/fused_moe/fused_moe_cutlass.py`:
- Around line 383-474: The _extract_moe_lora_tensors method uses
lora_params["num_seqs"] directly which can raise an unhelpful KeyError; update
_extract_moe_lora_tensors to defensively check for the presence and validity of
"num_seqs" (e.g., use lora_params.get("num_seqs") and verify it's an int > 0)
and raise a clear ValueError if missing/invalid, so subsequent slices like
kernel_ranks["fc1"][:num_seqs] and lora_params["host_request_types"][:num_seqs]
are safe and the error message names the missing/invalid "num_seqs" key and the
layer (reference self.layer_idx and function name _extract_moe_lora_tensors).
In `@tensorrt_llm/_torch/peft/lora/moe_layout.py`:
- Line 165: The tuple unpacking for w3_w1 currently assigns num_experts but it's
never used; change the unpack to prefix the unused variable with an underscore
(e.g., _num_experts, two_inter, hidden_check = w3_w1.shape) so the intent is
clear and linters won't flag it—update the unpacking where w3_w1.shape is
consumed in the moe_layout logic.
- Line 26: The parameter currently uses a call in its default value: device:
torch.device = torch.device("cpu"); change the signature to accept device:
Optional[torch.device] = None (or device=None) and inside the function body set
device = device if device is not None else torch.device("cpu"); update any type
hints/imports as needed and keep the variable name 'device' so usages in the
function (e.g., in moe_layout-related logic) remain unchanged.
In `@tests/unittest/_torch/lora/test_moe_layout.py`:
- Around line 18-20: Replace the hardcoded set assertion with a comparison
against the validator's constant: import MOE_LORA_MODULE_NAMES from the
validation module and assert that MOE_LORA_MODULES == MOE_LORA_MODULE_NAMES
(using the existing symbol names MOE_LORA_MODULES and MOE_LORA_MODULE_NAMES) so
the test verifies the two canonical lists stay consistent rather than comparing
to a literal set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c0ad5f96-e7fe-4325-b6b7-11e93f61d4ae
📒 Files selected for processing (11)
cpp/tensorrt_llm/thop/moeOp.cppdocs/source/features/lora.mdtensorrt_llm/_torch/custom_ops/torch_custom_ops.pytensorrt_llm/_torch/models/modeling_qwen_moe.pytensorrt_llm/_torch/modules/fused_moe/create_moe.pytensorrt_llm/_torch/modules/fused_moe/fused_moe_cutlass.pytensorrt_llm/_torch/peft/lora/moe_layout.pytensorrt_llm/_torch/peft/lora/validation.pytests/unittest/_torch/lora/test_moe_layout.pytests/unittest/_torch/lora/test_moe_lora_op.pytests/unittest/_torch/lora/test_moe_lora_validator.py
|
/bot run --disable-fail-fast |
|
PR_Github #51391 [ run ] triggered by Bot. Commit: |
266a23f to
58527c5
Compare
Signed-off-by: Balaram Buddharaju <169953907+brb-nv@users.noreply.github.com>
Signed-off-by: Balaram Buddharaju <169953907+brb-nv@users.noreply.github.com>
58527c5 to
1e5cfe2
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #51401 [ run ] triggered by Bot. Commit: |
|
PR_Github #51391 [ run ] completed with state |
|
PR_Github #51401 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #51456 [ run ] triggered by Bot. Commit: |
|
PR_Github #51456 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #51648 [ run ] triggered by Bot. Commit: |
|
PR_Github #51648 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #51681 [ run ] triggered by Bot. Commit: |
|
/bot run --disable-fail-fast |
|
PR_Github #51699 [ run ] triggered by Bot. Commit: |
|
PR_Github #51699 [ run ] completed with state |
|
PR_Github #51681 [ run ] completed with state
|
Description
This MR adds per-expert lora support in Pytorch workflow with Cutlass backend. Currently supports eager-only. Cudagraph support to be added in a follow-up MR.
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests