[TRTLLM-12288][feat] revert all the changes of the PR#14775 - #15772
[TRTLLM-12288][feat] revert all the changes of the PR#14775#15772JadoTu wants to merge 7 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (6)
📝 WalkthroughWalkthroughRemoves the SM<100 (Hopper) NVFP4→W4A16 fallback path: deletes ChangesW4A16 NVFP4 Hopper Fallback Removal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
e20d093 to
d3f98f5
Compare
…VIDIA#14775)" Signed-off-by: jiant <107457950+JadoTu@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #56593 [ run ] triggered by Bot. Commit: |
|
PR_Github #56593 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #56893 [ run ] triggered by Bot. Commit: |
|
PR_Github #56893 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #57319 [ run ] triggered by Bot. Commit: |
|
PR_Github #57319 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #57887 [ run ] triggered by Bot. Commit: |
|
PR_Github #57887 [ run ] completed with state
|
Signed-off-by: Jian Tu <107457950+JadoTu@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #59869 [ run ] triggered by Bot. Commit: |
|
PR_Github #59869 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #60103 [ run ] triggered by Bot. Commit: |
|
PR_Github #60103 [ run ] completed with state
|
|
/bot run |
|
PR_Github #60117 [ run ] triggered by Bot. Commit: |
|
PR_Github #60117 [ run ] completed with state
|
|
/bot run |
|
PR_Github #60149 [ run ] triggered by Bot. Commit: |
|
PR_Github #60149 [ run ] completed with state
|
|
/bot run |
|
PR_Github #60190 [ run ] triggered by Bot. Commit: |
|
PR_Github #60190 [ run ] completed with state
|
|
/bot run |
|
PR_Github #60245 [ run ] triggered by Bot. Commit: |
|
PR_Github #60245 [ run ] completed with state |
BowenFu
left a comment
There was a problem hiding this comment.
LGTM. Clean, symmetric revert of #14775: every W4A16/Hopper NVFP4-dequant helper import, definition, and call-site is removed (incl. the whole triton_dequant_nvfp4.py and the Hopper SM-gating in modeling_nemotron_h.py), with no orphaned references. Native NVFP4 dispatch (NVFP4LinearMethod/NVFP4CutlassFusedMoEMethod) is unchanged — restores the pre-#14775 known-good path (superseded by #13476).
ZhanruiSunCh
left a comment
There was a problem hiding this comment.
LGTM for infra part.
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
Revert is complete — I grepped the tree for every symbol #14775 introduced (W4A16NVFP4LinearMethod, W4A16NVFP4CutlassFusedMoEMethod, triton_dequant_nvfp4, _use_w4a16_for_nvfp4_on_hopper, _force_moe_backend_for_w4a16_on_hopper) and nothing dangles in source. One stale leftover: tests/integration/defs/.test_durations:436 still lists test_nvfp4_4gpus_hopper_w4a16; auto-generated, so drop it whenever that file is next regenerated.
The one thing the description should say out loud: #13476's Marlin path is not a drop-in replacement from the user's point of view. is_nvfp4_marlin_enabled() (tensorrt_llm/_torch/utils.py:108) requires marlin to be listed explicitly in nvfp4_gemm_config.allowed_backends, and the default is ['cutlass', 'cublaslt', 'cuda_core']; the MoE side additionally needs moe_config.backend="MARLIN" (create_moe.py:66). The code being reverted here engaged automatically on SM90. So a user who runs an NVFP4 Nemotron-H checkpoint on H100 with a default config gets a load failure after this merges, where it worked before. Two asks:
- Note the required config in the PR description so the behavior change is on record.
- Since Hopper NVFP4 Nemotron-H is now a documented-config-only path, add it to the docs — either the Nemotron deployment guide or
docs/source/models/supported-models.md— with the two settings spelled out. Alternatively, if Marlin is meant to be the Hopper default now, addmarlintoNvfp4GemmConfig.allowed_backendsdefaults (it is already gated on SM90 + kernel availability, so it's inert elsewhere) and pick MARLIN for MoE on SM90+NVFP4. That would make this a true no-user-impact revert.
| task.evaluate(llm, | ||
| extra_evaluator_kwargs=self.EXTRA_EVALUATOR_KWARGS) | ||
|
|
||
| @skip_pre_hopper |
There was a problem hiding this comment.
The removed test_nvfp4_4gpus_hopper_w4a16 was the only Hopper NVFP4 Nemotron-H case exercising MTP (max_draft_len=4) together with CudaGraphConfig(enable_padding=True). test_nvfp4_marlin_multi_gpus (line 6907) is a plain MMLU run with no speculative config and no CUDA-graph config, so that combination now has no SM90 coverage at all. Add a spec-decode + cuda-graph parametrization to the Marlin test rather than letting the dimension disappear with the reverted feature.
Summary by CodeRabbit
Bug Fixes
Tests
Description
This PR is to revert the changes of #14775 because this feature has the better perf with #13476.
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.