[None][fix] Fix Gemma4 illegal memory access when max_seq_len is at most the sliding window size - #16099
Conversation
When max_seq_len is at most the sliding window size, every attention window clamps to max_seq_len, so all layers land in one KV cache pool. Layers with different geometry (Gemma4: sliding head_dim 256, global head_dim 512) then share that pool while having different page-index scales, but page indices were converted with the single pool-level scale and shared across all layers. Global-attention layers received page ids up to 4x past their buffer, and append_paged_kv_cache crashed with CUDA_ERROR_ILLEGAL_ADDRESS during the warmup prefill on every Gemma4 checkpoint whenever max_seq_len <= 512. Default (large) max_seq_len was unaffected: pools then split per window and each pool is homogeneous. - kv_cache_manager_v2: convert per-layer page-index requests with that layer own scale (the V2 core documents that computed page indices may only be shared between buffers with equal scale). - FlashInfer metadata: share one page-index list only among layers with the same pool and scale; engage the per-pool machinery when windows differ (VSWA) or scales differ. - Add a Gemma4 dummy-weight regression test at max_seq_len 256 and 512 (real E2B geometry), registered in the B200 pre-merge list. Signed-off-by: tianruih <tianruih@nvidia.com>
📝 WalkthroughWalkthroughThis PR refines page-index scale handling for KV cache layers with differing head dimensions. A new ChangesPer-layer page-index scale support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant FlashInferAttentionMetadata
participant KVCacheManagerV2
FlashInferAttentionMetadata->>KVCacheManagerV2: get_layer_page_index_scale(layer_idx)
KVCacheManagerV2-->>FlashInferAttentionMetadata: page_index_scale (per layer)
FlashInferAttentionMetadata->>FlashInferAttentionMetadata: group layers by (pool_id, page_index_scale)
alt is_vswa true OR multiple scale groups
FlashInferAttentionMetadata->>FlashInferAttentionMetadata: build _vswa_layer_to_pool / _vswa_pool_to_rep_layer per group
else single group and not vswa
FlashInferAttentionMetadata->>FlashInferAttentionMetadata: use shared page-index list
end
🚥 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
🧹 Nitpick comments (2)
tests/unittest/_torch/modeling/test_gemma4_e2e_dummy.py (1)
245-254: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest implicitly depends on the real checkpoint's
sliding_windowremaining 512.With
shrink_hidden=False,sliding_windowis left as-is from the real E2Bconfig.jsonrather than being explicitly patched. The comment notes "sliding_window must stay 512 so both max_seq_len values are at most the window" — if the upstream checkpoint's config ever changes this value, the test would silently stop covering the intended regression scenario without failing loudly. Consider explicitly asserting/patchingsliding_window == 512in_make_dummy_config_dir(even outside theshrink_hiddenbranch) for determinism.🤖 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 `@tests/unittest/_torch/modeling/test_gemma4_e2e_dummy.py` around lines 245 - 254, The E2B dummy test setup in _make_dummy_config_dir currently relies on the upstream config.json to leave sliding_window at 512 when shrink_hidden is false, which makes the regression coverage non-deterministic. Update the dummy config generation path used by test_gemma4_e2e_dummy to explicitly assert or patch sliding_window to 512 regardless of shrink_hidden, so the geometry in the E2B case stays stable and the test fails loudly if the real checkpoint config changes.tensorrt_llm/_torch/pyexecutor/kv_cache_manager_v2.py (1)
2778-2783: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument
layer_idxsemantics in the docstring.
get_layer_page_index_scaleis a new cross-file public API (consumed fromflashinfer.py), but the docstring omits anArgs/Returnssection clarifying thatlayer_idxis the global layer index (converted internally viaself.layer_offsets), not the local pool offset. Given this entire PR is about layer-index/scale mismatches, documenting this explicitly would reduce future misuse risk.As per coding guidelines, "Externally called functions should have docstrings, and their arguments should be documented," and "Use Google style docstrings for classes and functions in Python, parseable by Sphinx."
📝 Proposed docstring update
def get_layer_page_index_scale(self, layer_idx: int) -> int: - """Page-index scale of this layer's KV buffer. Layers in one pool can - have different scales (e.g. different head_dim), so per-layer callers - must not use the pool-level scale.""" + """Page-index scale of this layer's KV buffer. + + Layers in one pool can have different scales (e.g. different + head_dim), so per-layer callers must not use the pool-level scale. + + Args: + layer_idx: Global layer index (mapped internally to the local + pool offset via ``self.layer_offsets``). + + Returns: + The KEY buffer's page-index scale for this layer. + """🤖 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/pyexecutor/kv_cache_manager_v2.py` around lines 2778 - 2783, The public method get_layer_page_index_scale currently lacks Google-style argument documentation and does not clarify that layer_idx is the global layer index, not the local pool offset. Update its docstring to include Args and Returns sections, explicitly stating that layer_idx is translated through self.layer_offsets before calling self.impl.get_page_index_scale, so callers in flashinfer.py and other external users do not misuse the API.Source: Coding guidelines
🤖 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/unittest/_torch/modeling/test_gemma4_e2e_dummy.py`:
- Around line 229-266: This test can silently run with TrtllmAttention instead
of FlashInfer, so add an explicit FlashInfer availability guard before
constructing LLM in test_e2e_text_e2b_dummy_small_max_seq_len. Use the existing
availability symbol or decorator pattern already used in tests (for example a
requires_* or skipif check tied to FlashInfer) so the test is skipped when
FlashInfer is unavailable. Keep the current attn_backend="FLASHINFER" setup, but
ensure the test only executes the intended FlashInfer VSWA path.
---
Nitpick comments:
In `@tensorrt_llm/_torch/pyexecutor/kv_cache_manager_v2.py`:
- Around line 2778-2783: The public method get_layer_page_index_scale currently
lacks Google-style argument documentation and does not clarify that layer_idx is
the global layer index, not the local pool offset. Update its docstring to
include Args and Returns sections, explicitly stating that layer_idx is
translated through self.layer_offsets before calling
self.impl.get_page_index_scale, so callers in flashinfer.py and other external
users do not misuse the API.
In `@tests/unittest/_torch/modeling/test_gemma4_e2e_dummy.py`:
- Around line 245-254: The E2B dummy test setup in _make_dummy_config_dir
currently relies on the upstream config.json to leave sliding_window at 512 when
shrink_hidden is false, which makes the regression coverage non-deterministic.
Update the dummy config generation path used by test_gemma4_e2e_dummy to
explicitly assert or patch sliding_window to 512 regardless of shrink_hidden, so
the geometry in the E2B case stays stable and the test fails loudly if the real
checkpoint config changes.
🪄 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: 63582079-4513-4ee8-91a5-898772f82e45
📒 Files selected for processing (4)
tensorrt_llm/_torch/attention_backend/flashinfer.pytensorrt_llm/_torch/pyexecutor/kv_cache_manager_v2.pytests/integration/test_lists/test-db/l0_b200.ymltests/unittest/_torch/modeling/test_gemma4_e2e_dummy.py
|
/bot run --disable-fail-fast |
|
PR_Github #58142 [ run ] triggered by Bot. Commit: |
|
PR_Github #58142 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #58223 [ run ] triggered by Bot. Commit: |
|
PR_Github #58223 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #58235 [ run ] triggered by Bot. Commit: |
|
PR_Github #58235 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #58348 [ run ] triggered by Bot. Commit: |
|
PR_Github #58348 [ run ] completed with state |
|
/bot run --disable-fail-fast |
1 similar comment
|
/bot run --disable-fail-fast |
|
PR_Github #58378 [ run ] triggered by Bot. Commit: |
|
PR_Github #58378 [ run ] completed with state |
|
[by Codex] @lowsfer Could you review this PR? Thanks! |
There was a problem hiding this comment.
Fix looks good to me.
A further round-up will be to have the grouping of (slot_group, index_scale) built inside the manager itself, building the partition where index_scales is built. A more schematic approach will be (as Yao mentioned to me in chat), a page index converter from the manager that adapts regarding known models.
For now the fix is enough to unblock the IMA in the FlashInfer backend. A core fix will free us from backend-local mappings (for example TRTLLM will fall for the same IMA, although not reachable right now for Gemma4 since the backend is not supported yet).
|
/bot run --disable-fail-fast |
👎 Promotion blocked, new vulnerability foundVulnerability report
|
006a92e to
991459c
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #59206 [ run ] triggered by Bot. Commit: |
|
PR_Github #59206 [ run ] completed with state
|
…rrect model root - kv_cache_manager_v2: get_batch_cache_indices_flat now uses the per-layer page-index scale when layer_idx is given (matches get_batch_cache_indices). prepare() builds VSWA pool block tables through this flat helper, so the earlier fix to the non-flat path alone left the trtllm-gen decode path still scaling shared-pool layers with the pool-level scale -> out-of-range page ids -> CUDA_ERROR_ILLEGAL_ADDRESS during warmup prefill. - test_gemma4_e2e_dummy: use the canonical model root subdir 'gemma' (per tests/test_common/llm_data.py) instead of 'gemma4', so the E2E tests actually find the checkpoints instead of silently skipping. Signed-off-by: Tianrui Hu <tianruih@nvidia.com>
991459c to
f80bb64
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #59347 [ run ] triggered by Bot. Commit: |
|
PR_Github #59347 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #59385 [ run ] triggered by Bot. Commit: |
|
PR_Github #59385 [ run ] completed with state |
Commit 9115e48 (NVIDIA#16099) added an index_scale keyword argument to KVCacheManagerV2._get_batch_cache_indices_by_pool_id and passes it explicitly from get_batch_cache_indices, so the MiniMax M3 override must accept it. M3 bypasses the V1 block-id conversion by design (its forward path indexes paged views directly by slot id), so the caller-supplied scale is ignored alongside index_scales. Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
Commit 9115e48 (NVIDIA#16099) added an index_scale keyword argument to KVCacheManagerV2._get_batch_cache_indices_by_pool_id and passes it explicitly from get_batch_cache_indices, so the MiniMax M3 override must accept it. M3 bypasses the V1 block-id conversion by design (its forward path indexes paged views directly by slot id), so the caller-supplied scale is ignored alongside index_scales. Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
Commit 9115e48 (NVIDIA#16099) added an index_scale keyword argument to KVCacheManagerV2._get_batch_cache_indices_by_pool_id and passes it explicitly from get_batch_cache_indices, so the MiniMax M3 override must accept it. M3 bypasses the V1 block-id conversion by design (its forward path indexes paged views directly by slot id), so the caller-supplied scale is ignored alongside index_scales. Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
Commit 9115e48 (NVIDIA#16099) added an index_scale keyword argument to KVCacheManagerV2._get_batch_cache_indices_by_pool_id and passes it explicitly from get_batch_cache_indices, so the MiniMax M3 override must accept it. M3 bypasses the V1 block-id conversion by design (its forward path indexes paged views directly by slot id), so the caller-supplied scale is ignored alongside index_scales. Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
Commit 9115e48 (NVIDIA#16099) added an index_scale keyword argument to KVCacheManagerV2._get_batch_cache_indices_by_pool_id and passes it explicitly from get_batch_cache_indices, so the MiniMax M3 override must accept it. M3 bypasses the V1 block-id conversion by design (its forward path indexes paged views directly by slot id), so the caller-supplied scale is ignored alongside index_scales. Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
Commit 9115e48 (NVIDIA#16099) added an index_scale keyword argument to KVCacheManagerV2._get_batch_cache_indices_by_pool_id and passes it explicitly from get_batch_cache_indices, so the MiniMax M3 override must accept it. M3 bypasses the V1 block-id conversion by design (its forward path indexes paged views directly by slot id), so the caller-supplied scale is ignored alongside index_scales. Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
Commit 9115e48 (NVIDIA#16099) added an index_scale keyword argument to KVCacheManagerV2._get_batch_cache_indices_by_pool_id and passes it explicitly from get_batch_cache_indices, so the MiniMax M3 override must accept it. M3 bypasses the V1 block-id conversion by design (its forward path indexes paged views directly by slot id), so the caller-supplied scale is ignored alongside index_scales. Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
Commit 9115e48 (NVIDIA#16099) added an index_scale keyword argument to KVCacheManagerV2._get_batch_cache_indices_by_pool_id and passes it explicitly from get_batch_cache_indices, so the MiniMax M3 override must accept it. M3 bypasses the V1 block-id conversion by design (its forward path indexes paged views directly by slot id), so the caller-supplied scale is ignored alongside index_scales. Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
Commit 9115e48 (NVIDIA#16099) added an index_scale keyword argument to KVCacheManagerV2._get_batch_cache_indices_by_pool_id and passes it explicitly from get_batch_cache_indices, so the MiniMax M3 override must accept it. M3 bypasses the V1 block-id conversion by design (its forward path indexes paged views directly by slot id), so the caller-supplied scale is ignored alongside index_scales. Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
Commit 9115e48 (NVIDIA#16099) added an index_scale keyword argument to KVCacheManagerV2._get_batch_cache_indices_by_pool_id and passes it explicitly from get_batch_cache_indices, so the MiniMax M3 override must accept it. M3 bypasses the V1 block-id conversion by design (its forward path indexes paged views directly by slot id), so the caller-supplied scale is ignored alongside index_scales. Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
Commit 9115e48 (NVIDIA#16099) added an index_scale keyword argument to KVCacheManagerV2._get_batch_cache_indices_by_pool_id and passes it explicitly from get_batch_cache_indices, so the MiniMax M3 override must accept it. M3 bypasses the V1 block-id conversion by design (its forward path indexes paged views directly by slot id), so the caller-supplied scale is ignored alongside index_scales. Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
Commit 9115e48 (NVIDIA#16099) added an index_scale keyword argument to KVCacheManagerV2._get_batch_cache_indices_by_pool_id and passes it explicitly from get_batch_cache_indices, so the MiniMax M3 override must accept it. M3 bypasses the V1 block-id conversion by design (its forward path indexes paged views directly by slot id), so the caller-supplied scale is ignored alongside index_scales. Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
Commit 9115e48 (NVIDIA#16099) added an index_scale keyword argument to KVCacheManagerV2._get_batch_cache_indices_by_pool_id and passes it explicitly from get_batch_cache_indices, so the MiniMax M3 override must accept it. M3 bypasses the V1 block-id conversion by design (its forward path indexes paged views directly by slot id), so the caller-supplied scale is ignored alongside index_scales. Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
Commit 9115e48 (NVIDIA#16099) added an index_scale keyword argument to KVCacheManagerV2._get_batch_cache_indices_by_pool_id and passes it explicitly from get_batch_cache_indices, so the MiniMax M3 override must accept it. M3 bypasses the V1 block-id conversion by design (its forward path indexes paged views directly by slot id), so the caller-supplied scale is ignored alongside index_scales. Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
Description
Symptom
Every Gemma4 checkpoint (E2B / E4B / 12B / 26B-A4B / 31B) crashes with
CUDA_ERROR_ILLEGAL_ADDRESSduring the warmup prefill on the FlashInferattention backend whenever
max_seq_lenis at most the sliding windowsize (512), e.g.
max_seq_len=256. Default (large)max_seq_lenisunaffected, which is why regular deployments and accuracy runs never hit
it. This is not a recent regression: it reproduces on months-old builds.
Root cause
The KV cache manager (
KVCacheManagerV2) allocates one slot per tokenblock covering every layer's page, and each layer converts the shared
slot number into a page index of its own buffer with a per-layer
multiplier ("page-index scale"). Layers whose pages have different byte
sizes have different scales: for gemma-4-E2B, the 28 sliding-attention
layers (head_dim 256) have scale 56 while the 7 global-attention layers
(head_dim 512) have scale 14. The core documents that computed page
indices may only be shared between buffers with the same scale
(
get_page_index_scale).When
max_seq_lenis at most the sliding window size, every attentionwindow is clamped to
max_seq_len, so all layers land in a single KVpool. The wrapper, however, kept one scale per pool (taken from the
pool's first layer) and shared one converted page-index list across all
layers. Global-attention layers therefore received page ids up to 4x
past the end of their buffer, and
append_paged_kv_cachewrote out ofbounds. With larger
max_seq_lenthe sliding and global layers sit inseparate pools, each pool is homogeneous, and the single per-pool scale
happens to be correct — which is why only small
max_seq_lencrashes.Fix (Python only; no changes to pool grouping, memory pools, or C++)
KVCacheManagerV2.get_batch_cache_indices(layer_idx=...)now convertswith that layer's own page-index scale (new helper
get_layer_page_index_scale), matching the core's documented contract.FlashInferAttentionMetadatanow shares one page-index list only amonglayers with the same pool and the same scale, and engages the per-pool
index swap when windows differ (VSWA) or scales differ (previously
VSWA only).
Test Coverage
tests/unittest/_torch/modeling/test_gemma4_e2e_dummy.py::test_e2e_text_e2b_dummy_small_max_seq_len[256|512](real gemma-4-E2B geometry, dummy weights). It fails with the illegal
memory access before this fix and passes with it. Registered in the
B200 pre-merge list (
l0_b200.yml).max_seq_len64 (batch 8), 256, and 512 crashed before the fix andpass with it; 768 and 2048 (above the window size) pass before and
after (no regression). Generated text is coherent in all passing runs.
Summary by CodeRabbit
Bug Fixes
Tests