Repository navigation
[Core] Add request-scoped producer state and fail processor exceptions - #8530
PeaceMaker-best wants to merge 1 commit into
Conversation
Signed-off-by: PeaceMaker-best <221849497+PeaceMaker-best@users.noreply.github.com>
|
This PR appears to belong to: docs/design/module/omni_connector.md, docs/design/module/input_output_modality_contracts.md, docs/design/module/diffusion/parallelism.md. Module owners: @princepride @fake0fan @natureofnature Routing: @princepride via module of the changed files, semantic router, CODEOWNERS; @fake0fan via module of the changed files, semantic router; @natureofnature via module of the changed files, semantic router @PeaceMaker-best, 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 routing recordAssigned Strict on cursor (cursor-grok-4.6-high) under experiment |
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
3 actionable finding(s).
CI at
81841c64429c(2026-10-10T12:38:09.181132+00:00): required check(s) blocking:buildkite/vllm-omni(missing).
Full review analysis
Scan:
| Category | Result |
|---|---|
| Tests / verification | 3 finding(s) below |
| Security | no finding reported |
| Docs / comments | no finding reported |
| Behavior / compatibility | no finding reported |
| Correctness | no finding reported |
Validated:
- [resolved] Author flagged #5556 empty-terminal-after-processor-exception vs this Draft’s no-put failure. Residual not named: Audex thinker2code2wav_async_chunk raises on zero codec expecting the adapter to still deliver the terminal marker (audex.py:211-216), wired through async_chunk=true (audex_tts.yaml:23, stage_config.py:1029-1030).
- [resolved] sender_token is None skips accessor install (chunk_transfer_adapter.py:816); save_async:479-482 always attaches a token on the enqueue path. Residual: hand-built tasks in older tests still invoke processors without request_state.
- [resolved] audex
thinker2code2wav_full_payloadreturns empty terminal instead of raise (audex.py:266-273); serving_speech.py:108 still holds. Residual: processors that raise. - [claim-verified] Whole-request terminal success pops payload via cleanup_sender->_clear_sender_state_locked request_payload.pop; failed put keeps it until finish_requests.
- [claim-verified] At this head no model processor is migrated; RFC #6453 is not closed. Adjacent #5556 empty-terminal alignment is still open.
- [claim-verified] 163-passed / GPU-correctness-not-claimed: cannot re-run pytest in this read-only step; command below is the author's CPU contract.
Keep two major findings on the PR’s primary change and one minor test-integrity fix: (1) the new processor except/return skips the empty terminal connector.put, while _log_failed_chunk_sends only logs, so raising producers (IndexTTS2, PersonaPlex async_chunk, and any other hooked custom_process_next_stage_input_func) park the consumer until the 600s input deadline rather than aborting; (2) is_active treats sender_token.cancelled as unavailability even though the send path still runs a cancelled-but-current terminal flush, so the first get_or_create on that path raises, skips put, and drops EOS — fence on generation identity only and add a CPU test. Also keep the vacuous post-exit adapter.request_state assert in test_processor_exception_fails_request_without_empty_terminal_send as a minor rewrite. Drop naming polish, resolved Audex/sender_token-None notes, and the no-issue CI/legacy/docs checks.
Verdict: REQUEST CHANGES
Findings
- **[P1] This hunk installs RequestStateAccessor with is_active
lambda: self._sender_to…** —vllm_omni/distributed/omni_connectors/transfer_adapter/chunk_transfer_adapter.py`
Evidence for This hunk installs RequestStateAccessor with is_active `lambda: self._sender_to…
This hunk installs RequestStateAccessor with is_active lambda: self._sender_tokens.get(external_req_id) is sender_token and not sender_token.cancelled, while _send_single_request still invokes the producer for a current generation when the task is terminal (cancelled does not veto a terminal chunk; if is_current and (is_terminal_task or not sender_token.cancelled)). cleanup_sender sets cancelled = True / cleanup_deferred = True without popping the token or request_payload, so a queued EOS can outlive cancellation. Combined with the new except/return, a producer that calls get_or_create on that cancelled-but-current flush raises RequestStateUnavailableError (request_state.py: if not self._is_active()), skips connector.put, and leaves the downstream stage in WAITING_FOR_CHUNK without EOS. Fence retained views with generation identity only (_sender_tokens.get(external_req_id) is sender_token); cancelled-but-current must remain readable for the in-flight terminal flush. Add a CPU test: terminal_pending + cleanup_sender cancel, producer get_or_create, assert put still runs.
Evidence: chunk_transfer_adapter.py:717-723 cancelled does not veto a terminal chunk / if is_current and (is_terminal_task or not sender_token.cancelled); cleanup_sender.py:1022-1033 sets cancelled = True and cleanup_deferred = True without popping the token or request_payload; request_state.py:59-60 / 74-75 raise when not is_active. No adapter test covers get_or_create on that cancelled-but-current terminal invocation.
Suggestion: lambda: self._sender_tokens.get(external_req_id) is sender_token,
🤖 This review was generated by InferMatrix Copilot, an open-source repo-maintenance agent for PR review, CI debugging and issue triage. Try it on your own repo, and ⭐ star it if it helped!
|
|
||
| except Exception as e: | ||
| logger.error(f"Failed to use custom_process_input_func for payload extraction: {e}") | ||
| self.record_send_failure(request.request_id, f"Stage input processor failed: {e}") |
There was a problem hiding this comment.
[P1] This hunk adds self.record_send_failure(...); return on every `custom_process…
Evidence and suggested fix
This hunk adds self.record_send_failure(...); return on every custom_process_next_stage_input_func exception, so a raise never reaches the payload_data is None fill-in that used to connector.put an empty terminal OmniPayloadStruct. That is not a scheduler-visible abort: omni_scheduler_mixin.py _log_failed_chunk_sends only logs (This only reports. Failing the request from here does not work); the consumer stays in WAITING_FOR_CHUNK until VLLM_OMNI_INPUT_WAIT_TIMEOUT_S (600s, or forever if 0). Audex's sync hook already returns an empty finished payload, so it is unchanged; IndexTTS2 talker2s2mel_full_payload and PersonaPlex async_chunk still raise, and every other hooked producer inherits the stall. test_processor_exception_fails_request_without_empty_terminal_send only checks collect_failed_send_request_ids and calls finish_requests itself. Do not land this protocol change with the namespaced-state API unless you still put the terminal marker on is_finished/is_segment_finished after recording, or finish_requests producer and consumer like _process_chunk_receive_failures.
Evidence: chunk_transfer_adapter.py:828-829 self.record_send_failure(...); return skips :833-838 empty-terminal fill-in. omni_scheduler_mixin.py:558-564 "This only reports. Failing the request from here does not work". chunk_transfer_adapter.py:717-720 "dropping it leaves that stage parked in WAITING_FOR_CHUNK until the input deadline". indextts2.py:214 raise ValueError(f"IndexTTS Stage 0 payload must be a dict...") on this hook.
Suggestion: self.record_send_failure(request.request_id, f"Stage input processor failed: {e}")
if not (is_segment_finished or is_finished):
return
| adapter.finish_requests(["internal"], RequestStatus.FINISHED_ERROR, {"internal": request}) | ||
| assert "external" not in adapter.request_payload | ||
| with pytest.raises(RequestStateUnavailableError): | ||
| _ = adapter.request_state |
There was a problem hiding this comment.
[P2] This diff's `test_processor_exception_fails_request_without_empty_terminal_send…
Evidence and suggested fix
This diff's test_processor_exception_fails_request_without_empty_terminal_send ends with with pytest.raises(RequestStateUnavailableError): _ = adapter.request_state. The new request_state property raises whenever _processor_state_context.accessor is None, and _send_single_request's finally always clears it on producer exit, so that assertion cannot fail even while request_payload still holds namespaces (the test already asserts "external" in adapter.request_payload before a manual finish_requests). Capture the accessor inside the producer and assert get_or_create on that retained view, as test_namespaced_producer_state_terminal_commit already does.
Evidence: chunk_transfer_adapter.py:830-831: finally: self._processor_state_context.accessor = None. chunk_transfer_adapter.py:243-245: if accessor is None: raise RequestStateUnavailableError(...). test_chunk_transfer_adapter.py:64-66: view, identity = views[0] / view.get_or_create(identity, "late", list).
Omni ReviewBot: finding feedback**[p1] This hunk adds See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement. |
|
Thanks for the review. Author self-review at
The revision should keep the namespaced-state API additive, preserve the existing terminal-marker protocol, and leave orchestrator-level error propagation to separately aligned work. It needs regressions for processor exceptions at terminal boundaries, the cancelled-current terminal flush, and an accessor captured inside the producer and exercised after cleanup. The previously reported 163 CPU passes cover the submitted accessor/adapter tests; they do not establish end-to-end error termination and missed these cases. These issues are still present in the current head, so this proposal needs changes before merging. Pre-commit/build/DCO passing does not resolve them. AI assistance: Codex helped inspect the cited production paths and draft this response; no code revision is claimed here. |
Omni ReviewBot: no human activity for 7 days@PeaceMaker-best this pull request has had no human commit, comment or review since 2026-10-10. Please confirm the current plan and next step. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
Purpose
Incremental producers currently keep raw mutable state in
request_payload.This core candidate for #6453 adds a typed, namespaced accessor backed by that
same container and fenced by the existing sender generation. A retained view,
including a factory overlapping cancellation, cannot recreate a retired
request's state. Whole-request cleanup remains runtime-owned; segment completion
retains request-wide state, and legacy tensor/dict entries remain supported.
Processor exceptions now enter the existing scheduler-visible failure path
instead of silently becoming empty terminal sends. Failed connector sends keep
state until failure finalization, while successful terminal sends release every
namespace through canonical cleanup.
Related to #6453. This implements the proposed generic core with a synthetic
producer; it does not close the entire RFC or migrate real models. The API and
failure semantics still need maintainer alignment. Proposed publication: Draft.
Adjacent work: #5556 implements model-specific LLaMA-Omni2 integration and
#6464 saves benchmark stage snapshots. Neither adds this generic namespaced
accessor. In particular, #5556 currently tests an empty terminal marker after a
processor exception; this Draft proposes an explicit scheduler-visible failure
instead. Those failure semantics need alignment before merging.
Test Plan
vLLM Version: official 0.31.0+cpu wheel, CPU PyTorch 2.13.0,
Transformers 5.14.1, Python 3.12.3, Ubuntu 24.04 Docker environment.
vLLM-Omni Commit: base
9146284c16abbe7309f3f79dfdacfb48cdf2499a;implementation
81841c6.CPU command run with normal repository imports and pytest fixtures:
python -m pytest tests/distributed/omni_connectors/test_request_state.py tests/distributed/omni_connectors/test_chunk_transfer_adapter.py -m 'core_model and cpu' -q -p no:cacheprovider --basetemp=/tmp/pytestTest Result
real vLLM runtime and normal repository pytest entrypoint. Includes the new
contract tests and existing chunk-transfer adapter regressions. Connectors are
mocked by the repository's normal fixture; no production module was replaced.
Warnings: TorchScript deprecation and the development checkout's version
fallback. No GPU models or weights were loaded.
terminal commit, raising connectors, processor errors, buffered no-output,
segment retention, canonical finalization, reuse and stale accessor fencing.
marks and Markdown checks passed. New core/test files pass mypy 1.11.1 for
Python 3.10.
files. The same-environment upstream adapter baseline has 34 diagnostics;
this branch has 33. Source-line comparison confirms no new diagnostics and
removal of the existing
request_payloadannotation error. The remainingadapter errors are inherited. Full-file mypy is not green; no full pre-commit
or upstream CI success is claimed.
AI disclosure: implementation, tests and this description were prepared with
Codex. The contributor confirmed this Draft submission after receiving the
patch and validation evidence. Maintainer alignment remains outstanding before
marking this ready for review.