[None][fix] return an explicit error if the requests can't be schedul… - #12206
Conversation
dc9e713 to
9c49136
Compare
|
/bot run --disable-fail-fast |
|
/bot kill |
|
PR_Github #38999 [ run ] triggered by Bot. Commit: |
|
PR_Github #39000 [ kill ] triggered by Bot. Commit: |
|
PR_Github #39000 [ kill ] completed with state |
9c49136 to
108b266
Compare
📝 WalkthroughWalkthroughAdds error handling for gen-only disaggregated KV-cache mode: when benchmark requests exceed KV capacity, the system now detects insufficient allocation, logs an error, and fails all active and waiting requests to prevent hangs. Error responses propagate through the execution pipeline and emit proper HTTP status via SSE events in streaming responses. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as openai_server
participant Executor as py_executor
participant Worker as postproc_worker
participant Result as result.py
Client->>Server: POST /completions (stream=true)
Server->>Executor: Enqueue benchmark request(s)
Executor->>Executor: Check KV cache capacity
Executor->>Executor: No space for new init requests<br/>& all requests waiting for allocation
Executor->>Executor: Generate ErrorResponse<br/>for all active & waiting requests
Executor->>Worker: Pass ErrorResponse
Worker->>Worker: Detect ErrorResponse type
Worker->>Result: Append error to batch<br/>Skip finalization
Result->>Result: Set _done = True<br/>(mark completion)
Result->>Result: Invoke error handler
Result->>Server: Error response data
Server->>Server: Catch Exception<br/>Log traceback
Server->>Client: SSE data: error object<br/>(HTTP 200 + error event)
Client->>Client: Process error event<br/>terminate stream
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tensorrt_llm/_torch/pyexecutor/py_executor.py`:
- Around line 1798-1813: The benchmark-failure path must fail queued
RequestQueueItem entries separately and stop normal scheduling: gather requests
from self.active_requests and from the RequestQueueItem objects in
self.waiting_queue (e.g., extract the contained LlmRequest from each queued
item), call self._handle_errors(error_msg, requests=all_requests) with that
combined list, clear self.waiting_queue, and then return immediately (do not
return or reuse scheduled_batch) so the scheduler does not continue preparing or
forwarding requests that were just terminated; update the block around
waiting_queue, _handle_errors, active_requests, RequestQueueItem and
scheduled_batch accordingly.
In `@tests/integration/defs/disaggregated/test_disaggregated.py`:
- Around line 712-715: The variable returned from setup_disagg_cluster is bound
to config but never used; rename that binding to _config (or drop it and only
unpack the values you need) where you call setup_disagg_cluster so the tuple
assignment becomes _config, ctx_workers, gen_workers, disagg_server,
server_port, work_dir = setup_disagg_cluster(...); update the surrounding call
site that refers to setup_disagg_cluster to use the new name (or reduced
unpacking) to silence the unused-variable warning.
- Around line 717-753: Replace the blind try/except in send_request with
submitting and collecting futures, then wait on each future with a bounded
timeout and assert the per-request failure contains the expected insufficient-KV
error; specifically, stop swallowing all exceptions in send_request, instead
submit callables via pool.submit into a list (e.g., futures =
[pool.submit(lambda: client.completions.create(...)) for _ in range(64)]), then
iterate over futures and call future.result(timeout=10) inside a try/except that
only catches the specific client/error class (or Exception if unavoidable) and
asserts the exception message includes the insufficient-KV text; keep the
existing gen_workers/process poll check but add these per-request assertions to
ensure each request fails as expected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b05bdb21-3dd5-4d61-bd5b-db81578fe077
📒 Files selected for processing (6)
tensorrt_llm/_torch/pyexecutor/py_executor.pytensorrt_llm/executor/postproc_worker.pytensorrt_llm/executor/result.pytensorrt_llm/serve/openai_server.pytests/integration/defs/disaggregated/test_configs/disagg_config_gen_only_insufficient_kv.yamltests/integration/defs/disaggregated/test_disaggregated.py
|
/bot run --disable-fail-fast |
|
PR_Github #39116 [ run ] triggered by Bot. Commit: |
cda212d to
d408fb8
Compare
|
PR_Github #39116 [ run ] completed with state
|
d408fb8 to
eca326b
Compare
|
/bot run --disable-fail-fast |
eca326b to
bc782b4
Compare
|
PR_Github #39208 [ run ] triggered by Bot. Commit: |
|
PR_Github #39208 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
/bot run --disable-fail-fast |
|
PR_Github #39430 [ run ] triggered by Bot. Commit: |
|
PR_Github #39430 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #39487 [ run ] triggered by Bot. Commit: |
|
PR_Github #39487 [ run ] completed with state |
85cfbe4 to
fd113e7
Compare
|
/bot run --disable-fail-fast |
…ed due to insufficient KVCache for gen-only Signed-off-by: Iman Tabrizian <10105175+tabrizian@users.noreply.github.com>
Signed-off-by: Iman Tabrizian <10105175+tabrizian@users.noreply.github.com>
Signed-off-by: Iman Tabrizian <10105175+tabrizian@users.noreply.github.com>
Signed-off-by: Iman Tabrizian <10105175+tabrizian@users.noreply.github.com>
fd113e7 to
6af3215
Compare
|
PR_Github #39623 [ run ] triggered by Bot. Commit: |
|
PR_Github #39623 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #39661 [ run ] triggered by Bot. Commit: |
|
PR_Github #39661 [ run ] completed with state |
NVIDIA#12206) Signed-off-by: Iman Tabrizian <10105175+tabrizian@users.noreply.github.com>
Design doc covering the deadlock fix in benchmark disaggregated serving mode: problem statement, root cause analysis (structural starvation in fill loop), comparison with prior PRs (NVIDIA#12091, NVIDIA#12206), non-blocking gate design, ADP dummy request handling, and test coverage. Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com> Made-with: Cursor
Design doc covering the deadlock fix in benchmark disaggregated serving mode: problem statement, root cause analysis (structural starvation in fill loop), comparison with prior PRs (NVIDIA#12091, NVIDIA#12206), non-blocking gate design, ADP dummy request handling, and test coverage. Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com> Made-with: Cursor
Design doc covering the deadlock fix in benchmark disaggregated serving mode: problem statement, root cause analysis (structural starvation in fill loop), comparison with prior PRs (NVIDIA#12091, NVIDIA#12206), non-blocking gate design, ADP dummy request handling, and test coverage. Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com> Made-with: Cursor
…hase The PR NVIDIA#12206 fail-fast ("Insufficient KV cache") fires in _prepare_and_schedule_batch BEFORE the gate check in _check_benchmark_disagg_gate. During the fill phase, INIT requests that are waiting for KV transfer cannot be scheduled (KV cache is occupied by already-transferred requests), which is expected and transient — not genuine KV insufficiency. Add `not self._benchmark_fill_phase_active` to the fail-fast guard so it only fires after the gate opens (fill phase complete). After that point, stuck INIT requests genuinely indicate insufficient KV cache. Add TestFailFastSuppressedDuringFill (5 tests) and TestFillPhaseEndToEnd (1 end-to-end lifecycle test) covering the full fill → gate-open → post-fill fail-fast sequence. Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com> Made-with: Cursor
Update the benchmark disagg fill design docs to reflect the final three-part PR NVIDIA#13347 fix: state-based gate, ADP router cap, and fill-phase fail-fast suppression. Add a dedicated note explaining why PR NVIDIA#12206 fail-fast must be disabled while the benchmark fill barrier is active. Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com> Made-with: Cursor
Description
In gen-only disaggregated benchmark mode, when the KV cache is too small to hold all benchmark requests, the executor gets stuck: requests waiting for KV cache allocation can never be scheduled because in-progress generation requests hold the cache indefinitely. Previously this caused the benchmark to hang silently with no error.
This PR detects the condition and returns an explicit error to every client, allowing the benchmark to terminate cleanly instead of hanging.
Root cause
In gen-only mode the benchmark fill loop pre-queues all requests before the first forward pass. When the KV cache is insufficient, some requests remain in
DISAGG_GENERATION_INITstate permanently because the scheduler cannot allocate KV blocks for them while existing generation requests hold theirs.Fix (4 files)
py_executor.py– After all benchmark requests have been fetched (num_fetch_requests >= benchmark_req_queues_size), detect stuck init requests and call_handle_errorson all active + waiting requests so every client receives an error response.postproc_worker.py–ErrorResponseobjects lack aresultattribute which crashed the postproc worker. Forward them directly to the proxy instead.result.py– Set_done = TruewhenGenerationResultreceives anErrorResponseso the async result completes instead of looping forever.openai_server.py–StreamingResponsecommits HTTP 200 before the generator yields, so raising an exception breaks the connection (ClientPayloadError). Yield an SSE error event instead so the stream terminates cleanly.Test Coverage
tests/integration/defs/disaggregated/test_disaggregated.py::test_disaggregated_benchmark_gen_only_insufficient_kv– Launches a gen-only disagg cluster with a tiny KV cache (
max_tokens=128)and 64 concurrent requests, then asserts the gen worker process terminates
instead of hanging.
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)
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 this 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.Summary by CodeRabbit
Bug Fixes
Tests