[TRTLLM-12721][fix] Bound disagg transfer status polling#15181
[TRTLLM-12721][fix] Bound disagg transfer status polling#15181chienchunhung wants to merge 6 commits into
Conversation
b5e32ac to
09121fb
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #53178 [ run ] triggered by Bot. Commit: |
09121fb to
7e90bda
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #53236 [ run ] triggered by Bot. Commit: |
|
PR_Github #53178 [ run ] completed with state |
|
PR_Github #53236 [ run ] completed with state
|
2517dff to
7d71ebd
Compare
|
/bot run --disable-fail-fast |
📝 WalkthroughWalkthroughThis PR converts KV cache transfer status checks from blocking single ChangesKV Cache Transfer Polling and Timeout Tracking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
🧹 Nitpick comments (1)
tests/unittest/others/test_kv_cache_transceiver.py (1)
61-73: 💤 Low valueConsider adding a docstring to the polling helper.
The
wait_for_transfer_completionhelper is used by multiple tests and would benefit from a brief docstring explaining its parameters (poll_fn,is_done_fn,timeout_s) and behavior (polls repeatedly until completion or timeout).📝 Suggested docstring
def wait_for_transfer_completion(poll_fn, is_done_fn, timeout_s=DEFAULT_KV_TRANSFER_TIMEOUT_S + KV_TRANSFER_COMPLETION_MARGIN_S): + """Poll until KV cache transfer completes or timeout expires. + + Args: + poll_fn: Callable that polls transfer status (e.g., check_context_transfer_status). + is_done_fn: Callable that returns True when transfer is complete. + timeout_s: Maximum time to wait before asserting failure. + """ deadline = time.monotonic() + timeout_s🤖 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/others/test_kv_cache_transceiver.py` around lines 61 - 73, Add a concise docstring to wait_for_transfer_completion describing its purpose (poll until a KV transfer completes or timeout), parameters (poll_fn: callable invoked each loop to drive progress, is_done_fn: callable returning True when transfer finished, timeout_s: seconds to wait before failing), default behavior (uses DEFAULT_KV_TRANSFER_TIMEOUT_S + KV_TRANSFER_COMPLETION_MARGIN_S and polls every 0.01s), and the failure behavior (raises an assertion on timeout). Place this docstring immediately below the def wait_for_transfer_completion(...) signature.
🤖 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/others/test_kv_cache_transceiver.py`:
- Line 477: The pytest timeout on the test is too short and may kill the test
before wait_for_transfer_completion can assert; change the decorator on this
test from `@pytest.mark.timeout`(60) to `@pytest.mark.timeout`(120) so it matches
other transfer-completion tests (e.g., test_kv_cache_transceiver_single_process,
test_async_transfer_keeps_llm_request_alive,
test_hybrid_cache_transceiver_single_process) and allows
wait_for_transfer_completion (the helper used around line 544) to finish.
---
Nitpick comments:
In `@tests/unittest/others/test_kv_cache_transceiver.py`:
- Around line 61-73: Add a concise docstring to wait_for_transfer_completion
describing its purpose (poll until a KV transfer completes or timeout),
parameters (poll_fn: callable invoked each loop to drive progress, is_done_fn:
callable returning True when transfer finished, timeout_s: seconds to wait
before failing), default behavior (uses DEFAULT_KV_TRANSFER_TIMEOUT_S +
KV_TRANSFER_COMPLETION_MARGIN_S and polls every 0.01s), and the failure behavior
(raises an assertion on timeout). Place this docstring immediately below the def
wait_for_transfer_completion(...) signature.
🪄 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: 891d89d2-3362-41d2-89f5-db6f40845a91
📒 Files selected for processing (2)
cpp/tensorrt_llm/batch_manager/cacheTransceiver.cpptests/unittest/others/test_kv_cache_transceiver.py
|
PR_Github #53393 [ run ] triggered by Bot. Commit: |
7d71ebd to
4654ab7
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #53403 [ run ] triggered by Bot. Commit: |
|
PR_Github #53393 [ run ] completed with state |
|
PR_Github #53403 [ run ] completed with state
|
4654ab7 to
d2006d7
Compare
|
/bot run --disable-fail-fast |
9e50e98 to
a179842
Compare
|
/bot run |
1 similar comment
|
/bot run |
|
/bot run --disable-fail-fast |
|
PR_Github #55413 [ run ] triggered by Bot. Commit: |
|
PR_Github #55416 [ run ] triggered by Bot. Commit: |
|
PR_Github #55413 [ run ] completed with state |
|
PR_Github #55420 [ run ] triggered by Bot. Commit: |
|
PR_Github #55420 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #55539 [ run ] triggered by Bot. Commit: |
|
PR_Github #55539 [ run ] completed with state |
|
Superseded by rebase; removing stale pre-rebase bot command. |
13c0eb2 to
e8f3802
Compare
|
/bot run --disable-fail-fast --stage-list "DGX_B200-8_GPUs-PyTorch-1, DGX_B200-8_GPUs-PyTorch-2" |
|
PR_Github #55583 [ run ] triggered by Bot. Commit: |
|
PR_Github #55588 [ run ] triggered by Bot. Commit: |
|
PR_Github #55583 [ run ] completed with state |
|
PR_Github #55588 [ 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>
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
e8f3802 to
47cf0af
Compare
|
Closing this PR; PR#15356 will carry the changes made by this PR. |
Pull request was closed
Summary
future.get()/ future wait). In that state, Python timeout handling cannot make progress because control has not returned fromcheck_*_transfer_status().CacheTransceiverConfig.kv_transfer_timeout_msplus a small margin.Dependency graph
Arrows point from prerequisite to dependent. PR numbers in graph nodes are clickable.
#15422is a standalone main-based teardown hardening PR. It is independent of the bounded-polling stack, but should merge before#15238if possible so the cancellation PR can build on the safer shutdown/lifetime behavior after rebasing.graph TD PR15139["<a href='https://github.com/NVIDIA/TensorRT-LLM/pull/15139'>#15139</a>: transfer state consensus (merged)"] PR15422["<a href='https://github.com/NVIDIA/TensorRT-LLM/pull/15422'>#15422</a>: teardown hardening (open for review, independent)"] PR15181["<a href='https://github.com/NVIDIA/TensorRT-LLM/pull/15181'>#15181</a>: bounded C++ transfer status polling (this PR, 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 PR15422 -.->|preferred hardening| 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 stroke:#64748b,stroke-width:2px,stroke-dasharray:3 3; linkStyle 5,6 stroke:#6b7280,stroke-width:2px,stroke-dasharray:5 5; class PR15139 merged; class PR15181 current; class PR15356 inflight; class PR15422 inflight; class PR15238 draft; class WORK_BLOCKALL,WORK_BUFFER downstream;Scope
Testing
git diff --check upstream/main..HEADpython -m py_compile tests/unittest/others/test_kv_cache_transceiver.pyTargeted pytest could not be run locally because this environment is missing
transformersduring repo import.Note: the final commit was created with
--no-verifybecause local pre-commit hooks invoke systempython33.9, and the existing repo test-list hook script uses newerstr | Nonetyping syntax.Summary by CodeRabbit