[TRTLLM-14010][feat] report KV cache transfer state on executor hangs - #16300
Conversation
e878d4c to
022bdc4
Compare
|
/bot run --disable-fail-fast --stage-list "PerfSanity" |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCacheTransceiver now publishes synchronized transfer status snapshots and exposes diagnostic output through C++ and Python interfaces. PyExecutor registers transceiver diagnostics with HangDetector, which logs provider output when a hang is detected. ChangesCacheTransceiver Hang Diagnostics
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PyExecutor
participant HangDetector
participant KvCacheTransceiver
participant CacheTransceiver
PyExecutor->>HangDetector: register transceiver status provider
HangDetector->>KvCacheTransceiver: request status dump on hang
KvCacheTransceiver->>CacheTransceiver: delegate bound implementation request
CacheTransceiver-->>KvCacheTransceiver: return formatted transfer snapshot
KvCacheTransceiver-->>HangDetector: return diagnostic string
HangDetector-->>PyExecutor: log status during hang handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/integration/defs/perf/test_perf_sanity.py (2)
1507-1534: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant failure-status write.
_write_server_failureruns in theexceptblock (Line 1509) and again infinally(Line 1526), sincefailure_messageis set. Theexceptwrite is immediately superseded by thefinallywrite (which also captures the post-terminationreturn_code). Consider dropping theexcept-block write and keeping only thefinallypath to avoid the double I/O and duplicated first-error scan.🤖 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/integration/defs/perf/test_perf_sanity.py` around lines 1507 - 1534, Remove the _write_server_failure call from the except block and retain the finally-block invocation after server cleanup, preserving failure_message assignment and exception propagation so the single write includes the post-termination return_code.
1521-1524: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
server_proc.wait()afterterminate()has no timeout and can hang the harness.If a server ignores
SIGTERM,wait()blocks indefinitely, stalling the test until the CI-level timeout. Bound the wait and escalate tokill().♻️ Proposed fix
if server_proc is not None: if server_proc.poll() is None: server_proc.terminate() - server_proc.wait() + try: + server_proc.wait(timeout=30) + except subprocess.TimeoutExpired: + server_proc.kill() + server_proc.wait()🤖 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/integration/defs/perf/test_perf_sanity.py` around lines 1521 - 1524, Update the server_proc cleanup block to bound the wait after terminate(), using a timeout and escalating to server_proc.kill() when the process does not exit promptly; then ensure the killed process is reaped without an unbounded wait.
🤖 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 `@tests/integration/defs/perf/test_perf_sanity.py`:
- Around line 1507-1534: Remove the _write_server_failure call from the except
block and retain the finally-block invocation after server cleanup, preserving
failure_message assignment and exception propagation so the single write
includes the post-termination return_code.
- Around line 1521-1524: Update the server_proc cleanup block to bound the wait
after terminate(), using a timeout and escalating to server_proc.kill() when the
process does not exit promptly; then ensure the killed process is reaped without
an unbounded wait.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 62d726ed-d58f-4f53-8b35-4b0475ec08f1
📒 Files selected for processing (1)
tests/integration/defs/perf/test_perf_sanity.py
|
PR_Github #58900 [ run ] triggered by Bot. Commit: |
|
/bot run --disable-fail-fast --stage-list "*GB200*PerfSanity*,*GB300*PerfSanity*" |
|
PR_Github #58900 [ run ] completed with state
|
|
PR_Github #58909 Bot args parsing error: usage: /bot [-h] |
|
/bot run --disable-fail-fast --stage-list "GB200PerfSanity*,GB300PerfSanity*" |
|
PR_Github #58914 [ run ] triggered by Bot. Commit: |
|
/bot run --disable-fail-fast --stage-list "*GB200*PerfSanity*,*GB300*PerfSanity*" |
|
PR_Github #58916 [ run ] triggered by Bot. Commit: |
|
PR_Github #58914 [ run ] completed with state |
|
/bot run --disable-fail-fast --stage-list "*GB200*PerfSanity*,*GB300*PerfSanity*" |
|
PR_Github #58927 [ run ] triggered by Bot. Commit: |
|
PR_Github #58916 [ run ] completed with state |
|
PR_Github #58927 [ run ] completed with state
|
5982080 to
08a66e7
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #59170 [ run ] triggered by Bot. Commit: |
|
PR_Github #59170 [ run ] completed with state
|
|
PR_Github #62860 [ run ] triggered by Bot. Commit: |
|
PR_Github #62860 [ run ] completed with state
|
|
/bot run |
|
PR_Github #62868 [ run ] triggered by Bot. Commit: |
|
PR_Github #62868 [ run ] completed with state
|
|
/bot run |
|
PR_Github #62886 [ run ] triggered by Bot. Commit: |
|
PR_Github #62886 [ run ] completed with state
|
|
/bot run |
|
PR_Github #62894 [ run ] triggered by Bot. Commit: |
|
PR_Github #62894 [ run ] completed with state
|
|
/bot run |
|
PR_Github #62908 [ run ] triggered by Bot. Commit: |
|
PR_Github #62908 [ run ] completed with state
|
|
/bot run |
|
/bot run --disable-fail-fast |
|
PR_Github #62928 [ run ] triggered by Bot. Commit: |
|
PR_Github #62929 [ run ] triggered by Bot. Commit: |
|
PR_Github #62928 [ run ] completed with state |
|
PR_Github #62929 [ run ] completed with state
|
|
/bot run |
|
/bot skip --comment "The failed test has been fixed in #16980" |
|
PR_Github #62980 [ run ] triggered by Bot. Commit: |
|
PR_Github #62981 [ skip ] triggered by Bot. Commit: |
|
PR_Github/16300-9a78660 #62980 was force-killed by a newer pipeline run. |
|
PR_Github #62981 [ skip ] completed with state |
| status_counts = Counter() | ||
| receiver_ready = 0 | ||
| for session in sessions_snapshot: | ||
| status = session.status |
There was a problem hiding this comment.
Could we check has_failed() before bucketing here？
| f"ready_to_transfer={status_counts[SessionStatus.READY]}", | ||
| f"transferring={status_counts[SessionStatus.TRANSFERRING]}", | ||
| f"kv_transferred={status_counts[SessionStatus.KV_TRANSFERRED]}", | ||
| f"fully_transferred={status_counts[SessionStatus.FULLY_TRANSFERRED]}", |
There was a problem hiding this comment.
This field stays 0 in the default setup.
| fields = [ | ||
| f"sessions={len(sessions_snapshot)}", | ||
| f"init={status_counts[SessionStatus.INIT]}", | ||
| f"ready_to_transfer={status_counts[SessionStatus.READY]}", |
There was a problem hiding this comment.
The receive side never yields the ready state.
|
|
||
| tx_status = summarize(self._send_sessions, include_receiver_ready=True) | ||
| rx_status = summarize(self._recv_sessions, include_receiver_ready=False) | ||
| return ( |
There was a problem hiding this comment.
Would you consider adding a few stuck request ids and the oldest session's age, so context-side and generation-side logs can be joined?
|
|
||
| detector = HangDetector(timeout=1, on_detected=lambda: events.append(("detected", None))) | ||
| detector.register_status_provider(failing_provider) | ||
| detector.register_status_provider(lambda: "transceiver status") |
There was a problem hiding this comment.
Neither real dump is executed by any test.
| std::unordered_set<LlmRequest::RequestIdType> mFailedRequesterRequestIds; | ||
| std::unordered_map<LlmRequest::RequestIdType, std::shared_ptr<LlmRequest>> mRequesterRequestsAwaitingConsensus; | ||
| std::atomic_size_t mSyncRequesterActive{0}; | ||
| // Live transfer containers are owned by the executor worker thread. Synchronous receive threads update only the |
There was a problem hiding this comment.
The comments are outdated.
| try: | ||
| print_all_stacks() | ||
| self.on_detected() | ||
| except Exception: # noqa: BLE001 - stack dump must not block hard kill |
There was a problem hiding this comment.
Could we log one line here?
| # diagnostics are done and on_detected is about to fire. | ||
| with self.lock: | ||
| self._detected = True | ||
| self.on_detected() |
There was a problem hiding this comment.
Could we consider moving it in to finally?
| def failing_provider(): | ||
| raise RuntimeError("provider failed") | ||
|
|
||
| monkeypatch.setattr(hang_detector_module.asyncio, "sleep", no_sleep) |
There was a problem hiding this comment.
A small timeout might be enough.
|
Thanks for the effort. Since the CI has already passed, these issues can be addressed later and submitted together with other PRs to save effort. |
Summary
Status output
V1 / C++ transceiver:
V2 / Python NIXL transceiver:
Dev Engineer Review
CacheTransceivernow maintains a synchronizedStatusSnapshot(mutex-guarded, with an atomic “sync requester active” counter) and exposesgetStatusDump()for a consistent TX/RX/transfer-buffer status view.SyncRequesterStatusGuardpluspublishStatusSnapshot() noexceptto refresh snapshot data at key transfer checkpoints and around consensus/terminal cleanup.noexcept, swallowsstd::system_error) to avoid disrupting transfer flows._wait_reqssize.HangDetectornow supports registration of caller-provided status providers; failures in individual providers are caught and logged so hang handling/stack reporting andon_detectedstill proceed.getStatusDump()is surfaced to Python viaKvCacheTransceiver.get_status_dump()/BindKvCacheTransceiver.get_status_dump(), and then registered withHangDetectorfromPyExecutorwhen a transceiver is present.<nanobind/stl/string.h>and reformats a constructor default.tests/unittest/_torch/executor/test_hang_detector_kill.pyis listed intests/integration/test_lists/test-db/l0_sanity_check.yml(entry at line ~41).QA Engineer Review
tests/unittest/_torch/executor/test_hang_detector_kill.py::test_status_provider_errors_are_loggedtests/integration/test_lists/test-db/l0_sanity_check.ymlentry forunittest/_torch/executor/test_hang_detector_kill.py.Description
Test Coverage
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.