Repository navigation
[#16801][fix] Use layer-aware KV indices in Vanilla attention - #16803
mihai-chiorean wants to merge 8 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. ℹ️ Recent review info
WalkthroughVanilla attention detects layer-specific KV cache layouts, invalidates stale cache-index state after manager changes, and defers lookup until execution. Ordinary and MLA generation use the executing layer index. Tests cover uniform and layer-specific layouts. ChangesVanilla cache index resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Merge Risk:
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 4.55% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Title check ✅ Passed The title clearly and concisely describes the primary change: using layer-aware KV indices in Vanilla attention. Description check ✅ Passed The description explains the issue, implementation, validation, scope, and checklist. It provides focused test coverage and relevant runtime validation, although it uses equivalent headings instead of… Linked Issues check ✅ Passed The changes address issue #16801by deferring KV index lookup for layer-specific layouts, resolving indices with the executing layer for ordinary and MLA generation, preserving the uniform-manager fas…Out of Scope Changes check ✅ Passed The code and test changes are directly related to correcting layer-aware KV index resolution in Vanilla attention. No unrelated or out-of-scope changes are evident.
Full details: Description check
-
Explanation
The description explains the issue, implementation, validation, scope, and checklist. It provides focused test coverage and relevant runtime validation, although it uses equivalent headings instead of the template headings.
Full details: Linked Issues check
-
Explanation
The changes address issue
#16801by deferring KV index lookup for layer-specific layouts, resolving indices with the executing layer for ordinary and MLA generation, preserving the uniform-manager fast path, and adding focused regression coverage.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create PR with unit tests
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
5412ca0 to
aaaf44a
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. |
|
I think Vanilla Attention @yihwang-nv is more familiar, could you please help to review? |
|
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. |
66d6949 to
ae36d28
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. |
|
Addressed the latest review note in 93e6400 by reusing the KV cache manager already validated in prepare(). Current main is merged, pre-commit is green, and there are no unresolved conversations. @yihwang-nv, could you take another look when convenient? |
|
The latest review feedback is addressed in |
93e6400 to
e79eeac
Compare
|
Keeping @yihwang-nv as the trt-llm-torch-attention-devs reviewer based on their existing review activity on this PR. This confirms existing ownership; it does not request another review or add review points. Why this reviewer is retainedEvidence: inline review comment at 2026-08-19T06:50:00Z. The bot keeps the first eligible attention reviewer who started reviewing. Current target-branch CODEOWNERS includes attention-owned changes in this PR. |
Vanilla attention metadata is prepared once and shared by every layer. Resolving block IDs during prepare therefore has no layer identity, so VSWA, hybrid-linear, and multi-pool managers can reject the lookup or return indices for the wrong page-index space. Defer block-ID resolution for layer-specific managers and query with the executing layer in ordinary and MLA generation paths. Preserve the existing prepared-once lookup for uniform single-pool managers to avoid adding overhead to that path. Signed-off-by: Mihai Chiorean <mihai.v.chiorean@gmail.com>
Signed-off-by: Mihai Chiorean <mihai.v.chiorean@gmail.com>
KVCacheManagerV2 can use different page-index scales for layers that share one physical pool. Treat that manager capability as layer-specific so Vanilla resolves cache indices with the executing layer instead of the pool default. Signed-off-by: Mihai Chiorean <mihai.v.chiorean@gmail.com>
Detect differing per-layer page-index scales once and cache the result in attention metadata. Uniform single-pool V2 managers keep the prepared-once lookup, while heterogeneous layouts still resolve indices with the executing layer. Signed-off-by: Mihai Chiorean <mihai.v.chiorean@gmail.com>
Signed-off-by: Mihai Chiorean <mihai.v.chiorean@gmail.com>
Signed-off-by: Mihai Chiorean <mihai.v.chiorean@gmail.com>
Signed-off-by: Mihai Chiorean <mihai.v.chiorean@gmail.com>
append_mla_latent_cache now lives in tensorrt_llm._torch.attention.backends.utils; the attention_backend compatibility shim does not re-export the utils module, so the patch target failed to resolve. Signed-off-by: Mihai Chiorean <mihai.v.chiorean@gmail.com>
e79eeac to
7eb6406
Compare
|
@yihwang-nv friendly follow-up. I rebased onto current main ( On CPU the focused tests in |
Summary
Fixes #16801.
Resolve Vanilla-attention KV block indices with the executing layer whenever
the cache layout is layer-specific.
Vanilla metadata is shared across layers. Resolving indices during metadata
preparation without
layer_idxcan select the wrong sliding window, physicalpool, or V2 page-index scale. The same problem affects ordinary and MLA
generation.
Implementation
heterogeneous-scale V2 managers.
layer_idx=self.layer_idxin ordinary and MLAgeneration.
Validation
Validated on DGX Spark / GB10 (SM121) with Gemma 3 1B, the PyTorch
VANILLAattention backend, KVCacheManager V1, a 661-token prompt, and 32 decoded tokens.
VSWA production path
max_attention_window=[512, 512, 512, 512, 512, 1024]is_vswa=TrueUniform single-pool control
max_attention_window=[1024]is_vswa=FalseThis confirms that layer-dependent layouts resolve indices with the executing
layer while uniform layouts retain the existing prepared-once fast path.
Additional validation:
tests/unittest/_torch/attention/test_vanilla_attention.py: 8 tests and3 subtests passed on SM121.
passed.
gap and potential uniform-layout overhead were addressed before submission.
Coverage includes VSWA, linear attention, multi-pool, heterogeneous and uniform
V2 scales, ordinary generation, and MLA generation.
Scope
No public API or kernel changes. Layer-invariant layouts retain their existing
prepared-once behavior.
PR Checklist
Dev Engineer Review
QA Engineer Review
Added or updated unit tests for:
The tests are outside
tests/integration/test_lists/. No correspondingtest-db/orqa/entries were found.Verdict: needs follow-up — CBTS or integration coverage is not documented.