[None][feat] top-k: route decode to CuTe DSL GVR top-k in e2e#16420
Conversation
📝 WalkthroughWalkthroughThe change propagates ChangesCuTe DSL heuristic TopK
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Config
participant DSAtrtllmAttentionMetadata
participant Indexer
participant cute_dsl_gvr_topk_decode
Config->>DSAtrtllmAttentionMetadata: propagate use_cute_dsl_topk
DSAtrtllmAttentionMetadata->>DSAtrtllmAttentionMetadata: update kv_lens_row_reorder
DSAtrtllmAttentionMetadata->>Indexer: provide row reorder
Indexer->>cute_dsl_gvr_topk_decode: pass order_row and TopK parameters
cute_dsl_gvr_topk_decode-->>Indexer: produce heuristic TopK results
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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 (2)
tensorrt_llm/_torch/attention_backend/sparse/dsa.py (2)
1791-1805: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winWarm up the GVR kernel before capture.
warmup_heuristic_topk_decode()covers the other top-k runners, but it never precompilesCuteDSLGvrTopKDecodeRunner._compile/_compile_lb. Add a GVR warmup in the same eager path so the firstcute_dsl_gvr_topk_decodecall can’t JIT inside CUDA graph capture.🤖 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/attention_backend/sparse/dsa.py` around lines 1791 - 1805, Extend the layer_idx == 0 eager warmup path around warmup_heuristic_topk_decode to also precompile the GVR top-k decode runner, covering both CuteDSLGvrTopKDecodeRunner._compile and _compile_lb. Invoke the existing GVR warmup mechanism before CUDA Graph capture, while preserving the current conditional behavior and top-k configuration.
1788-1789: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGate GVR Top-K decode on SM100/103 only
_enable_heuristic_topkat 1788-1789 is too broad for the newcute_dsl_gvr_topk_decodebranch at 2785-2803. That op only supports SM100/103, so SM120/121 can reach it and raise at runtime instead of falling back. Narrow the dispatch withis_sm_100f()(orget_sm_version() in (100, 103)).🤖 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/attention_backend/sparse/dsa.py` around lines 1788 - 1789, Narrow the _enable_heuristic_topk condition to enable heuristic Top-K only for SM100/103 architectures, using is_sm_100f() or an equivalent get_sm_version() check. Preserve the existing sparse_params.enable_heuristic_topk requirement so SM120/121 fall back instead of entering cute_dsl_gvr_topk_decode.
🧹 Nitpick comments (1)
tensorrt_llm/_torch/attention_backend/sparse/deepseek_v4/deepseek_v4.py (1)
239-258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo parallel builders construct
DeepSeekV4MetadataParamswith duplicated field-mapping logic. This PR had to adduse_cute_dsl_topk=...in both places to keep them in sync — a concrete DRY violation that risks future drift if only one path gets updated.
tensorrt_llm/_torch/attention_backend/sparse/deepseek_v4/deepseek_v4.py#L239-L258: consider havingmake_deepseek_v4_sparse_metadata_params()delegate to (or be replaced by)DeepSeekV4SparseAttentionConfig.to_sparse_metadata_params(), or extract one shared field-mapping helper both call sites use, so new fields only need to be wired once. Verify whether this function still has an active caller distinct fromto_sparse_metadata_params()before merging the two.tensorrt_llm/llmapi/llm_args.py#L1060-L1069: keep as the canonical Pydantic-config-driven builder; ifmake_deepseek_v4_sparse_metadata_params()is confirmed dead/redundant, remove it, otherwise route it through this method.🤖 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/attention_backend/sparse/deepseek_v4/deepseek_v4.py` around lines 239 - 258, Unify the duplicated DeepSeek V4 metadata field mapping by making make_deepseek_v4_sparse_metadata_params() delegate to DeepSeekV4SparseAttentionConfig.to_sparse_metadata_params(), or extract a shared helper used by both builders; first verify whether the standalone function has an active distinct caller and remove it if redundant. In tensorrt_llm/_torch/attention_backend/sparse/deepseek_v4/deepseek_v4.py:239-258, route the builder through the shared canonical mapping or remove it; in tensorrt_llm/llmapi/llm_args.py:1060-1069, retain to_sparse_metadata_params() as the canonical Pydantic-config-driven builder and ensure both paths use it.
🤖 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 `@tests/integration/defs/accuracy/test_llm_api_pytorch.py`:
- Around line 3379-3387: Update the dsa_config construction around
disable_skip_indexer, enable_heuristic_topk, and use_cute_dsl_topk to accumulate
keyword arguments before creating a single DeepSeekSparseAttentionConfig,
preserving skip_indexer_for_short_seqs=False alongside heuristic or cute-DSl
settings when flags are combined. Mirror the existing dsa_kwargs pattern used by
test_nvfp4_multi_gpus.
---
Outside diff comments:
In `@tensorrt_llm/_torch/attention_backend/sparse/dsa.py`:
- Around line 1791-1805: Extend the layer_idx == 0 eager warmup path around
warmup_heuristic_topk_decode to also precompile the GVR top-k decode runner,
covering both CuteDSLGvrTopKDecodeRunner._compile and _compile_lb. Invoke the
existing GVR warmup mechanism before CUDA Graph capture, while preserving the
current conditional behavior and top-k configuration.
- Around line 1788-1789: Narrow the _enable_heuristic_topk condition to enable
heuristic Top-K only for SM100/103 architectures, using is_sm_100f() or an
equivalent get_sm_version() check. Preserve the existing
sparse_params.enable_heuristic_topk requirement so SM120/121 fall back instead
of entering cute_dsl_gvr_topk_decode.
---
Nitpick comments:
In `@tensorrt_llm/_torch/attention_backend/sparse/deepseek_v4/deepseek_v4.py`:
- Around line 239-258: Unify the duplicated DeepSeek V4 metadata field mapping
by making make_deepseek_v4_sparse_metadata_params() delegate to
DeepSeekV4SparseAttentionConfig.to_sparse_metadata_params(), or extract a shared
helper used by both builders; first verify whether the standalone function has
an active distinct caller and remove it if redundant. In
tensorrt_llm/_torch/attention_backend/sparse/deepseek_v4/deepseek_v4.py:239-258,
route the builder through the shared canonical mapping or remove it; in
tensorrt_llm/llmapi/llm_args.py:1060-1069, retain to_sparse_metadata_params() as
the canonical Pydantic-config-driven builder and ensure both paths use it.
🪄 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: f9cda4a4-7d17-4a2e-bcad-b6f8d4a49d24
📒 Files selected for processing (4)
tensorrt_llm/_torch/attention_backend/sparse/deepseek_v4/deepseek_v4.pytensorrt_llm/_torch/attention_backend/sparse/dsa.pytensorrt_llm/llmapi/llm_args.pytests/integration/defs/accuracy/test_llm_api_pytorch.py
|
/bot run |
50b901d to
20cb47b
Compare
|
PR_Github #59437 [ run ] triggered by Bot. Commit: |
|
/bot run --disable-fail-fast --add-multi-gpu-test |
|
PR_Github #60663 [ run ] triggered by Bot. Commit: |
The T.f32() argument was accidentally bundled into the threshold-fix commit. Under the repo-pinned nvidia-cutlass-dsl==4.5.0 the nvvm.fmin signature is fmin(a, b, *, ...) with no leading result-type parameter, so T.f32() raises TypeError at JIT time. Revert to the original 4.5.0-compatible call. Signed-off-by: Mindy Li <11663212+limin2021@users.noreply.github.com>
|
/bot run --disable-fail-fast --add-multi-gpu-test |
|
PR_Github #60663 [ run ] completed with state
|
|
PR_Github #60699 [ run ] triggered by Bot. Commit: |
|
PR_Github #60699 [ run ] completed with state
|
longcheng-nv
left a comment
There was a problem hiding this comment.
LGTM. All five review threads are resolved and verified in code: the row-count engage threshold fix (+ its new unit test), symmetric IS_CUTLASS_DSL_AVAILABLE gating, removal of the duplicate _compute_kv_lens_row_reorder call, the mtp3 GVR test case, and the exact revert of the accidental nvvm.fmin change (its 1500+ CuTe-DSL MoE failures cleared in L0 #48990).
Approving pending a green CI re-run: the 5 remaining failures on 329dd6d (A100X llmapi x2, B300 attention 'terminated unexpectedly' x2, DGX_B200 Qwen3 accuracy) are recurring infra flakes seen on unrelated PRs the same day and don't touch this PR's DSA dispatch path — please re-trigger before merge.
|
/bot run --disable-fail-fast --add-multi-gpu-test |
|
/bot run --disable-fail-fast --add-multi-gpu-test |
1 similar comment
|
/bot run --disable-fail-fast --add-multi-gpu-test |
|
PR_Github #60893 [ run ] triggered by Bot. Commit: |
|
PR_Github #60893 [ run ] completed with state
|
|
/bot reuse-pipeline |
|
PR_Github #60950 [ reuse-pipeline ] triggered by Bot. Commit: |
|
PR_Github #60950 [ reuse-pipeline ] completed with state |
|
/bot run --disable-fail-fast --add-multi-gpu-test |
|
PR_Github #60967 [ run ] triggered by Bot. Commit: |
|
PR_Github #60967 [ run ] completed with state
|
|
/bot run --disable-fail-fast --add-multi-gpu-test |
|
PR_Github #61281 [ run ] triggered by Bot. Commit: |
|
PR_Github #61281 [ run ] completed with state |
…nnahz/dep-1083-port-flashinfer-stable-va-lifecycle-for-native-all-reduce * 'main' of https://github.com/NVIDIA/TensorRT-LLM: (83 commits) [None][feat] Bind SourceIdentity to checkpoint artifacts (NVIDIA#16159) [None][infra] Waive 4 failed cases for main in pre-merge 49550 (NVIDIA#16798) [None][fix] Make FlashInfer sampling op wrappers opaque to Dynamo (NVIDIA#16732) [None][feat] top-k: route decode to CuTe DSL GVR top-k in e2e (NVIDIA#16420) [None][feat] Default GLM-5 to the Python KV-cache transceiver (NVIDIA#16524) [None][chore] Add NVTX ranges to per-iteration ADP sync points in PyExecutor (NVIDIA#16422) [https://nvbugs/6426850][test] Unwaive Qwen3.5 397B NVFP4 ADP4 TRTLLM test (NVIDIA#16348) [https://nvbugs/6445456][fix] Restore inplace ops for functionalization v2 (NVIDIA#16410) [None][infra] Waive 1 failed cases for main in pre-merge 49229 (NVIDIA#16786) [None][fix] Load DeepSeek V4 mixed-precision NVFP4 checkpoints (NVIDIA#16433) [None][feat] ADP conversation router: configurable least-queued placement for new conversations (NVIDIA#16294) [None][infra] Waive 1 failed cases for main in pre-merge 49424 (NVIDIA#16780) [None][infra] Waive 1 failed cases for main in pre-merge 49424 (NVIDIA#16781) [TRTLLMINF-188][infra] Require approval for PerfSanity wildcard runs (NVIDIA#16777) [TRTLLM-13969][feat] Support MiniMax M3 for Disaggregated Serving (NVIDIA#16017) [NVIDIA#11932][fix] Filter CUTLASS MoE GEMM tile configs by device shared memory on SM121 (NVIDIA#12704) [None][chore] Remove attention backend test waivers (NVIDIA#16723) [TRTLLM-14540][perf] Skip fp32 state round-trip in FlashInfer GDN pre… (NVIDIA#16703) [TRTLLM-13694][feat] Add IBDB recipe provenance and refresh configs (NVIDIA#16254) [None][infra] Preview/bump/main (NVIDIA#16758) ... Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
…#16420) Signed-off-by: Mindy Li <11663212+limin2021@users.noreply.github.com>
Wire the existing CuTe DSL GVR (Guess-Verify-Refine) top-k decode op into the DSA indexer decode dispatch. The flag combo
use_cute_dsl_topk=True AND enable_heuristic_topk=True now routes to cute_dsl_gvr_topk_decode, covering all compress_ratio and next_n values.
Summary by CodeRabbit
New Features
Tests
Description
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.