Repository navigation
[Bugfix] Fix multimodal sender-cache invalidation race with queued renders - #62164
Conversation
…nders The AsyncLLM output handler invalidated the sender (P0) multimodal cache on the asyncio thread while the renderer reads that cache twice on its single-worker multimodal executor. An invalidation between the is_cached lookup and the get_and_update merge left the processor with hash-only input and no cached data, surfacing as 'AssertionError: Expected a cached item' (HTTP 500) under concurrent cache drift recovery. Queue missed hash strings on a per-renderer SimpleQueue before publishing the miss outputs, and drain the queue at every _process_multimodal entry before the first cache lookup. A running render completes undisturbed; a render queued before the miss observes the invalidation before processing. No extra executor job, Future callback or event-loop wait is introduced; unrelated outputs keep flowing while the multimodal worker is occupied. If invalidate raises, the current hash is requeued and the exception propagates to the render. Co-authored-by: Claude Code <noreply@anthropic.com> Signed-off-by: Shen <312058264+YueshenZ@users.noreply.github.com>
|
@claude review |
Ignore miss hashes absent from the sender cache and coalesce repeat notifications in a pending set. Keep the lock limited to set updates and swaps so output handling does not wait for cache invalidation. Restore all unfinished hashes on failure without losing concurrent notifications. Cover absent and duplicate hashes, cache clear and resend, and failed invalidation with concurrent misses. Reuse the existing renderer fixture. Validated with 29 cache tests and 28 renderer tests on CPU; applicable changed-file pre-commit checks passed. This follow-up has no new GPU run. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Shen <312058264+YueshenZ@users.noreply.github.com>
|
@claude review |
|
Could you run |
|
Ran Below are means across the three runs, with their min/max. The P95/P99 rows are averages of per-run quantiles.
The mean batch-time changes are small (+0.25% with caching, +0.09% without), but the initial preprocessing P99s were higher. I therefore ran a separate 1,024-prompt/32-warmup pair per cache setting; all 4,096 requests completed:
The larger-sample pairs did not reproduce the initial P99 increase. Tail results remain variable; these measurements show small observed mean-time differences in this tested configuration. Since the internal stage timer excludes the added drain, I also timed complete real Reproduction and environmentLinux, Python 3.12.3, PyTorch 2.14.0+cu130, Transformers 5.19.0, NVIDIA driver 580.105.08. Both exact Python source revisions use the same official For batch wall time, both benchmark copies identically export the existing export OMP_NUM_THREADS=1 MKL_NUM_THREADS=1 TOKENIZERS_PARALLELISM=false
export VLLM_USE_FLASHINFER_SAMPLER=0 VLLM_USE_DEEP_GEMM=0 VLLM_PLUGINS=""
# MODEL is a local snapshot of Qwen/Qwen2-VL-2B-Instruct at
# 895c3a49bc3fa70a340399125c650a463535e71c.
# Run CACHE_GB=4 and 0; three fresh processes per revision/configuration.
vllm bench mm-processor \
--model "$MODEL" --dataset-name random-mm \
--num-prompts 256 --num-warmups 16 --seed 42 \
--random-input-len 128 --random-output-len 1 --random-range-ratio 0 \
--random-mm-base-items-per-request 1 --random-mm-num-mm-items-range-ratio 0 \
--random-mm-limit-mm-per-prompt '{"image":1,"video":0}' \
--random-mm-bucket-config '{(224,224,1):0.5,(672,672,1):0.5}' \
--limit-mm-per-prompt '{"image":1,"video":0}' \
--max-model-len 2048 --max-num-seqs 32 --no-enable-prefix-caching \
--enforce-eager --gpu-memory-utilization 0.5 \
--mm-processor-cache-type lru --mm-processor-cache-gb "$CACHE_GB" \
--metric-percentiles 50,95,99 --disable-tqdm --output-json "$RESULT"The separate follow-up changes only |
Could you also run each one three times so we could see the variance? |
|
Reran the 1,024-prompt/32-warmup configuration three times per revision and cache setting — 12 fresh processes run back-to-back (~14 minutes total), with arm order alternated between repetitions (base→patch, patch→base, base→patch). Every run used the same
(sd is the sample standard deviation of the three runs; Change is mean-of-three vs mean-of-three.) Reading the variance:
Under this configuration the run-to-run variance dominates; no metric shows a patch effect beyond the spread of the three repetitions. Per-run values, in execution order
|
…ache Hoist the cache checks in queue_mm_cache_invalidations and _drain_mm_cache_invalidations into early returns, per review: both helpers are no-ops when the mm processor cache is unset, so return before the pending-set work instead of guarding the queue body and the drain loop. Semantics are unchanged: the cache reference is only assigned during renderer initialization and never goes back to None, so pending entries cannot exist while the cache is unset. Validated with tests/multimodal/test_cache.py and the sparse-tensor and warmup renderer tests on an offline A800 host; pass sets match the parent revision exactly. ruff check and format pass. Co-authored-by: Claude Code <noreply@anthropic.com> Signed-off-by: Shen <312058264+YueshenZ@users.noreply.github.com>
Return before locking when incoming hashes or filtered pending entries are empty. Let output handling call the helper directly while retaining the cache guard and enqueue-before-publish ordering. Validated with 57 CPU cache/renderer regressions, the unchanged GPU sleep/wake test on A800, and a real-renderer probe showing empty calls do not block on the invalidation lock or discard existing notifications. Co-authored-by: OpenAI Codex <noreply@openai.com> Signed-off-by: Shen <312058264+YueshenZ@users.noreply.github.com>
DarkLight1337
left a comment
There was a problem hiding this comment.
Thanks for your patience!
|
/ci run |
|
✅ Triggered Buildkite CI #96285 for commit ℹ️ This PR is 225 commits behind upstream |
|
Thanks again for the review, @DarkLight1337! The Buildkite checks are now green. The GitHub pre-commit workflow is still gated by |
|
/ci run |
|
✅ @YueshenZ, CI is now available for this PR.
|
|
✅ Triggered Buildkite CI #96323 for commit |
|
/ci retry |
|
✅ Queued 1 failed job(s) for retry in Buildkite CI #96323. |
…nders (vllm-project#62164) Signed-off-by: Shen <312058264+YueshenZ@users.noreply.github.com> Co-authored-by: Shen <312058264+YueshenZ@users.noreply.github.com> Co-authored-by: Claude Code <noreply@anthropic.com> Co-authored-by: Codex <noreply@openai.com> Co-authored-by: vllm-agent <claw@inferact.ai> Co-authored-by: Cyrus Leung <tlleungac@connect.ust.hk>
Overview
Fixes #62121. Apply sender-cache invalidations between generation renders on the single-worker multimodal executor so an engine cache-miss report cannot remove an entry between the processor's cache lookup and merge.
Claims
Expected a cached itemassertion; already-queued renders consume pending invalidations before looking up cached data.featurestraffic that never invokes the renderer.Validation
Current review cleanup (
20f04478): both helpers return when the cache is absent; enqueue also returns for empty input or an empty filtered pending set. The output handler delegates empty-list handling to the helper while keeping its outer cache guard and enqueue-before-publish ordering.pytest -p pytest_skipgc tests/multimodal/test_cache.py tests/renderers/test_warmup.py tests/renderers/test_executor_replace.py -k "not sleep" -q→ 57 passed, 1 deselected. The cleanup opt-out is described below; test assertions were unchanged.895c3a49: the unmodifiedpytest tests/multimodal/test_cache.py::test_sleep_wake_preserves_mm_cache_consistency -s -vv -ra→ 1 passed, 0 skipped. Logs confirm both generations and the intervening sleep/wake; sleep freed 38.05 GiB. Python sources were verified against20f04478; native artifacts were reused from official wheel7b6310c1with PyTorch 2.14.0+cu130 / Transformers 5.19.0, and all model-file hashes matched the pinned revision.actionlintwas excluded because its local environment could not initialize; no workflow files changed.git diff --checkpassed.The offline processor performance measurements compare
a0e7137cwithd2097ba5, not this cleanup: initial benchmark, three-repeat 1,024-prompt results. These synchronous measurements do not cover async miss-notification contention.Coalescing implementation (
d2097ba5), earlier macOS arm64 CPU validation, Python 3.13.15 / PyTorch 2.14.1:pytest -p pytest_skipgc tests/multimodal/test_cache.py -k "not sleep" -q: 29 passed, 1 deselected. Includes LRU, SHM/IPC, the original forced-interleaving regression, coalescing/clear behavior, and a multi-hash failure with concurrent notifications.pytest -p pytest_skipgc tests/renderers/test_warmup.py tests/renderers/test_executor_replace.py -q: 28 passed.pytest_skipgcplugin applies the repository'sskip_global_cleanupmarker to avoid the previously observed macOS torch teardown crash; it does not modify test assertions. SHM tests ran with local shared-memory access enabled.actionlintwas skipped; no workflow file changed.git diff --checkpassed.dc1a962queue/drain methods reproduced the backlog: 1,000 absent hashes retained 1,000 records before and 0 after; 200 notifications for one cached hash retained 200 before and 1 after. The next render resent the payload in both cases. This was not a full baseline-tree pytest run.The original assertion regression previously failed on pristine
a0e7137cand passed on submitted commitdc1a962. That submitted implementation also has Linux CPU validation (27 passed, 1 deselected) and A800 serving evidence below. The historical HTTP stress measurements below remain specific todc1a962. Thed2097ba5offline benchmarks and20f04478sleep/wake test do not replace a rerun of that HTTP stress workload.Historical A800 serving comparison for submitted commit dc1a962
Qwen2-VL-2B @ 895c3a49, A800 80GB PCIe, localhost HTTP using the reporter's pause/resume plus background-image load, no server-side hook. Five-minute trigger and two-minute control per arm:
Twelve fixed public-image responses were byte-identical for text, finish reason and completion-token count. Remaining errors were the existing P1-miss class; internal-error lines increased from 344 to 369 (1.07% to 1.16%). No overall error-rate improvement is claimed.
Details
The output handler previously invalidated the sender cache on the asyncio thread while the renderer performed its lookup and merge on its single-worker multimodal executor.
The handler now records missed hashes before publishing the associated outputs. Only hashes currently present in the sender cache are retained. When a processor cache is present, a short lock protects a pending set; each render swaps that set before its first lookup. Both helpers return immediately when there is no cache. Enqueue also returns for empty input or when filtering leaves no live hashes, before acquiring the lock. Membership checks and cache invalidation execute outside the lock. Failed invalidation restores the current and remaining hashes without discarding newly arrived notifications. Cache clear retains its existing behavior; stale notifications become no-ops at the next render boundary.
This avoids a second executor job and queue/set mirrored state. Pending records can briefly outlive the cache entry they reference, so this does not claim that their count always equals the instantaneous live cache size. Sync LLM retains direct invalidation.
This is distinct from #34749's assertion-to-error conversion and #61656's endpoint retries. P1 drift itself, automatic HTTP retries, and #62123's SHM handle issue are out of scope. The pre-existing pooling/embedding cross-executor race is also not addressed by this fix. The issue came from a public report and was investigated with constructed reproductions; it was not encountered in the submitter's production workload. AI assistance was used for investigation, implementation and validation, including this follow-up review.
Pull Request Checklist
/pr-checklistskill.