Repository navigation
[Frontend] Write run-batch responses incrementally instead of at the end - #52505
DarkLight1337 merged 45 commits into
Conversation
run_batch held every response in memory until the whole batch finished and wrote the output file only once, so an interrupted run produced no output at all -- a batch killed at 99% had to be redone from scratch -- and frontend memory grew with the size of the input file. Requests are now parsed in a first pass and executed in a second, with at most --max-inflight alive at a time and responses appended as they complete. The validation pass keeps today's behaviour that a malformed request fails the batch before any output is produced. A URL input is downloaded to a temporary file so it can be read by both passes, and a URL output is staged locally and uploaded once, leaving upload_data untouched. Responses are now ordered by completion rather than by input position; custom_id remains the key for matching them back to requests. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Fangchen Li <fangchen.li@outlook.com>
Two fixes to the incremental output path. The output file is opened for writing before the input is re-read, so an output path that resolves to the input -- directly, or through a relative path or symlink -- truncated the batch after validation and produced an empty, successful run. Reject that case up front, before the engine starts. Progress also under-counted: a request with an unsupported URL is answered with a synthesized error that never reaches run_request, the only caller of tracker.completed(), so the bar stopped short of the batch size. Count each response as it is written instead, which covers both paths. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Fangchen Li <fangchen.li@outlook.com>
The registry mapped endpoint keys to dict[str, Any], so a mistyped key in run_one_request surfaced only as a KeyError at request time. Its entries all have the same three fields, so describe them with a TypedDict and use it for the registry, its builder, and the dispatch functions. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Fangchen Li <fangchen.li@outlook.com>
Progress is now counted as each response is written, so the tracker no longer needs a separate submitted count: it was incremented on every dispatch but never read, since the bar's total comes from the validation pass. Removing it also takes the tracker out of the request path, leaving it used only where responses are written. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Fangchen Li <fangchen.li@outlook.com>
The drain ran in two places, once when the inflight set filled and once after the input was exhausted, which needed a helper to avoid duplicating it and left the two sites free to diverge. Top up and drain in one loop instead, so every response is written by the same code and a missing final drain is no longer possible. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Fangchen Li <fangchen.li@outlook.com>
Reading the batch twice assumed the input could be reopened, which is not true for a pipe or a process substitution: the validation pass drained it and the run then saw an empty file, finishing successfully with no output. Stage anything that is not a regular file, the same way a URL is staged. Also from review: reject a non-positive --max-inflight instead of silently running one request at a time, and cancel whatever is still in flight when the batch aborts so the requests do not outlive the run. Three docs were inaccurate. Incremental writes only make a local output recoverable, since a URL destination uploads once at the end; --output-tmp-dir now stages inputs as well as outputs; and the second parse of each request is deliberate. The batch example said responses come back in input order, which has not been true since responses started being written as they complete. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Fangchen Li <fangchen.li@outlook.com>
The bound is what keeps frontend memory flat, but nothing asserted it: removing it left every test passing while the whole input file was submitted at once. Record peak concurrency through a stubbed request runner and assert it stays within the limit. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Fangchen Li <fangchen.li@outlook.com>
Adds tests for the paths the earlier ones left open: staging a URL input to a local file, staging and uploading a URL output exactly once after the batch finishes, uploading nothing when the batch fails, which pins the documented limit that only a local output is recoverable, and cancelling the requests still in flight when a batch aborts. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Fangchen Li <fangchen.li@outlook.com>
A descriptor alias such as /dev/stdin or /dev/fd/3 backed by a regular file stats as one, so it took the no-copy path, but reopening it shares the descriptor's offset on macOS: the validation pass drained it and the run then saw an empty file and exited successfully with no responses. Decide staging by path as well as file type so these are copied like any other stream. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Fangchen Li <fangchen.li@outlook.com>
The message listed the supported endpoints as a literal, so it could fall out of step with the registry it describes, which has already happened elsewhere in this file: the batch example documented three of the six endpoints, and BatchRequestInput still says only chat completions is supported. Record what each matcher accepts next to the matcher and build the message from that. The wording also gets more accurate. Score and rerank match by suffix, which the list now shows as */score and */rerank rather than deferring to api_server.py for the supported versions. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Fangchen Li <fangchen.li@outlook.com>
The list was joined inside the per-request path, rebuilding the same constant string for every request that could not be routed. Build it once when the batch starts and pass it down, keeping it derived from the registry so it still cannot drift. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Fangchen Li <fangchen.li@outlook.com>
supported_urls repeated what url_matcher already encoded, so the pair had to be kept in step by hand, which is the drift the message was meant to avoid. Keep the pattern and derive the match from it: an exact URL, or a "*" prefix for the suffix match that score and rerank use. That also drops the string the previous commit threaded through the batch to avoid rebuilding it. The list is built from the registry where the error is raised, which only happens for a request that cannot be routed. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Fangchen Li <fangchen.li@outlook.com>
The "*" prefix read as a glob while the code behind it was endswith, which matches more than a glob would: "*/score" suggests one path segment, but /a/b/score matched too. Say it plainly with a match_suffix flag next to the URL, and pass the endpoint's config to the handler rather than unpacking a field at a time. Matching is unchanged: score and rerank still accept any URL ending in theirs, and everything else still compares exactly. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Fangchen Li <fangchen.li@outlook.com>
Three fixes from review. Requests complete in groups, and the group was written by calling result() on each in turn, so an unexpected failure raised partway through and discarded every response after it in that group. With the default bound that is up to a thousand finished responses thrown away, and nothing uploaded at all for a URL destination. Write the whole group, then raise the first failure. Cancelling the rest of the batch did not wait for the cancellations, so the engine could be torn down while requests were still unwinding. Await them. Staging also caught too much: keying on the /dev and /proc trees copied ordinary files such as a batch on tmpfs, which are re-readable and can be large. Recognise the descriptor aliases themselves instead. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Fangchen Li <fangchen.li@outlook.com>
The suffix match said only that the URL ended in /score, which also accepted paths that were never endpoints, /foo/score among them, and left the rule implicit. Versioning is the reason those two differ, so say that: an endpoint is matched exactly, and a versioned one also under a /vN prefix. Every URL the server actually serves still routes, /score, /v1/score and /v2/rerank included. Paths that only happened to end in the same segment no longer do; they now return the unsupported-endpoint error instead of reaching a handler. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Fangchen Li <fangchen.li@outlook.com>
The versioned flag repeated what the URL already said: the endpoints served under a version prefix are exactly the ones whose own URL carries no version. Keeping both meant they could disagree, so drop the flag and read it from the URL. Routing is unchanged. /score still matches /v1/score and /v2/score, an endpoint that already names its version still matches only that version, and a doubled prefix such as /v1/v1/embeddings is still refused. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Fangchen Li <fangchen.li@outlook.com>
The batch example listed three of the six endpoints served, omitting rerank and both audio ones, and BatchRequestInput said only chat completions was supported while the dispatch immediately below it handled all six. Both predate this branch but describe the routing it changes. /v1/completions is still genuinely unsupported, so that note stays. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Fangchen Li <fangchen.li@outlook.com>
|
Documentation preview: https://vllm--52505.org.readthedocs.build/en/52505/ |
|
This pull request has merge conflicts that must be resolved before it can be |
There was a problem hiding this comment.
Beyond the inline finding, this run also checked three other angles and ruled them out: a local --input-file changing between validate_batch's and dispatch_batch's separate reads, the same race causing a silent 0-request run while the model loads, and output-file truncation on a rerun that fails before writing any response — none produced a distinct defect beyond what's flagged inline.
Extended reasoning...
The diff rewrites vllm/entrypoints/launchers/run_batch.py to stream batch requests/responses instead of buffering, adding open_batch_input (local/piped/URL staging, including a URL download via global_http_connection.async_download_file), batch_output_writer, validate_batch, and a concurrency-bounded dispatch_batch with reorder-window semantics, plus matching test and doc updates. Security-relevant surface is the URL-based input download and output upload path; the confirmed inline finding is that the download call passes no timeout, turning the previous ~5 minute default stall timeout into an indefinite hang. Given that confirmed finding, this run also examined input-file TOCTOU between the two reads and output-file truncation-on-failed-rerun scenarios and found no additional distinct defects.
Pass aiohttp's default 300s total timeout, which the old read_file got implicitly. The shared download helper retries on timeout with a longer timeout each attempt. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Fangchen Li <fangchen.li@outlook.com>
|
@claude review |
A dead engine answers every remaining request with an error response, and those rows used to be written in order until the input ran out, so the output was no longer a resumable prefix. Raise the dead-engine error on the first error response after the engine dies; responses completed before then are still written. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Fangchen Li <fangchen.li@outlook.com>
The engine answers requests past --max-num-queued-reqs or --max-num-queued-tokens with a 503 so an online client can retry elsewhere; run-batch could only write each one out as an error row, and its request-count cap missed n > 1 requests. Warn and clear both limits before the engine starts, leaving --max-inflight as the one concurrency bound. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Fangchen Li <fangchen.li@outlook.com>
|
@claude review |
|
/ci retry |
|
✅ No earlier CI build exists for this PR. Use |
|
/ci run |
|
✅ Triggered Buildkite CI #95658 for commit |
|
/ci retry |
|
✅ Triggered Buildkite CI #96052 for commit |
Overview
vllm run-batchbuffered every response and wrote the output only when the whole batch finished. Responses are now written in input order as the batch runs, and frontend memory no longer grows with the input. Follows #53500, which moved this file. Related: #30268.Claims
--max-inflightrequests run at once, and at most 16 times that many are dispatched but not yet written.--output-filefails before the model loads. A dead engine fails the run at its first error response, so the output stays a prefix of real responses, instead of exiting 0.Validation
At 5dbc1cb on an RTX A6000:
pytest tests/entrypoints/launchers/test_run_batch.py -vPre-commit passes. @vinitrbl's A10G run showed SIGTERM and SIGKILL leaving every finished line, where v0.31.0 left no file, and peak frontend RSS on 50k requests dropping from 1876 to 1159 MiB.
On an L4: killing EngineCore at 417 of 4000 requests left 448 rows with no errors, exit 1, and no URL upload at d73fed1, where the previous head wrote all 4000 rows with 3580 errors. With both queue limits cleared, 200 requests went from 196 rows of 503 to 0 at
--max-num-queued-reqs 8withn=4, and from 199 to 0 at--max-num-queued-tokens 64.Reorder window. Writing in input order lets one long request hold back everything behind it. Throughput against writing in completion order, on an H100 with 4000 chat requests, median of 3:
--max-inflightQwen3-0.6B on the heavy tail gave −41%, −4%, and +1%. Peak frontend RSS was about 1.1 GB throughout.
Details
--max-inflightof them, which is about 45 MB of chat responses at the default.--max-inflightis twicemax_num_seqsacross data-parallel engines, at least 1024.--max-num-queued-reqsand--max-num-queued-tokensare ignored with a warning, since the engine would answer requests past them with 503s.vllm/connections.py.results.jsonl.ahead, in the same format and input order. A resume, or a later--resumeflag, could then skip both the written prefix and that file. Handling SIGTERM the same way would cover preemption too.AI assistance (Claude Code) was used for this change. The author reviewed every line and ran the tests above.
Pull Request Checklist
I used vLLM's
/pr-checklistskill. (Mandatory for agents, optional for humans).AI assistance was used during the creation of this PR.
Design Fit: Minimizes impact on core components, reuses existing functionality, and justifies added complexity.
Testing and Validation: Validates the change and ensures any added tests are meaningful and reliable, with CI coverage or documented CI resource constraints and validation performed outside CI.
Code Quality and Style: Keeps code and comments clear and concise, and updates relevant documentation and examples.
Pull Request Contents: Includes a brief summary and relevant links, supports claims with evidence, explains root causes and implementation trade-offs, and follows the contributing guide.
🤖 Generated with Claude Code