[TRTLLM-9920][feat] Add support for arbitrary KVCache transfer - #13055
Conversation
3ecb491 to
bd56626
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #43303 [ run ] triggered by Bot. Commit: |
|
PR_Github #43303 [ run ] completed with state
|
b67e79b to
07a2536
Compare
471fa91 to
5ed9f52
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #44376 [ run ] triggered by Bot. Commit: |
|
PR_Github #44376 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #44473 [ run ] triggered by Bot. Commit: |
|
PR_Github #44473 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #60622 [ run ] completed with state
|
…ss-case expectation Store the sender-side request with one generated token beyond the prompt, matching a completed context request; storeBlocksForReuse drops the trailing token, so without it the final prompt block never enters the reuse tree and the lookup rejects the transfer. Expect the miss case to surface as an exception on the receive future instead of a request state, which is set by the Python layer. Signed-off-by: Iman Tabrizian <10105175+tabrizian@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #60786 [ run ] triggered by Bot. Commit: |
|
PR_Github #60786 [ run ] completed with state
|
…VCacheTransfer Signed-off-by: Iman Tabrizian <10105175+tabrizian@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #60835 [ run ] triggered by Bot. Commit: |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
cpp/tensorrt_llm/executor/cache_transmission/ucx_utils/ucxCacheCommunicator.cpp (1)
614-648: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMagic literal for the polling interval.
std::chrono::milliseconds(100)is a bare magic literal. As per coding guidelines, "Avoid magic literals except0,nullptr,true, andfalse; initialize named constants instead, usingk-prefixed camelCase names."+ auto constexpr kRecvPollIntervalMs = std::chrono::milliseconds(100); + if (!req->isCompleted()) { auto const& terminate = ctx.getTransferTerminate(); - while (future.wait_for(std::chrono::milliseconds(100)) != std::future_status::ready) + while (future.wait_for(kRecvPollIntervalMs) != std::future_status::ready)🤖 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 `@cpp/tensorrt_llm/executor/cache_transmission/ucx_utils/ucxCacheCommunicator.cpp` around lines 614 - 648, Replace the bare 100-millisecond value in the receive polling loop around future.wait_for with a named k-prefixed camelCase duration constant, then use that constant for the polling interval while preserving the existing timeout behavior.Source: Coding guidelines
cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp (1)
2247-2252: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
refreshBlocks()holds the reuse-tree mutex across a blocking transfer sync.
mTransferManager->syncTransfers()is now executed while holdingmLookupTree->getMutex(). Since this PR makes the reuse tree concurrently accessed by transfer threads (pinning/lookup) in addition to the main executor thread, serializing a per-iteration stream-sync call behind this shared lock adds a new contention point on a hot path. Consider releasing the lock before the sync call ifmEvictionPolicy->refresh()doesn't need to be atomic with it.void WindowBlockManager::refreshBlocks() { - std::lock_guard<std::recursive_mutex> lock(mLookupTree->getMutex()); - mEvictionPolicy->refresh(); - mTransferManager->syncTransfers(); + { + std::lock_guard<std::recursive_mutex> lock(mLookupTree->getMutex()); + mEvictionPolicy->refresh(); + } + mTransferManager->syncTransfers(); }🤖 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 `@cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp` around lines 2247 - 2252, Update WindowBlockManager::refreshBlocks() so the lookup-tree mutex is held only while calling mEvictionPolicy->refresh(), then release it before calling mTransferManager->syncTransfers(). Preserve the existing refresh and transfer-sync ordering without holding the tree lock across the potentially blocking synchronization.
🤖 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 `@cpp/tensorrt_llm/batch_manager/dataTransceiver.cpp`:
- Around line 880-894: Guard the mRequestToSession lookup in pinReuseTreeBlocks
by validating that it is not equal to mRequestToSession.end() before accessing
it->second, matching the existing check in sendSyncFromReuseTree; preserve the
current block lookup and return behavior.
- Around line 885-893: Update pinReuseTreeBlocks() around the cache manager
window metadata lookup to proceed only when getWindowSizesMetadata() contains
exactly one window, returning without pinning for multi-window managers.
Preserve the existing single-window lookup and pinnedIds behavior, or use a
window-qualified handle if the surrounding API supports it.
In `@cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp`:
- Around line 3031-3061: Update WindowBlockManager::pinBlocks to skip blocks
where block->isPlaceholder() is true before calling pinBlock. Keep pinBlock
behavior unchanged and continue pinning all non-placeholder entries in
allocatedBlocks.
---
Nitpick comments:
In `@cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp`:
- Around line 2247-2252: Update WindowBlockManager::refreshBlocks() so the
lookup-tree mutex is held only while calling mEvictionPolicy->refresh(), then
release it before calling mTransferManager->syncTransfers(). Preserve the
existing refresh and transfer-sync ordering without holding the tree lock across
the potentially blocking synchronization.
In
`@cpp/tensorrt_llm/executor/cache_transmission/ucx_utils/ucxCacheCommunicator.cpp`:
- Around line 614-648: Replace the bare 100-millisecond value in the receive
polling loop around future.wait_for with a named k-prefixed camelCase duration
constant, then use that constant for the polling interval while preserving the
existing timeout behavior.
🪄 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: d0ef3292-f06f-4b64-bc7d-65d2f64ebd65
📒 Files selected for processing (13)
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/cacheFormatter.cppcpp/tensorrt_llm/batch_manager/cacheFormatter.hcpp/tensorrt_llm/batch_manager/cacheTransceiver.cppcpp/tensorrt_llm/batch_manager/dataTransceiver.cppcpp/tensorrt_llm/batch_manager/dataTransceiver.hcpp/tensorrt_llm/batch_manager/kvCacheManager.cppcpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cppcpp/tensorrt_llm/batch_manager/rnnCacheFormatter.cppcpp/tensorrt_llm/executor/cache_transmission/ucx_utils/ucxCacheCommunicator.cppcpp/tensorrt_llm/executor/serialization.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- cpp/tensorrt_llm/batch_manager/cacheFormatter.h
- cpp/include/tensorrt_llm/batch_manager/cacheTransceiver.h
- cpp/tensorrt_llm/batch_manager/rnnCacheFormatter.cpp
- cpp/tensorrt_llm/batch_manager/cacheFormatter.cpp
|
PR_Github #60835 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #61100 [ run ] triggered by Bot. Commit: |
|
PR_Github #61100 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #61328 [ run ] triggered by Bot. Commit: |
|
PR_Github #61328 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #61429 [ run ] triggered by Bot. Commit: |
|
PR_Github #61429 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #61462 [ run ] triggered by Bot. Commit: |
Shixiaowei02
left a comment
There was a problem hiding this comment.
Today, we effectively have five tools available:
- Session affinity
- Conditional disaggregation
- P2P direct KV transfer (this PR)
- KV offload
- Recomputation (re-prefill)
Roughly speaking:
- Short follow-up turns: (1)
- Tool-oriented agentic workflows (short generation + long tool output): (3)
- Shared system prefixes: (1) + (4)
- Heavy agentic workloads (long generation + large inputs): (2)
- Multi-user shared generation: (4)
- KV not yet committed: (4) or (5)
As the framework evolves, we now have a broader set of tools to address different requirements. For the agentic workflow scenario above, P2P KV transfer seems like a viable approach. While extending the current transceiver model could enable additional use cases, it may also be worth balancing that flexibility with keeping the underlying design simple and maintainable. We can certainly explore this direction in v1. For v2, my current view is that it may fit better as a dedicated abstraction or module rather than an extension of the existing transfer model.
Thanks for your efforts!
|
PR_Github #61462 [ run ] completed with state |
…A#13055) Signed-off-by: Iman Tabrizian <10105175+tabrizian@users.noreply.github.com>
Dev Engineer Review
get_data_transceiver_state().TRTLLM-13970.QA Engineer Review
Added:
test_arbitrary_kv_cache_transfertest_arbitrary_kv_cache_transfer_missing_blocksBoth are included in
tests/integration/test_lists/test-db/l0_a10.ymlfor CI coverage. The tests cover successful transfer, cache reuse, missing sender blocks, error handling, and continued worker operation.Verdict: sufficient
Description
Adds support for arbitrary KV cache transfer between TRT-LLM workers: a generation worker can request KV cache blocks directly from another worker's reuse tree, without requiring a preceding
context_onlyrequest on the sender.Use cases:
Process overview
sequenceDiagram participant Client participant W0 as Worker 0 (Sender) participant W1 as Worker 1 (Receiver) Note over W0: Normal request fills<br/>KV cache reuse tree Client->>W0: generate("What is...?") W0-->>Client: output tokens + KV stored in reuse tree Note over Client: Client queries<br/>DataTransceiverState from W0 Client->>W0: llm.get_data_transceiver_state() W0-->>Client: opaque_state (bytes) Note over Client,W1: Arbitrary KV transfer via generation_only Client->>W1: generate(prompt, generation_only,<br/>opaque_state=state) W1->>W0: requestAndReceive (RequestInfo) W0->>W0: BlockRange::fromReuseTree(<br/>pinBlocks=true) W0->>W0: pinBlocksById (incRef + claimBlock) W0->>W1: send KV cache blocks W0->>W0: unpinBlocksById (decRef) W1-->>Client: generated tokensKey implementation points
LLM.get_data_transceiver_state()— new API that returns the serializedCacheState + CommState(connection + layout info) needed by the receiver.CacheFormatter::getBlockRangeForSending()— when noLlmRequestis provided, the blocks are looked up viaBlockRange::fromReuseTree()using the token hashes the receiver has declared.fromReuseTree(pinBlocks=true)increments the ref count (and claims the block from the eviction policy if it had no refs) to prevent the sender's reuse tree from evicting the blocks mid-transfer.sendSyncFromReuseTree()unpins after the send completes.Test Coverage
Integration test in
tests/integration/defs/disaggregated/test_disaggregated_single_gpu.py::test_arbitrary_kv_cache_transfer(registered inl0_a10.yml):data_transceiver_statefrom worker 0generation_onlywithopaque_state+max_tokens=1→ asserts KV cache was delivered (no prefill recompute path)cached_tokens > 0)cached_tokens ≤ 1(only BOS matches)PR Checklist
PR description clearly explains what and why.
PR follows TRT-LLM coding guidelines.
Test cases provided for new code paths.
No new dependencies.
Reviewers assigned.
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.