Repository navigation
[Core] Reduce async chunk request snapshot overhead - #8833
Conversation
Signed-off-by: psv666 <2693925048@qq.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
This PR appears to belong to: docs/design/module/input_output_modality_contracts.md, docs/design/module/omni_connector.md, docs/design/module/diffusion/parallelism.md. Module owners: @princepride @alex-jw-brooks @amy-why-3459 Routing: @princepride via module of the changed files, CODEOWNERS; @alex-jw-brooks via module of the changed files; @amy-why-3459 via module of the changed files @psv666, please review your own changes and leave a short self-review comment describing what you checked. PRs without author self-review may not be assigned a reviewer. Please take a look when you have a chance. If you would like an automated review, mention @vllm-omni-review-bot in a comment. |
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
Omni ReviewBot: three questions on the performance claim@psv666 this PR reads as a performance or value claim:
Before the full evidence checklist, three short questions:
When you answer, the evidence that settles it is: base and head SHA, hardware, model, workload, warm-up and repeat count, mean or percentiles with their spread, and a correctness/quality-equivalence signal; an end-to-end claim also needs stage attribution. |
amy-why-3459
left a comment
amy-why-3459
left a comment
There was a problem hiding this comment.
Thanks for the clear write-up and the before/after numbers on #8794.
The fast path matches the existing _all_token_ids / _output_token_ids .copy() pattern, and the long-prompt + embeds-only tests cover the isolation cases that matter here. prompt_token_ids on current vLLM Request is a plain list[int] | None, so the isinstance(..., list) gate is the right one for today’s runtime.
Two non-blocking notes below.
| if name == "prompt_token_ids" and isinstance(value, list): | ||
| # Token IDs are immutable integers. Copy the list without | ||
| # walking every prompt token in Python for each chunk. | ||
| value = value.copy() |
There was a problem hiding this comment.
Shallow list.copy() is correct for the list[int] contract and removes the per-element Python walk.
One robustness note: if prompt_token_ids ever becomes a non-list sequence (e.g. a ConstantList-style wrapper around a mutable buffer), the else branch falls through to snapshot_container, which returns unrecognized containers by identity — so the snapshot would keep sharing the live prompt. Today’s vLLM Request stores a plain list, so this is fine; if you want belt-and-suspenders, something like list(value) for any non-None prompt sequence would keep both the fast path and isolation without depending on the concrete type.
There was a problem hiding this comment.
Addressed in 8e74d1a: ordinary lists retain .copy(), while other non-None prompt sequences use list(value). The empty, short, and 2,508-token ConstantList queued-mutation cases all fail before this change and pass afterward. The six related suites pass with 221 tests on both the PR branch and latest-main integration (66bb974ff).
| if name == "prompt_token_ids" and isinstance(value, list): | ||
| # Token IDs are immutable integers. Copy the list without | ||
| # walking every prompt token in Python for each chunk. | ||
| value = value.copy() |
There was a problem hiding this comment.
Follow-up (not blocking): even with list.copy(), every queued chunk still allocates/copies the full prompt. For the common case where the prompt list is not mutated between chunks, caching one prompt snapshot per request (and only re-copying _output_token_ids / _all_token_ids) would drop the remaining O(prompt × chunks) producer cost. The resumable/append path in the test shows when a fresh copy is still required.
There was a problem hiding this comment.
Agreed that caching could remove the remaining prompt-copy cost. I’m keeping it as a separate follow-up: resumable input can extend the same prompt list in place, so request/list identity alone cannot detect changes. A safe cache needs append/replacement invalidation and a clear processor ownership contract before queued snapshots can share a cached prompt.
Signed-off-by: psv666 <2693925048@qq.com>
|
@natureofnature PTAL |
amy-why-3459
left a comment
amy-why-3459
left a comment
There was a problem hiding this comment.
Reviewed head 8e74d1a4882e7f3451d94a99cc56ea19f7196520 against baseline d78d8b13e4dfe7f2cad208fa82d7ec84cdc79ea3.
I found no actionable correctness defect in the snapshot change. Copying the list[int] prompt directly preserves isolation without recursively visiting immutable integers; nested metadata still uses the existing container snapshot logic. Local targeted verification passed 221 tests across chunk transfer, SHM, request snapshots, and the relevant Qwen3-Omni input/forward/speaker tests. A CPU microbenchmark using the actual base/head method bodies, 2,508 prompt tokens and 450 output tokens measured 241.1 → 16.6 µs per snapshot (~14.6× faster).
However, independent end-to-end validation only partially reproduces the reported TTFP improvements.
Method: three NVIDIA L20X GPUs, Qwen3-Omni-30B-A3B-Instruct, async chunking, two Talker and two Code2Wav replicas, fixed Thinker KV allocation of 8,281,128,960 bytes (84,240 tokens). Four fresh server deployments in base → head → head → base (ABBA) order; each ran concurrency 24 then 32, with 64 measured requests per concurrency after 24/32 warmup requests. Random input/output lengths were 2,500/900, range ratio 0, seed 0, --ignore-eos, speaker ethan, and stage metrics enabled.
Mean audio TTFP (ms; lower is better):
| Concurrency | Base runs B1 / B2 | Head runs P1 / P2 | Change in mean across deployments | PR-reported reduction |
|---|---|---|---|---|
| 24 | 1176.8 / 961.8 | 1512.8 / 1436.2 | 37.9% increase | 28.7% |
| 32 | 2493.6 / 2068.5 | 1439.9 / 1425.2 | 37.2% reduction | 44.5% |
At concurrency 24, both head runs were slower than both base runs. The initial 24-request burst also had mean text TTFT of about 2.33 s on head versus 0.85 s on base; this is an observed symptom, not an established root cause. Please investigate this case and either explain the discrepancy or qualify the performance claim. The concurrency-32 improvement is supported in this environment, although its magnitude is smaller.
Environment and limits: Python 3.12.13, vLLM 0.31.0, Transformers 5.14.1, PyTorch 2.13.0+cu130/CUDA 13.0. The PR reports cu129/CUDA 12.9, so this is independent validation, not an exact-environment refutation of the original measurements. All 512/512 measured requests succeeded, with no JIT warnings in the measured windows. Client results recorded 900 output tokens throughout; stage-0 metrics independently confirmed this for 511 requests, with one missing stage-0 snapshot. Audio durations varied with generation, and audio-quality equivalence was not tested, so E2EL/RTF should not be treated as fixed-workload comparisons. Repeating the ABBA comparison in the original CUDA environment would help resolve the concurrency-24 discrepancy.
Full-suite validation remains outstanding. The targeted tests and performance runs do not establish complete repository UT coverage. Please provide same-head evidence for the complete supported UT suite, including required CI shards and hardware/backend-specific UT jobs: exact commands, environment, passed/failed/skipped/deselected/collection-error counts, and CI or complete-log links. Existing complete results on this SHA are sufficient; no redundant rerun is needed. Please explain skipped/unavailable coverage and complete it or obtain an explicit maintainer disposition. If failures are pre-existing, provide a same-environment base/head comparison. Merge readiness is not established while this evidence is incomplete; this is a validation gap, not a claimed code defect.
|
Reproduced one pass of the published 3-GPU audio-TTFP setup on the existing cu129 stack (no reinstall): vLLM 0.31.0, torch 2.13.0+cu129, CUDA 12.9, 3× L20X. Compared PR head Mean audio TTFP (
Text TTFT barely moved (c24 367→364 ms, c32 557→520 ms), so the audio-TTFP drop is not coming from a slower first text token. On this host both concurrencies are faster with the patch. c32 (−38%) lines up with the author’s −44.5% and with @amy-why-3459’s −37.2%. c24 is also faster here, but only −8.9%, smaller than the author’s −28.7% on |
Contributor intent
1. What outcome do you want to achieve?
Reduce high-concurrency audio first-packet latency caused by sender backlog, while preserving request snapshot isolation.
2. What is the highest-leverage variable for that outcome?
Python CPU overhead in the per-chunk request snapshot, which competes with the sender thread in the same stage process.
3. How will this work move that variable?
Copy the prompt's immutable integer token IDs with
list.copy()instead of recursively visiting every element in Python on every queued chunk. Retain recursive copying for nested additional information and the existing output/all-token snapshots.4. Whose customer or user needs does this address, and what are those needs?
vLLM-Omni users need lower audio first-packet latency under concurrent requests, and streaming users need queued work to remain isolated from later request mutations.
Purpose
save_asyncsnapshots mutable processor inputs before admitting a task to the background sender FIFO._snapshot_processor_requestrecursively traversedprompt_token_idseven though its contract islist[int] | None. With a 2,508-token prompt and 900 generated tokens, one request visits more than 2.2 million prompt elements in Python. That producer work competes with the sender, whose shared-memory operations repeatedly release/reacquire Python execution, and the resulting backlog delays new requests' first payloads behind older decode tasks.Use a shallow copy for this flat immutable-element list and materialize other non-
Noneprompt sequences as independent lists. Keep nested metadata snapshots, output token snapshots, request lifetime fences, and FIFO order unchanged. Exercise queued-mutation isolation for empty, short and 2,508-token prompts with both plain lists andConstantListwrappers, and cover an embeddings-only request whose token list isNone.Related: #8794. This reduces a measured source of backlog; it does not claim to eliminate all FIFO head-of-line waiting or establish H100 baseline values. The benchmark speaker/baseline contract is handled separately in #8826.
Test Plan
vLLM Version: 0.31.0+cu129
GPU benchmark commits: main
d78d8b13e4dfe7f2cad208fa82d7ec84cdc79ea3vs patche8efb07b91c7a04c1c63918bde5ab09538669faaCheckpoint: Qwen3-Omni-30B-A3B-Instruct snapshot
26291f793822fb6be9555850f06dfe95f2d7e695.Environment: PyTorch 2.13.0+cu129, CUDA 12.9, Transformers 5.14.1, NVIDIA driver 570.133.20; 3 NVIDIA L20X GPUs (143,771 MiB reported per device).
uv run --no-sync python -m pytest -o addopts='' --run-level=core_model \ tests/distributed/omni_connectors/test_chunk_transfer_adapter.py \ tests/distributed/omni_connectors/test_shm_connector.py \ tests/engine/test_request_snapshot.py \ tests/model_executor/stage_input_processors/test_qwen3_omni_chunk_ramp.py \ tests/model_executor/models/qwen3_omni/test_qwen3_omni_default_speaker.py \ tests/model_executor/models/qwen3_omni/test_qwen3_omni_forward_contract.pyCI-like focused contract check (CPU-marked logic tests; no weights required):
Performance reproduction requires the repository CUDA environment, the cached checkpoint above and three GPUs. Use the cached snapshot directory for the model/tokenizer when reproducing that exact checkpoint. Run the server and client from separate terminals, under the appropriate GPU reservation on shared hosts:
uv run --no-sync python -m vllm_omni.entrypoints.cli.main serve \ Qwen/Qwen3-Omni-30B-A3B-Instruct --omni --async-chunk \ --revision 26291f793822fb6be9555850f06dfe95f2d7e695 \ --served-model-name Qwen/Qwen3-Omni-30B-A3B-Instruct \ --host 127.0.0.1 --port 18397 \ --init-timeout 1500 --stage-init-timeout 1200 \ --stage-overrides '{"0":{"kv_cache_memory_bytes":8281128960},"1":{"num_replicas":2,"devices":"1,2"},"2":{"num_replicas":2,"devices":"1,2"}}'For before/after comparison, start independent servers in main → patch → patch → main order and repeat the client sweep for each server. Thinker KV capacity is pinned to the issue's 84,240 tokens, rather than relying on this larger local GPU's auto-sizing. Thinker temperature is the deploy default 0; Talker retains natural EOS, temperature 0.9, seed 42 and max_tokens 4096. There are no sender profiling monkeypatches in this validation; per-request client results are recorded after measurements finish. Returning stage metrics is enabled explicitly for output-workload validation.
Test Result
217 passed, 17 warnings in 1.51sacross the six files above.mypy-3.10reports the same 19 existing library errors on main and the patch after normalizing line numbers; the changed test file passes type checking. No new type errors or allowlist changes.Fresh-main production-patch validation: 512/512 formal requests succeeded, plus 224 warmups, across four independently started servers (main → patch → patch → main). All formal requests used identical per-index input hashes and produced exactly 900 Thinker tokens; all frozen source/harness hashes matched. No formal-window JIT warnings. All four servers stopped with exit code 0.
Individual deployments (64 formal requests per row; warmup count equals concurrency):
Secondary metrics (equal-size deployments averaged):
The c24 TTFT mean changes by +2.5%; individual patch runs span 369–466 ms vs 399–415 ms on main. Two deployments do not establish statistical equivalence for that metric. Natural Talker EOS and generated text/audio lengths vary with scheduling even with fixed input/seed, so the RTF/E2EL changes are supporting observations, not isolated measurements of snapshot cost or output-quality parity.
At c32, the replacement 32 requests have mean TTFP 1,872/1,835 ms on main vs 358/383 ms on the patch, and maximum TTFP 11,114/9,433 ms vs 629/857 ms. The patch P99 around 2.2 s mainly reflects the initial 32-request prompt burst; this production validation does not instrument exact sender FIFO residence time.
5-second sampled GPU peak VRAM on main: Thinker 74793–76139 MiB; Talker/Code2Wav devices 93491–94461 MiB.
5-second sampled GPU peak VRAM on patch: Thinker 74793–75307 MiB; Talker/Code2Wav devices 94015–95485 MiB.
This is not an isolated GPU-allocation benchmark: audio-device sampled peaks vary by about 1 GiB between variants, with different natural-EOS/batching behavior. Thinker KV capacity is fixed at 84,240 tokens; the patch only changes Python container copying. L20X measurements are not H100 baseline values.
Latest-main integration check: both patches apply cleanly to
c10115052;289 passed, 17 warnings in 2.29sacross the six snapshot/model suites, performance-runner metadata and stage-engine client dispatch tests. The GPU measurements above use the explicitly recordedd78d8b13ebaseline; they are not presented as a new GPU run ofc10115052.Review follow-up
Commit
8e74d1a4882e7f3451d94a99cc56ea19f7196520addresses the non-blocking sequence-wrapper isolation suggestion. Current vLLMRequeststill stores plainlist[int] | None; that ordinary-list path retains.copy(). Non-Nonewrappers now uselist(value)rather than sharing their backing buffer. Nested metadata and embeddings-onlyNonehandling are unchanged.ConstantListqueued-mutation cases (empty/short/2,508 tokens) fail because a later backing-list append changes an earlier queued prompt. The plain-list cases pass.221 passed, 17 warnings in 1.63s. Applying the full PR cleanly to latest main66bb974ffgives221 passed, 17 warnings in 1.82s.mypy-3.10still has exactly the same 19 line-normalized baseline errors; the modified test file passes.e8efb07b91c7, not a new run of this follow-up. The current ordinary-list copy operation is unchanged; no new end-to-end performance figure is claimed for wrapper handling.Per-request prompt caching remains a follow-up: resumable input can extend the same prompt list in place, and downstream paths can replace it. Request/list identity alone cannot invalidate such a cache. Reusing a mutable cached list would also make queued snapshots share data unless processor ownership is tightened. This PR adds no cache or new lifecycle state.
AI assistance: Codex assisted with diagnosis, implementation, validation, and PR drafting.