[None][perf] Allocate DSA indexer k-cache only for layers that own an indexer - #16558
[None][perf] Allocate DSA indexer k-cache only for layers that own an indexer#16558Tabrizian wants to merge 4 commits into
Conversation
bd467f8 to
3bc287d
Compare
|
/bot run --disable-fail-fast |
3bc287d to
d10f1ce
Compare
|
/bot run --disable-fail-fast --add-multi-gpu-test |
|
PR_Github #61600 [ run ] triggered by Bot. Commit: |
|
PR_Github #61600 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #61650 [ run ] triggered by Bot. Commit: |
|
PR_Github #61650 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #61660 [ run ] triggered by Bot. Commit: |
|
PR_Github #61660 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #61676 [ run ] triggered by Bot. Commit: |
|
PR_Github #61676 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
… indexer (KVCacheManager V1) GLM 5.2 (glm_moe_dsa) uses DeepSeek Sparse Attention with cross-layer indexer sharing: only 21 of 78 layers own an indexer; the rest reuse the previous full layer's top-k and never touch the indexer k-cache. The V1 KVCacheManager allocated the indexer k-cache stride for every layer, wasting 13.6% of the KV cache footprint (55,224 -> 47,700 B/token, +15.8% effective KV token capacity per rank). Changes: - C++: add an optional per-local-layer indexerKCacheLayerMask to the KVCacheManager/BlockManager/WindowBlockManager ctors; createIndexerKCachePools now builds an indexer pool with one row per masked-in layer (rows follow the KV pool's layer order) and records the layer -> pool-row map (getIndexerKCachePoolLayerIdx). - nanobind: new indexer_k_cache_layer_mask ctor kwarg; get_indexer_k_cache_pool_data translates local layer -> pool row and rejects masked-out layers; get_indexer_k_cache_pool rejects a null pool. - Python: DSACacheManager derives the mask from to_sparse_params(pretrained_config, layer_idx).is_full_indexer_layer (the same source of truth MLA uses to construct Indexer modules, including trailing spec/MTP layers, which are always full), stores None pool entries for shared layers, and asserts if a shared layer requests indexer buffers. Both estimators (get_cache_size_per_token / get_cache_bytes_per_token) charge indexer bytes for full-indexer layers only, PP-sliced consistently. - Disagg is gated with clear fail-fast errors (C++ cache transceiver and Python NIXL extractor) until the transfer layouts understand the per-layer masked pool; dense models (DeepSeek V3.2) are unaffected. - Tests: two C++ pool-shape tests covering the masked and dense-default indexer pool layouts. Signed-off-by: Iman Tabrizian <10105175+tabrizian@users.noreply.github.com>
… the C++ cache transceiver Follow-up to the per-layer masked indexer k-cache pool: ctx->gen disaggregated transfer of the indexer k-cache now understands the masked layout (only full-indexer layers own a pool row, e.g. GLM 5.2 cross-layer indexer sharing) instead of failing fast. - CacheState carries per-PP indexer layer counts (indexerLayerNumPerPP; empty = dense fallback to the attention counts), wired through the ctors, operator==, toString, and serialization. - New targetIRanksForIndexerKCache keeps the attention pass's rank topology but recomputes per-peer layer counts in indexer layer space (interval intersections of the per-PP indexer prefix sums). splitKVCache/concatKVCache use it, plus the self indexer count as the kernel layer count, when transferring the indexer pool. - MLACacheFormatter sizes indexer-pass buffers with the indexer counts, skips zero-sized receive targets, and inquireSupport validates indexer layout equality, equal indexer layer totals, and rejects PP partitions that would produce zero-sized indexer exchanges (rank or overlap without a full-indexer layer). - The NIXL/Mooncake agent path computes a dedicated indexer-pass write-offset ratio in indexer layer space (previously shared the attention-space KV ratio, which would corrupt masked-pool transfers under PP resharding). - CacheTransceiver takes indexerLayerNumPerPP, validates it against the local pool row count, and the Python transceiver gathers it over PP from the manager's indexer layer mask. - CacheTransBufferManager sizes the indexer transfer buffer from the pool's actual layer count. - Tests: CacheState serialization round-trip of the new field (serializeUtilsTest) and a multi-GPU indexer-layer-mask transceiver test with ctx/gen PP resharding (cacheTransceiverTest). Also fixes a positional off-by-one in the transceiver test's CacheState construction (isIndexerKCache previously landed on enablePartialReuse). Signed-off-by: Iman Tabrizian <10105175+tabrizian@users.noreply.github.com>
…transceiver The Python KV transceiver runtime does not support the per-layer masked DSA indexer k-cache pool yet, so switch TestGLM52NVFP4::test_nvfp4_nixl_python to the C++ NIXL transceiver (renamed to test_nvfp4_nixl), exercising ctx->gen transfer of the masked indexer pool end to end (MTP draft layer included). The l0_dgx_b200 and QA list entries are updated to the new test id. Signed-off-by: Iman Tabrizian <10105175+tabrizian@users.noreply.github.com>
…gs/5838199) The full cacheTransceiverTest binary hangs under 8-process UCX due to a pre-existing cumulative resource issue in the transceiver test suite, independent of this PR: on an H100 node the same binary hangs at a non-indexer base case on both this branch and main (09e5d0c). This mirrors the already-waived sibling variant test_cache_transceiver[8proc-mooncake_kvcache-90] and is tracked by the same NVBug. Signed-off-by: Iman Tabrizian <10105175+tabrizian@users.noreply.github.com>
643f133 to
81c0712
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #61989 [ run ] triggered by Bot. Commit: |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tensorrt_llm/_torch/attention_backend/sparse/dsa.py (1)
3370-3379:⚠️ Potential issue | 🟠 MajorPreviously reported indexer accounting bugs remain.
The
num_layersbranch still counts every layer, andget_cache_bytes_per_tokenstill sums KV heads instead of one row per owning layer. Masked DSA layouts therefore overestimate indexer bytes and understate capacity. This is the same unresolved issue as the previous review comments.Also applies to: 3414-3424
🤖 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 3370 - 3379, The indexer accounting still overestimates masked DSA memory. Update the num_layers handling near num_indexer_layers and get_cache_bytes_per_token so only indexer-owning layers are counted, with one indexer row per owning layer rather than summing KV heads; preserve the existing layer mapping and full-indexer detection logic for both branches.
🧹 Nitpick comments (1)
tensorrt_llm/_torch/attention_backend/sparse/dsa.py (1)
3193-3205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate the new helper interface.
pretrained_configis untyped and the return annotation usesList[bool]; use the repository’s precise config type andlist[bool], and add a short Google-style docstring describing the per-layer mask and dense fallback. As per coding guidelines, every function must be annotated and built-in generics are preferred; repository learnings confirm Python 3.10+ support.🤖 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 3193 - 3205, Update derive_indexer_k_cache_layer_mask to type pretrained_config with the repository’s precise configuration type and change the return annotation to list[bool]. Add a concise Google-style docstring describing that it returns a per-layer indexer mask and uses dense layers as the fallback.Sources: Coding guidelines, Learnings
🤖 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.
Duplicate comments:
In `@tensorrt_llm/_torch/attention_backend/sparse/dsa.py`:
- Around line 3370-3379: The indexer accounting still overestimates masked DSA
memory. Update the num_layers handling near num_indexer_layers and
get_cache_bytes_per_token so only indexer-owning layers are counted, with one
indexer row per owning layer rather than summing KV heads; preserve the existing
layer mapping and full-indexer detection logic for both branches.
---
Nitpick comments:
In `@tensorrt_llm/_torch/attention_backend/sparse/dsa.py`:
- Around line 3193-3205: Update derive_indexer_k_cache_layer_mask to type
pretrained_config with the repository’s precise configuration type and change
the return annotation to list[bool]. Add a concise Google-style docstring
describing that it returns a per-layer indexer mask and uses dense layers as the
fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9ecbb79c-929f-42c7-ac62-ccbe0ff9f94c
📒 Files selected for processing (18)
cpp/include/tensorrt_llm/batch_manager/cacheTransceiver.hcpp/include/tensorrt_llm/batch_manager/kvCacheManager.hcpp/include/tensorrt_llm/executor/dataTransceiverState.hcpp/tensorrt_llm/batch_manager/cacheTransBuffer.cppcpp/tensorrt_llm/batch_manager/cacheTransceiver.cppcpp/tensorrt_llm/batch_manager/kvCacheManager.cppcpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cppcpp/tensorrt_llm/executor/cache_transmission/agent_utils/connection.cppcpp/tensorrt_llm/executor/cache_transmission/cacheSplitConcat.cucpp/tensorrt_llm/executor/cache_transmission/cacheSplitConcat.hcpp/tensorrt_llm/executor/serialization.cppcpp/tensorrt_llm/nanobind/batch_manager/cacheTransceiver.cppcpp/tensorrt_llm/nanobind/batch_manager/kvCacheManager.cppcpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cppcpp/tests/unit_tests/executor/serializeUtilsTest.cppcpp/tests/unit_tests/multi_gpu/cacheTransceiverTest.cpptensorrt_llm/_torch/attention_backend/sparse/dsa.pytensorrt_llm/_torch/disaggregation/resource/kv_extractor.py
🚧 Files skipped from review as they are similar to previous changes (15)
- cpp/include/tensorrt_llm/batch_manager/cacheTransceiver.h
- cpp/tests/unit_tests/executor/serializeUtilsTest.cpp
- cpp/tensorrt_llm/executor/cache_transmission/cacheSplitConcat.h
- cpp/tensorrt_llm/nanobind/batch_manager/cacheTransceiver.cpp
- cpp/tensorrt_llm/executor/cache_transmission/agent_utils/connection.cpp
- cpp/tensorrt_llm/executor/cache_transmission/cacheSplitConcat.cu
- cpp/tensorrt_llm/batch_manager/cacheTransBuffer.cpp
- cpp/tensorrt_llm/nanobind/batch_manager/kvCacheManager.cpp
- cpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cpp
- cpp/include/tensorrt_llm/executor/dataTransceiverState.h
- cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp
- cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h
- cpp/tensorrt_llm/batch_manager/cacheTransceiver.cpp
- cpp/tests/unit_tests/multi_gpu/cacheTransceiverTest.cpp
- cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp
|
PR_Github #61989 [ run ] completed with state
|
Dev Engineer Review
getIndexerKCachePoolLayerIdx), and enforces rejection of masked-out layers/buffers on the consumer side (C++ and Python).indexerKCacheLayerMask/indexerLayerNumPerPPthrough KV cache manager construction,CacheTransceiver, andkv_cache::CacheState(including equality +toString()updates).targetIRanksForIndexerKCache) and buffer-kind offset-ratio handling forBufferKind::kKV_INDEXER.kv_cache::CacheStateserialization/deserialization withindexerLayerNumPerPP.indexer_k_cache_layer_maskand expose layer→pool-row mapping with masking enforcement.Nonepools and access is blocked.QA Engineer Review
Test-code changes (files outside
tests/integration/test_lists/)cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cppcpp/tests/unit_tests/executor/serializeUtilsTest.cppCacheStateIndexerKCacheserialization/deserialization round-trip to includeindexerKCacheUseFp4andindexerLayerNumPerPP.cpp/tests/unit_tests/multi_gpu/cacheTransceiverTest.cppAsymmetricalCacheTestWithIndexerLayerMaskand updated Indexer-K block-data generation/verification to use indexer-layer counts.Coverage in
tests/integration/test_lists/: these are unit-test binary changes (not directly represented in the integration test list files).Verdict (unit tests): sufficient.
Test-list / integration changes (files under
tests/integration/test_lists/)tests/integration/defs/accuracy/test_disaggregated_serving.pyTestGLM52NVFP4.test_nvfp4_nixl_python[...]→TestGLM52NVFP4.test_nvfp4_nixl[...].tests/integration/test_lists/qa/llm_function_core.txtcache_mgr_v1to point totest_nvfp4_nixl[...]instead oftest_nvfp4_nixl_python[...].tests/integration/test_lists/test-db/l0_dgx_b200.ymlcache_mgr_v1similarly to usetest_nvfp4_nixl[...](timeout unchanged).tests/integration/test_lists/waives.txtcpp/test_multi_gpu.py::test_cache_transceiverwith8proc-ucx_kvcache-90, referencing NVBUG 5838199.Verdict (integration lists): needs follow-up (CI failures were reported for specific helper commit runs in the PR context; additionally, CBTS coverage sufficiency for the updated list entries is not confirmed here).
Overall QA verdict
needs follow-up due to the reported L0 pipeline failures and to validate end-to-end stability across the affected masked Indexer-K transceiver/PP-resharding and Python NIXL paths.
Description
GLM 5.2 (
glm_moe_dsa, served via the DeepSeek-V3 path) uses DeepSeek Sparse Attention with cross-layer indexer sharing: only 21 of 78 layers own an indexer ("full" layers); the remaining 57 "shared" layers reuse the previous full layer's top-k indices and never touch the indexer k-cache. The V1KVCacheManagerallocated the indexer k-cache stride for every layer, so the shared layers' indexer pools were dead memory — 13.6% of the whole KV cache footprint (55,224 B/token instead of 47,700 B/token, i.e. +15.8% effective KV token capacity per rank once removed). On long-context agentic workloads with block reuse, KV capacity directly drives cache hit rate and prefill throughput.Commit 1 — per-layer masked indexer k-cache pool (KVCacheManager V1):
indexerKCacheLayerMaskthrough theKVCacheManager/BlockManager/WindowBlockManagerctors;createIndexerKCachePoolsnow allocates one pool row per masked-in layer (rows follow the KV pool's layer order) and records the layer → pool-row map (getIndexerKCachePoolLayerIdx).indexer_k_cache_layer_maskctor kwarg;get_indexer_k_cache_pool_datatranslates local layer → pool row and rejects masked-out layers.DSACacheManagerderives the mask fromto_sparse_params(pretrained_config, layer_idx).is_full_indexer_layer— the exact source of truth MLA uses to decide whether a layer constructs anIndexermodule (trailing spec/MTP layers always resolve to full). Shared layers getNonepool entries and assert if asked for indexer buffers.get_cache_size_per_token/get_cache_bytes_per_token) charge indexer bytes for full-indexer layers only, PP-sliced consistently with the pool.kv_cache_manager_v2.pyis untouched.NotImplementedError(its FLAT pool-view contract assumes one row per layer);TestGLM52NVFP4::test_nvfp4_nixl_pythonis switched to the C++ NIXL transceiver and renamedtest_nvfp4_nixl(CI list entries updated).Commit 2 — C++ cache transceiver support for the masked pool:
CacheStatecarries per-PP indexer layer counts (indexerLayerNumPerPP; empty = dense fallback to the attention counts), wired through the ctors,operator==,toString, and serialization.targetIRanksForIndexerKCachekeeps the attention pass's rank topology (same connection set) but recomputes per-peer layer counts in indexer layer space (interval intersections of the per-PP indexer prefix sums).splitKVCache/concatKVCacheuse it — plus the self indexer count as the kernel layer count — for the indexer pass.MLACacheFormattersizes indexer-pass buffers with the indexer counts;inquireSupportvalidates indexer layout equality and equal totals, and rejects PP partitions that would produce zero-sized indexer exchanges (a rank or layer overlap without a full-indexer layer).CacheTransceivertakesindexerLayerNumPerPP, validates it against the local pool row count; the Python transceiver gathers it over PP from the manager's indexer layer mask.CacheTransBufferManagersizes the indexer transfer buffer from the pool's actual (masked) layer count.Expected GLM 5.2 server-start accounting:
kv size per token is 47700 bytes/token(was 55,224) and ~+15.8%max tokens in paged KV cacheat the samekv_cache_free_gpu_memory_fraction. Shared layers never used the pools, so model outputs are bitwise-identical.Test Coverage
New tests (all passing on H100):
cpp/tests/unit_tests/batch_manager/kvCacheManagerTest.cpp—IndexerKCachePoolLayerMaskTest(masked pool shape, row mapping, block size) andIndexerKCachePoolDenseDefaultTest(dense layout unchanged without a mask).cpp/tests/unit_tests/executor/serializeUtilsTest.cpp—CacheStateround-trip includingindexerLayerNumPerPP.cpp/tests/unit_tests/multi_gpu/cacheTransceiverTest.cpp— a newAsymmetricalCacheTestWithIndexerLayerMaskend-to-end masked-indexer transfer with ctx/gen PP resharding (also fixes a positional off-by-one in the existingCacheStateconstruction whereisIndexerKCachelanded onenablePartialReuse).Regression: full
kvCacheManagerTestandserializeUtilsTestsuites pass; existing DSA indexer scatter/roundtrip GPU tests pass (dense pool → identity mapping). End-to-end guard:TestGLM52::test_nvfp4(GSM8K) exercises the masked pool implicitly;TestDeepSeekV32covers the dense path.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.