[https://nvbugs/6212252][fix] Lift the SM list onto DeepGemmFusedMoE._SUPPORTED_SM_VERSIONS = {100, 103} and - #14704
Conversation
📝 WalkthroughWalkthroughThis PR centralizes SM version compatibility checking for the DeepGEMM MoE backend. It extracts the supported SM versions into a class constant and uses that constant both in backend-selection fallback logic and in the existing can_implement validation method. ChangesDeepGEMM SM Compatibility Gating
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
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: 0
🧹 Nitpick comments (1)
tensorrt_llm/_torch/modules/fused_moe/fused_moe_deepgemm.py (1)
380-380: ⚡ Quick winUse
frozensetinstead ofsetfor immutable class constant.
_SUPPORTED_SM_VERSIONSis named as a constant but uses a mutableset. Replace withfrozensetto enforce immutability and align with the constant naming convention.🔒 Proposed fix
- _SUPPORTED_SM_VERSIONS = {100, 103} + _SUPPORTED_SM_VERSIONS = frozenset({100, 103})As per coding guidelines, Python constants should be immutable. The static analysis tool flagged this as RUF012 (mutable default value for class attribute).
🤖 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_deepgemm.py` at line 380, Replace the mutable set assigned to the class constant _SUPPORTED_SM_VERSIONS with an immutable frozenset to enforce immutability and follow constant naming conventions; locate the declaration of _SUPPORTED_SM_VERSIONS in fused_moe_deepgemm.py and change the RHS from {100, 103} to use frozenset({100, 103}) (or frozenset((100, 103))) so the constant cannot be mutated at runtime.
🤖 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.
Nitpick comments:
In `@tensorrt_llm/_torch/modules/fused_moe/fused_moe_deepgemm.py`:
- Line 380: Replace the mutable set assigned to the class constant
_SUPPORTED_SM_VERSIONS with an immutable frozenset to enforce immutability and
follow constant naming conventions; locate the declaration of
_SUPPORTED_SM_VERSIONS in fused_moe_deepgemm.py and change the RHS from {100,
103} to use frozenset({100, 103}) (or frozenset((100, 103))) so the constant
cannot be mutated at runtime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 04f18277-6509-4e14-ac9a-914fdb23b535
📒 Files selected for processing (2)
tensorrt_llm/_torch/modules/fused_moe/create_moe.pytensorrt_llm/_torch/modules/fused_moe/fused_moe_deepgemm.py
9a2c217 to
149d1a3
Compare
| # kernel runs and trips a C++ scale-factor dtype assertion at warmup. | ||
| from tensorrt_llm._utils import get_sm_version | ||
| sm_version = get_sm_version() | ||
| if sm_version not in DeepGemmFusedMoE._SUPPORTED_SM_VERSIONS: |
There was a problem hiding this comment.
Please add another constraint: only FP8_BLOCK QUANT on HOPPER is supported on CUTLASS.
|
Or a suggested way to fix is to specify the CUTLASS MOE backend in the test case instead of the fallback. |
149d1a3 to
649aee8
Compare
…nsupported on current SM DeepGemmFusedMoE kernels only support SM100/SM103 (Blackwell), but when explicitly requested via MoeConfig(backend='DEEPGEMM'), get_moe_cls() returned it without checking SM compatibility. On Hopper (SM90) this caused DeepGEMM to run anyway and trip a C++ assertion at autotuner warmup (`sfa_dtype == torch::kFloat`). Add an SM version check in the DEEPGEMM dispatch branch with graceful fallback to CutlassFusedMoE (which supports FP8_BLOCK_SCALES on SM90), matching the existing DENSEGEMM/CUTEDSL patterns. Lift the supported SM list onto DeepGemmFusedMoE._SUPPORTED_SM_VERSIONS for reuse. Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com>
649aee8 to
7a9d344
Compare
Summary
get_moe_cls()unconditionally returnsDeepGemmFusedMoEforbackend='DEEPGEMM'; on H20 (SM90) the Blackwell-only DeepGEMM kernel runs anyway and assertssfa_dtype == kFloat.DeepGemmFusedMoE._SUPPORTED_SM_VERSIONS = {100, 103}and add an SM check increate_moe.py's DEEPGEMM branch with fallback toCutlassFusedMoE(which supports FP8_BLOCK_SCALES on SM90), mirroring DENSEGEMM/CUTEDSL.Test plan
Links
Summary by CodeRabbit