[https://nvbugs/6004530][fix] get_moe_cls() returned DeepGemmFusedMoE when explicitly requested via `MoeCo - #13337
Conversation
📝 WalkthroughWalkthroughThis change introduces GPU SM (Streaming Multiprocessor) version detection and capability gating for the DeepGemm MoE backend. When DeepGemm is selected, the code now detects the current GPU's SM version and falls back to the Cutlass backend if the detected version is not in the supported list. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tensorrt_llm/_torch/modules/fused_moe/create_moe.py (1)
1-1:⚠️ Potential issue | 🟠 MajorAdd SPDX/NVIDIA copyright header to this modified source file.
This file is modified in this PR but currently has no NVIDIA SPDX header block at the top.
Proposed fix
+ # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + # SPDX-License-Identifier: Apache-2.0 + # + # Licensed under the Apache License, Version 2.0 (the "License"); + # you may not use this file except in compliance with the License. + # You may obtain a copy of the License at + # + # http://www.apache.org/licenses/LICENSE-2.0 + # + # Unless required by applicable law or agreed to in writing, software + # distributed under the License is distributed on an "AS IS" BASIS, + # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + # See the License for the specific language governing permissions and + # limitations under the License. + import osAs per coding guidelines,
All TensorRT-LLM source files must contain an NVIDIA copyright header with the year of latest meaningful modification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tensorrt_llm/_torch/modules/fused_moe/create_moe.py` at line 1, Add the required NVIDIA SPDX copyright/header block at the top of the modified source file (create_moe.py in the fused_moe module) containing the SPDX identifier and the NVIDIA copyright line with the year of the latest meaningful modification; ensure the header appears before any imports (before the existing "import os") and matches the project's standard header format used across other TensorRT-LLM source files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tensorrt_llm/_torch/modules/fused_moe/fused_moe_deepgemm.py`:
- Line 380: Replace the mutable set assigned to _SUPPORTED_SM_VERSIONS with an
immutable tuple to make the constant read-only; specifically locate the symbol
_SUPPORTED_SM_VERSIONS in
tensorrt_llm._torch.modules.fused_moe.fused_moe_deepgemm (same constant used in
the parallel DenseGEMMFusedMoE) and change its value from a set literal {100,
103} to a tuple literal (100, 103) so the constant is immutable at runtime.
---
Outside diff comments:
In `@tensorrt_llm/_torch/modules/fused_moe/create_moe.py`:
- Line 1: Add the required NVIDIA SPDX copyright/header block at the top of the
modified source file (create_moe.py in the fused_moe module) containing the SPDX
identifier and the NVIDIA copyright line with the year of the latest meaningful
modification; ensure the header appears before any imports (before the existing
"import os") and matches the project's standard header format used across other
TensorRT-LLM source files.
🪄 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: Pro Plus
Run ID: 691454d4-df0f-4b6a-b82f-7fb202ccd332
📒 Files selected for processing (2)
tensorrt_llm/_torch/modules/fused_moe/create_moe.pytensorrt_llm/_torch/modules/fused_moe/fused_moe_deepgemm.py
| model_config (ModelConfig): Configuration object for the model. | ||
| """ | ||
|
|
||
| _SUPPORTED_SM_VERSIONS = {100, 103} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --type=py -C2 '_SUPPORTED_SM_VERSIONS' tensorrt_llm/_torch/modules/fused_moe
rg -n --type=py -C2 '_SUPPORTED_SM_VERSIONS\.(add|remove|update|clear|discard|pop)\(' tensorrt_llm/_torch/modules/fused_moe || trueRepository: NVIDIA/TensorRT-LLM
Length of output: 4556
Change _SUPPORTED_SM_VERSIONS from a set to a tuple to use an immutable type.
set is mutable and can be changed at runtime (RUF012); this should be an immutable constant. The parallel class DenseGEMMFusedMoE in fused_moe_densegemm.py already uses a tuple for this field.
Proposed fix
- _SUPPORTED_SM_VERSIONS = {100, 103}
+ _SUPPORTED_SM_VERSIONS = (100, 103)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _SUPPORTED_SM_VERSIONS = {100, 103} | |
| _SUPPORTED_SM_VERSIONS = (100, 103) |
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 380-380: Mutable default value for class attribute
(RUF012)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tensorrt_llm/_torch/modules/fused_moe/fused_moe_deepgemm.py` at line 380,
Replace the mutable set assigned to _SUPPORTED_SM_VERSIONS with an immutable
tuple to make the constant read-only; specifically locate the symbol
_SUPPORTED_SM_VERSIONS in
tensorrt_llm._torch.modules.fused_moe.fused_moe_deepgemm (same constant used in
the parallel DenseGEMMFusedMoE) and change its value from a set literal {100,
103} to a tuple literal (100, 103) so the constant is immutable at runtime.
| ) | ||
| return CutlassFusedMoE | ||
| elif moe_backend.upper() == "DEEPGEMM": | ||
| from tensorrt_llm._utils import get_sm_version |
There was a problem hiding this comment.
Please help to move the import to the top of the file.
29855b0 to
32d5b93
Compare
|
/bot run |
|
PR_Github #48851 [ run ] triggered by Bot. Commit: |
|
PR_Github #48851 [ run ] completed with state
|
|
/bot run |
|
PR_Github #48926 [ run ] triggered by Bot. Commit: |
|
PR_Github #48926 [ run ] completed with state
|
|
/bot run |
|
PR_Github #49136 [ run ] triggered by Bot. Commit: |
|
PR_Github #49136 [ run ] completed with state
|
3742798 to
1cc7d02
Compare
|
/bot run --disable-fail-fast |
1cc7d02 to
e6f2582
Compare
|
this fixing PR doesn't make sense to me. |
…nsupported on current SM DeepGemmFusedMoE only supports SM100/SM103 (Blackwell), but when explicitly requested via MoeConfig(backend='DEEPGEMM'), get_moe_cls() returned it without checking SM compatibility, causing assertion errors on Hopper (SM90) GPUs. Add an SM version check with graceful fallback to CutlassFusedMoE, matching the existing DenseGEMMFusedMoE pattern. Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com>
- Move `get_sm_version` import to the top of `create_moe.py` (per xxi-nv). - Make `_SUPPORTED_SM_VERSIONS` an immutable tuple in `DeepGemmFusedMoE`, matching the parallel `DenseGEMMFusedMoE` style (per CodeRabbit RUF012). Signed-off-by: Xiwen Yu <13230610+VALLIS-NERIA@users.noreply.github.com>
Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com>
e6f2582 to
f9150f6
Compare
4 participants
Summary
get_moe_cls()returnedDeepGemmFusedMoEwhen explicitly requested viaMoeConfig(backend='DEEPGEMM')without checking SM compatibility — DeepGEMM only supports SM100/SM103 but the test runs on SM90 (Hopper H20), causing a DeepGEMM assertion error about scale factor dtypeget_moe_cls()with graceful fallback to CutlassFusedMoE (which supports FP8_BLOCK_SCALES on SM90), matching the existing DenseGEMM patternTest plan
Links
Summary by CodeRabbit