[https://nvbugs/6434512][fix] Select Marlin for Qwen3.5 MoE on Hopper - #17293
[https://nvbugs/6434512][fix] Select Marlin for Qwen3.5 MoE on Hopper#17293lfr-0531 wants to merge 3 commits into
Conversation
WalkthroughQwen3.5 MoE models now share hardware-aware backend defaults. Supported Hopper configurations select MARLIN for MoE and NVFP4 GEMM. Tests cover Hopper, mixed-precision, and Blackwell configurations. ChangesQwen3.5 NVFP4 backend defaults
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
🤖 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 `@tensorrt_llm/_torch/models/modeling_qwen3_5.py`:
- Line 58: Annotate every listed function: add parameter and return annotations
to _get_qwen35_moe_model_defaults, Qwen3_5MoeForCausalLM.get_model_defaults, and
Qwen3_5MoeVLModel.get_model_defaults in
tensorrt_llm/_torch/models/modeling_qwen3_5.py; annotate monkeypatch and add ->
None to the three affected test functions in
tests/unittest/_torch/modeling/test_modeling_qwen3_5_vl_moe.py at lines 161,
184, and 201.
🪄 Autofix
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: 88037976-5a87-4cc1-85d9-fe18ea04a3b1
📒 Files selected for processing (2)
tensorrt_llm/_torch/models/modeling_qwen3_5.pytests/unittest/_torch/modeling/test_modeling_qwen3_5_vl_moe.py
|
/bot run --disable-fail-fast |
|
PR_Github #63983 [ run ] triggered by Bot. Commit: |
|
PR_Github #63983 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64036 [ run ] triggered by Bot. Commit: |
|
PR_Github #64036 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64064 [ run ] triggered by Bot. Commit: |
|
PR_Github #64064 [ run ] completed with state |
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
Change looks right for the reported H20 failure and the marlin-only GEMM list is fine in practice — the other NVFP4 backends are all SM100+, so nothing is lost on Ada/Hopper.
Two things to confirm before merge (details inline): whether the trigger covers the ModelOpt MIXED_PRECISION NVFP4 exports this file already normalizes, and whether the dense Qwen3.5 classes need the same nvfp4_gemm_config treatment.
Description nit: the auto-generated summary claims three new test functions (test_qwen35_moe_model_defaults_select_marlin_on_hopper, ..._keep_non_nvfp4_backends, ..._keep_blackwell_backends) — the diff actually adds one parametrized test with three params. Worth fixing so a future reader grepping for those names isn't confused.
One more question: MarlinFusedMoE doesn't support the MoE load balancer. Since this now flips the backend by default, is there a configuration (EPLB / wide-EP) where the new default turns a working run into a startup error? If so, a note in the helper's comment would help the next person.
| def _get_qwen35_moe_model_defaults(llm_args): | ||
| defaults = Qwen3NextForCausalLM.get_model_defaults(llm_args) | ||
| quant_config = getattr(llm_args, "quant_config", None) | ||
| if getattr( |
There was a problem hiding this comment.
This gates on the global quant_algo. ModelOpt MIXED_PRECISION exports for Qwen3.5 — which _normalize_qwen35_quant_config_dict / _lm_head_nvfp4_enabled below go to some length to handle — report quant_algo=MIXED_PRECISION with the real algorithms per layer, so they'd never take this branch and would still hit the no-supported-tactic failure on H20.
Was the validated checkpoint (Qwen3.5-397B-A17B-NVFP4) a plain NVFP4 export? If mixed-precision exports exist for this family, note that widening the predicate alone isn't enough: on SM90 _normalize_qwen35_quant_config_dict leaves routed experts at W4A16_NVFP4 (convert_to_nvfp4 is SM100/103 only), and MarlinFusedMoE.can_implement rejects anything but NVFP4. Worth either handling explicitly or stating in the comment that mixed-precision is out of scope.
There was a problem hiding this comment.
Thanks, Brian! Added MIXED_PRECISION support.
| } | ||
|
|
||
|
|
||
| def _get_qwen35_moe_model_defaults(llm_args): |
There was a problem hiding this comment.
Only the MoE classes get this. Qwen3_5ForCausalLM / Qwen3_5VLModel (dense) still inherit the default allowed_backends=['cutlass','cublaslt','cuda_core'], and all three of those NVFP4 paths require SM100+ (CudaCoreNVFP4Runner.MIN_SM_VERSION = 100), so a dense NVFP4 Qwen3.5 checkpoint on Ada/Hopper would fail on the plain Linear GEMMs.
The nvfp4_gemm_config half of this dict isn't MoE-specific — consider splitting it into a _qwen35_nvfp4_gemm_defaults(llm_args) helper wired into the dense classes too, and keeping moe_config on the MoE classes only. If dense NVFP4 exports don't exist yet, a one-line note saying so would keep the asymmetry from reading like an oversight.
There was a problem hiding this comment.
Thanks! This PR is scoped to the nvidia/Qwen3.5-397B-A17B-NVFP4 MoE checkpoint. I'll keep this change MoE-only and we can handle dense NVFP4 separately.
| } | ||
| ) | ||
|
|
||
| llm_args = SimpleNamespace(quant_config=SimpleNamespace(quant_algo=quant_algo)) |
There was a problem hiding this comment.
This builds llm_args from SimpleNamespace and compares against a literal copy of the dict the helper returns, so it's close to tautological: a typo'd field name (nvfp4_config), a wrong enum spelling ("Marlin"), or a value the Pydantic Literal rejects would all still pass here and only blow up at runtime on the machine that actually loads the model.
Suggest making at least the hopper-nvfp4 case round-trip through the real schema — construct a TorchLlmArgs, run apply_model_defaults_to_llm_args(llm_args, defaults), and assert llm_args.moe_config.backend == "MARLIN" / llm_args.nvfp4_gemm_config.allowed_backends == ["marlin"]. That also pins the behavior the fix depends on (defaults applied only when the user didn't set the field).
There was a problem hiding this comment.
Fixed. The consolidated test now uses real TorchLlmArgs and QuantConfig, applies the defaults, and checks the parsed backend fields for both text and VL MoE models.
Signed-off-by: Fanrong Li <23290157+lfr-0531@users.noreply.github.com>
Signed-off-by: Fanrong Li <23290157+lfr-0531@users.noreply.github.com>
7ea8d3f to
69b54e4
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…Hopper Signed-off-by: Fanrong Li <23290157+lfr-0531@users.noreply.github.com>
62cbf9e to
050a0d3
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #64574 [ run ] triggered by Bot. Commit: |
|
PR_Github #64574 [ run ] completed with state
|
lfr-0531
changed the title
3 participants
Description
Qwen3.5 MoE checkpoints with NVFP4 experts currently inherit the Qwen3-Next defaults without selecting a Hopper-compatible MoE GEMM backend. This affects both globally NVFP4 and mixed-precision checkpoints. On H20, the default CUTLASS W4A4 path requires Blackwell and fails during warmup because no supported MoE tactic remains.
This change preserves the existing Qwen3-Next defaults and selects Marlin for Qwen3.5 text and VL MoE models when the checkpoint quantization algorithm is NVFP4 or MIXED_PRECISION and the GPU is in the Marlin-supported SM range. Other quantization configurations and Blackwell keep their existing backend selection.
The change restores the default H20 execution path without introducing an API, dependency, documentation, ownership, or architecture change. Same-node 8-GPU H20 measurements confirmed that the fixed automatic selection matches an explicit-Marlin reference within normal run-to-run variation.
Related NVBug: https://nvbugs/6434512
Test Coverage
pytest tests/unittest/_torch/modeling/test_modeling_qwen3_5_vl_moe.py -k qwen35_moe_model_defaults: 3 passed, 6 deselected.050a0d3036and the modifiedmodeling_qwen3_5.py.tests/integration/test_lists/qa/llm_perf_core.yml: all passed on 8 H20 GPUs.Qwen3.5-397B-A17B-NVFP4-V2verification on 8 H20 GPUs:MIXED_PRECISIONwith 375 FP8 and 60 NVFP4 layer entries.No supported MoE GEMM tactic remains after replacing unsupported NO_SMEM epilogues.MoeConfig(backend='MARLIN')withallowed_backends=['marlin'].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.