[None][feat] add KV cache reuse probe - #14333
Conversation
📝 WalkthroughWalkthroughRefactored KV cache reuse matching to return a structured ChangesKV Cache Reuse Matching Contract and Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unittest/kv_cache_manager_v2_tests/test_kv_cache_manager_v2.py (1)
517-524: QA list updates look unnecessary here.This PR still looks unit-scope only, so I don’t see a new entry needed under
tests/integration/test_lists/qa/ortest-dbfor this change. The new assertion covers the happy path forprobe_reuse(), but I’d still want direct unit regressions for the two pruning edge cases in_block_radix_tree.pybefore relying on this refactor.
🤖 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 `@tensorrt_llm/runtime/kv_cache_manager_v2/_block_radix_tree.py`:
- Around line 510-517: The current SSM truncation can keep a partially matched
block that has an SSM snapshot, causing _KVCache._setup_for_reuse() to seed
_ssm_blocks from a future state; instead, when scanning matched for the last
block with a snapshot (ssm_lc_id), require that the block is fully matched
before accepting it: in the reversed loop over matched, only set ssm_trunc = i +
1 if matched[i][0].storage[ssm_lc_id] is not None AND the matched entry
indicates a full match (e.g., matched[i][1] equals the block's full token count
such as matched[i][0].num_tokens or matched[i][0].length); otherwise skip that
index so any partially matched tail is dropped, then slice matched =
matched[:ssm_trunc] as before.
- Around line 538-540: The current slice keeps the first missing SWA window
block when n==0, causing probe_reuse() to over-report and later
_setup_for_reuse() to hit unwrap_optional(None); adjust the truncation to drop
that missing block by slicing matched to exclude the found index itself (use
matched[: len(matched) - n - 1] instead of matched[: len(matched) - n]) after
computing n via find_index(reversed(matched[stale_end:]), check_no_page_stale),
ensuring the newest missing block is removed from the reusable prefix so
probe_reuse() and _setup_for_reuse() no longer see an invalid block.
🪄 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: 094c3dea-a403-4c95-ba3e-b900bf4da3a3
📒 Files selected for processing (6)
tensorrt_llm/runtime/kv_cache_manager_v2/__init__.pyitensorrt_llm/runtime/kv_cache_manager_v2/_block_radix_tree.pytensorrt_llm/runtime/kv_cache_manager_v2/_core/_kv_cache.pytensorrt_llm/runtime/kv_cache_manager_v2/_core/_kv_cache_manager.pytests/unittest/kv_cache_manager_v2_tests/test_kv_cache_manager_v2.pytests/unittest/kv_cache_manager_v2_tests/test_kv_cache_salting.py
Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com>
b26561f to
f9968c6
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #49350 [ run ] triggered by Bot. Commit: |
|
PR_Github #49350 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #49628 [ run ] triggered by Bot. Commit: |
|
PR_Github #49628 [ run ] completed with state |
Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com>
Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com>
Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com>
…dings Migrates tensorrt_llm/runtime/kv_cache_manager_v2 from pure Python to a C++ implementation under cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/ with nanobind bindings compiled into bindings.so, preserving the same public API. The dispatcher __init__.py selects the backend via TLLM_KV_CACHE_MANAGER_V2_BACKEND (default "cpp"); both backends pass the shared test suite. CODEOWNERS assigns the new C++ tree to trt-llm-kv-cache-manager-devs. Includes ports of subsequent main features: commit-min-snapshot + SWA-slot reservation, SHA-256 block-key hashing, CUDA-graph request IDs, event manager and stats API to C++, uint64 ReuseScope salt/lora_id, resume-utilization KV constraints, per-conversation KV cache block reuse (PlannedDropHandle), the reuse-probe _KVCache refactor (compute the ReuseMatch once in the manager and pass it into the cache, NVIDIA#14333), SSM-snapshot iteration stats (NVIDIA#16598), the simplified plan_committed_block_drop (db117e6276), and an MPI teardown fix for the unittest/bindings CI shard. Migration planning docs (TODO.md, MIGRATION_PLAN_CPP.md, CPP_MIGRATION_PLAN_MAIN_15633.md) are kept on a separate branch. Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com>
…dings Migrates tensorrt_llm/runtime/kv_cache_manager_v2 from pure Python to a C++ implementation under cpp/tensorrt_llm/batch_manager/kv_cache_manager_v2/ with nanobind bindings compiled into bindings.so, preserving the same public API. The dispatcher __init__.py selects the backend via TLLM_KV_CACHE_MANAGER_V2_BACKEND (default "cpp"); both backends pass the shared test suite. CODEOWNERS assigns the new C++ tree to trt-llm-kv-cache-manager-devs. Includes ports of subsequent main features: commit-min-snapshot + SWA-slot reservation, SHA-256 block-key hashing, CUDA-graph request IDs, event manager and stats API to C++, uint64 ReuseScope salt/lora_id, resume-utilization KV constraints, per-conversation KV cache block reuse (PlannedDropHandle), the reuse-probe _KVCache refactor (compute the ReuseMatch once in the manager and pass it into the cache, NVIDIA#14333), SSM-snapshot iteration stats (NVIDIA#16598), the simplified plan_committed_block_drop (db117e6276), and an MPI teardown fix for the unittest/bindings CI shard. Migration planning docs (TODO.md, MIGRATION_PLAN_CPP.md, CPP_MIGRATION_PLAN_MAIN_15633.md) are kept on a separate branch. Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com>
Summary:
Tests: