[TRTLLM-12721][feat] Add disagg transfer state consensus - #15139
Conversation
|
/bot run --disable-fail-fast |
|
PR_Github #52963 [ run ] triggered by Bot. Commit: |
|
PR_Github #52963 [ run ] completed with state
|
5115366 to
27b39f2
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #53107 [ run ] triggered by Bot. Commit: |
6a495f4 to
61dc211
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #53177 [ run ] triggered by Bot. Commit: |
📝 WalkthroughWalkthroughCacheTransceiver now implements cross-rank consensus for KV cache transfer outcomes. Local transfer completions and failures are recorded per-request, gathered across MPI/ProcessGroup ranks via new utilities, reduced into consensus sets, and then committed to request states. Both context and generation transfer paths now defer state transitions until after consensus. ChangesKV cache transfer consensus mechanism
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (1)
cpp/tensorrt_llm/batch_manager/cacheTransceiver.cpp (1)
71-75: 💤 Low valueConsider adding an explicit
kPendingor sentinel value to prevent accidental zero-initialization bugs.
TransferConsensusStatestarts at 1, leaving 0 unassigned. If any packed buffer is zero-initialized or truncated, parsing could yield undefined behavior when casting to this enum. AddingkPending = 0or a sentinel would make such cases detectable.♻️ Optional: Add sentinel value
enum class TransferConsensusState : std::uint64_t { + kPending = 0, // Sentinel for uninitialized/invalid state kCompleted = 1, kFailed = 2, };And add a
defaultcase in the switch at line 158-162 to catch unknown values.🤖 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/cacheTransceiver.cpp` around lines 71 - 75, Add an explicit zero-value sentinel to the enum by defining TransferConsensusState::kPending = 0 so zero-initialized or truncated buffers map to a known state, and update the switch that handles TransferConsensusState (the switch covering the consensus handling logic) to include a default branch that treats unknown/invalid enum values robustly (e.g., log error and handle/return/fail gracefully) to make such cases detectable and safe.
🤖 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.
Nitpick comments:
In `@cpp/tensorrt_llm/batch_manager/cacheTransceiver.cpp`:
- Around line 71-75: Add an explicit zero-value sentinel to the enum by defining
TransferConsensusState::kPending = 0 so zero-initialized or truncated buffers
map to a known state, and update the switch that handles TransferConsensusState
(the switch covering the consensus handling logic) to include a default branch
that treats unknown/invalid enum values robustly (e.g., log error and
handle/return/fail gracefully) to make such cases detectable and safe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 477aa235-789b-4c35-acea-207c25e5b630
📒 Files selected for processing (2)
cpp/include/tensorrt_llm/batch_manager/cacheTransceiver.hcpp/tensorrt_llm/batch_manager/cacheTransceiver.cpp
|
PR_Github #53107 [ run ] completed with state |
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
61dc211 to
019efd9
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #53235 [ run ] triggered by Bot. Commit: |
|
PR_Github #53177 [ run ] completed with state |
|
PR_Github #53235 [ run ] completed with state
|
|
/bot run --stage-list "DGX_H100-PyTorch-2,DGX_B200-8_GPUs-PyTorch-1,DGX_H100-4_GPUs-PyTorch-Others-2,GB10-PyTorch-1" --disable-fail-fast |
|
/bot run --disable-fail-fast --stage-list "DGX_H100-PyTorch-2,DGX_B200-8_GPUs-PyTorch-1,DGX_H100-4_GPUs-PyTorch-Others-2,GB10-PyTorch-1" |
|
PR_Github #53377 [ run ] triggered by Bot. Commit: |
|
PR_Github #53377 [ run ] completed with state
|
|
/bot run --disable-fail-fast --stage-list "DGX_B200-8_GPUs-PyTorch-1" |
1 similar comment
|
/bot run --disable-fail-fast --stage-list "DGX_B200-8_GPUs-PyTorch-1" |
|
PR_Github #53613 [ run ] triggered by Bot. Commit: |
|
PR_Github #53615 [ run ] triggered by Bot. Commit: |
|
PR_Github #53615 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #53674 [ run ] triggered by Bot. Commit: |
|
PR_Github #53674 [ run ] completed with state |
Tabrizian
left a comment
There was a problem hiding this comment.
There is a consensus gathering logic already in cache transceiver. Should we remove it now given what you are adding in this PR?
if ((syncComm) && syncComm->getSize() > 1)
{
auto gatherRequestIdVec = gatherRequestIds(syncComm, contextCompleteRequestIds);
for (auto&& requestId : gatherRequestIdVec)
{
frequencyMap[requestId]++;
}
}
Thanks @Tabrizian ! Yes, this is the cleanup candidate I called out in the PR description: The existing So they may overlap, but they are not obviously identical yet. The Follow-ups mentioned in the PR description is to explicitly track auditing/removing this path, including this context-side gather and the analogous generation-side gather:
I’d like to keep that as a separate cleanup/performance PR so this one stays focused on correctness of terminal-state consensus. |
Description
Adds a V1
CacheTransceiverrequest-state consensus step for disaggregated KV transfer terminal transitions.The implementation records local completed/failed transfer outcomes, packs
{request_id, state}entries, gathers them through the existing communication backend, and applies a deterministic consensus result before mutating request state. Context-side consensus uses the existing TP/CP sync group and then the PP group; generation-side consensus uses the existing generation sync group.This keeps PR#1 scoped to the consensus foundation for safe in-flight cancellation. It does not yet add deadline enforcement, deferred cleanup/quarantine, richer cancellation states, or hard mid-transfer abort semantics; those remain for the follow-up cancellation/lifetime PR.
Dependency graph
Arrows point from prerequisite to dependent. PR numbers in graph nodes are clickable.
graph TD PR15139["<a href='https://github.com/NVIDIA/TensorRT-LLM/pull/15139'>#15139</a>: transfer state consensus (this PR, merged)"] PR15181["<a href='https://github.com/NVIDIA/TensorRT-LLM/pull/15181'>#15181</a>: bounded C++ transfer status polling (inflight)"] PR15356["<a href='https://github.com/NVIDIA/TensorRT-LLM/pull/15356'>#15356</a>: bounded V2 context transfer polling (inflight)"] PR15238["<a href='https://github.com/NVIDIA/TensorRT-LLM/pull/15238'>#15238</a>: in-flight cancellation + buffer poison (draft)"] WORK_BLOCKALL["blockAll / wait-all cancellation (planned)"] WORK_BUFFER["multi-slot buffers + unpoison recovery (planned)"] PR15139 -->|satisfied| PR15238 PR15181 -->|blocking| PR15356 PR15181 -->|blocking| PR15238 PR15356 -->|blocking| PR15238 PR15238 -.->|planned| WORK_BLOCKALL PR15238 -.->|planned| WORK_BUFFER classDef merged fill:#dcfce7,stroke:#16a34a,color:#14532d; classDef inflight fill:#dbeafe,stroke:#2563eb,color:#1e3a8a; classDef draft fill:#ffedd5,stroke:#f97316,color:#7c2d12; classDef current fill:#ede9fe,stroke:#7c3aed,color:#3b0764,stroke-width:3px; classDef downstream fill:#f3f4f6,stroke:#6b7280,color:#374151,stroke-dasharray:5 5; linkStyle 0 stroke:#16a34a,stroke-width:2px; linkStyle 1,2,3 stroke:#ea580c,stroke-width:3px; linkStyle 4,5 stroke:#6b7280,stroke-width:2px,stroke-dasharray:5 5; class PR15139 current; class PR15181,PR15356 inflight; class PR15238 draft; class WORK_BLOCKALL,WORK_BUFFER downstream;Notes
kDISAGG_TRANS_ERRORwithkDISAGG_GENERATION_TRANS_COMPLETE.Follow-ups
checkContextTransferStatus:gatherRequestIds(syncComm, contextCompleteRequestIds)checkGenTransferStatus:gatherRequestIds(syncComm, genTransferReadyRequestIds){request_id, completed/failed}transfer-state consensus fully covers the same cross-rank agreement semantics, remove or fold those ready-ID gathers into the consensus path.Testing
pre-commit run clang-format --files cpp/include/tensorrt_llm/batch_manager/cacheTransceiver.h cpp/tensorrt_llm/batch_manager/cacheTransceiver.cpppre-commit run codespell --files cpp/include/tensorrt_llm/batch_manager/cacheTransceiver.h cpp/tensorrt_llm/batch_manager/cacheTransceiver.cppgit diff --check upstream/main..HEADAttempted full file-scoped pre-commit as well, but local hooks fail in
scripts/check_test_list.pywithTypeError: unsupported operand type(s) for |: 'type' and 'NoneType'under the local hook environment; unrelated C++ hooks passed.Summary by CodeRabbit
Bug Fixes
Refactor