Repository navigation
Conversation
97b0883 to
84e7067
Compare
DarkLight1337
left a comment
There was a problem hiding this comment.
This still feels a bit complicated. Let's just target common endpoints for now and retry the prompts imperatively instead of passing function objects into AsyncLLM
|
Thanks for the guidance. Before I rework the PR, I want to confirm the intended scope and retry boundary. My plan is to:
Does this match what you had in mind? Also, should /v1/responses be included in the initial set of common endpoints? |
|
Yep sounds good |
Retry Chat Completions, Completions, and OpenAI Embeddings once at the serving layer by rerendering with skip_mm_cache=True. Keep AsyncLLM responsible only for request cleanup and cache-miss propagation, and return a retryable 503 if recovery is unavailable or exhausted. Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: linchaoying6 <linchaoying6@jd.com>
04da796 to
3df1760
Compare
|
@DarkLight1337 Thanks — I implemented the confirmed simpler design and force-updated this PR from the latest
Validation: 11 focused tests passed, every changed-file hook passed, and a B200/Qwen2-VL HTTP E2E induced a real P1 miss on the second identical request; the endpoint logged its retry and returned HTTP 200 with identical output. The new head is |
|
@claude review |
| except MultiModalCacheMissError: | ||
| if q is not None: | ||
| await self.abort(q.request_id, internal=True) | ||
| if self.log_requests: | ||
| logger.info("Request %s failed (multi-modal cache miss).", request_id) | ||
| raise |
There was a problem hiding this comment.
🔴 Endpoints without the new retry wrapper now get a raw MultiModalCacheMissError (a RuntimeError) instead of an output with finish_reason="error". Streaming Responses, beam search and speech-to-text lose their error handling. The output-processor branch fires for every AsyncLLM request with a queue. AsyncLLM re-raises the miss unconverted at both sites. Fix: have AsyncLLM raise a RetryableRequestError that wraps the miss, or a subclass of it the retry wrappers can catch, so every consumer gets a GenerationError. This covers both sites (async_llm.py:807, async_llm.py:1072).
Why this was flagged
The trigger is a P0/P1 multi-modal cache drift on any AsyncLLM request that does not come from chat or completion. Examples are Responses (responses/serving.py:665), beam search (beam_search/online.py:90) and the decisions encode calls. The new branch at output_processor.py:749 now puts MultiModalCacheMissError on the queue whenever req_state.queue is not None. AsyncLLM then re-raises it unchanged at async_llm.py:807/1072. On the base branch these callers got a RequestOutput with finish_reason "error", or a pooling output with a retryable error. They turned that into a GenerationError and returned a 500, or an SSE error event. Now streaming Responses only catches GenerationError (responses/serving.py:1395), so the RuntimeError escapes and the SSE stream is cut off with no error event. Non-streaming paths return 500 instead of the 503 the PR description promises for ineligible paths.
Verification: output_processor.py:749-771 puts MultiModalCacheMissError(...) on the queue and does continue, so no RequestOutput with finish_reason="error" is produced. async_llm.py:807 and 1072 re-raise it unchanged as a plain RuntimeError (multimodal/cache/base.py:33). Streaming Responses catches only GenerationError at serving.py:1395, so the SSE stream is cut off with no error event.
| except MultiModalCacheMissError as e: | ||
| if not retry_cache_miss or attempt: | ||
| raise RetryableRequestError( | ||
| "Multi-modal processor cache miss." | ||
| ) from e |
There was a problem hiding this comment.
🔴 Multimodal score and rerank requests on the flash late-interaction path now get HTTP 500 on a processor-cache miss, where the base branch returned a retryable 503. The conversion to RetryableRequestError only happens in PoolingBaseServing.__call__ (serving.py:84-88). ServingScores.__call__ skips it when enable_flash_late_interaction is set, yet still runs _prepare_generators/_collect_batch. Fix: move the MultiModalCacheMissError to RetryableRequestError mapping into _collect_batch, so every caller of it gets a 503, flash scoring included, and keep the retry loop in __call__.
Why this was flagged
The trigger is a P0/P1 multimodal cache drift during a /score or /rerank request on a late-interaction model with enable_flash_late_interaction on. scoring/serving.py:69-73 sends these requests to flash_late_interaction, which calls _prepare_generators and _collect_batch directly (scoring/serving.py:269-270, 312-313). It never goes through the new try/except in PoolingBaseServing.__call__. With this change, output_processor.py:749-771 puts a MultiModalCacheMissError on the queue, and AsyncLLM.encode (async_llm.py:1072-1077) re-raises it. On the base branch the miss arrived as an output carrying a retryable RequestError, and _collect_batch (base/serving.py:228-230) turned it into RetryableRequestError, which is HTTP 503 ServiceUnavailableError. Now the error is a raw RuntimeError subclass (multimodal/cache/base.py:33). create_error_response falls through to its else branch and returns 500 InternalServerError, so clients that retry on 503 stop retrying.
Verification: The only place that converts the MultiModalCacheMissError to RetryableRequestError is the new try/except in PoolingBaseServing.__call__ (base/serving.py:84-88). ServingScores.__call__ (scoring/serving.py:69-73) skips it when enable_flash_late_interaction is set. So the error falls through to the else branch of create_error_response: 500 InternalServerError instead of the base's retryable 503.
|
@DarkLight1337 Thanks for triggering the review. After tracing both findings, I agree that using an exception from AsyncLLM as the cache-miss control signal changes the contract for every AsyncLLM consumer. In particular, beam search expects a terminal I would therefore like to revise the design as follows:
This is a somewhat larger output-contract change than the exception approach, but it keeps a cache miss as a request-level result rather than a generator failure and avoids regressions in existing consumers. Does this revised structured-output direction look acceptable before I implement it? |
|
cc @njhill since he's in charge of managing the request outputs |
Keep AsyncLLM output-based error semantics while common serving paths retry structured transient failures once. Preserve retry codes so multimodal cache misses re-render with the sender cache bypassed. Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: linchaoying6 <linchaoying6@jd.com>
|
This pull request has merge conflicts that must be resolved before it can be |
Overview
Propagates multi-modal processor-cache drift as a structured request result
instead of changing the
AsyncLLMgenerator exception contract. Serving pathscan then retry a transient request once while other consumers retain their
existing output-based error handling.
This supersedes the earlier exception-based implementation in this PR and
addresses both review findings without passing replay callbacks into
AsyncLLM.Claims
RequestOutput.error: RequestError | None, aligned with the existingPoolingRequestOutput.errorcontract.finish_reason="error"plusRequestError(code="multimodal_cache_miss", retryable=True, ...).n > 1parent request and emit one structuredterminal error to the parent collector.
Multi-modal cache misses re-render the retained source request with
skip_mm_cache=True; other retryable errors re-render normally._collect_batchas the common pooling error boundary and retry itsRetryableRequestErroronce in the ordinary pooling serving path. Thiscovers OpenAI and Cohere Embeddings while preserving the retryable 503
mapping for direct
_collect_batchconsumers such as flash score/rerank.RetryableRequestError; non-streaming HTTPrequests receive 503.
Validation
Focused regression tests on macOS arm64 / Python 3.12:
Changed-file checks:
Real HTTP end-to-end validation ran on one NVIDIA B200 with PyTorch
2.14.0+cu130. A temporary test-only receiver-cache hook removed the P1 itemimmediately before the second identical request first read it:
/v1/chat/completions/v1/embeddings/v2/embedAll three runs logged
E2E_INJECT_P1_CACHE_MISSfollowed by the enginemulti-modal cache-miss warning. The temporary hook and model symlinks were
removed, both test services were stopped, and both B200 devices returned to
0 MiB allocated memory.
Model evaluation is not applicable: this changes recovery after an internal
cache miss, not weights, decoding, or successful-request output semantics.
Details
P0 keeps a metadata-only shadow of the P1 multi-modal cache. On a P0 hit, the
rendered engine input may omit the tensor. If P1 has lost that item, replaying
the same rendered input cannot recover. The engine therefore invalidates the
stale P0 hash and emits a structured retryable error. Eligible serving paths
retain the source request, rebuild it, and bypass the sender cache only for the
multi-modal cache-miss retry.
Keeping the miss in
RequestOutputavoids changing theAsyncLLMcontract forResponses, Batch, Beam Search, Speech-to-Text, scale-out, and other consumers.
They continue to receive terminal output with
finish_reason="error"and canapply their existing error policy.
Why this is not duplicate work
#61645 was the earlier external/client-retry alternative and was closed in
favor of this maintainer-requested endpoint-owned direction. A current open-PR
search found no other implementation of this recovery path; #61656 is the
existing PR being updated, not a new duplicate.
AI assistance disclosure: OpenAI Codex assisted with implementation, focused
tests, code review, B200 end-to-end validation, and PR preparation. The human
submitter reviewed and approved the implementation and validation before this
update.
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: Cache misses remain request-level output, so unrelated
AsyncLLMconsumers keep their established contract.Testing and Validation: 99 focused tests, changed-file checks, and real B200 HTTP recovery for generation and both embedding protocols passed.
Code Quality and Style: Applicable hooks pass and the retry remains bounded to one attempt.
Pull Request Contents: Root cause, behavior, scope, validation, cleanup, non-duplication, human approval, and AI assistance are documented.
BEFORE SUBMITTING, PLEASE READ https://docs.vllm.ai/en/latest/contributing