Repository navigation
Conversation
|
This PR appears to belong to: docs/design/module/profiling.md, docs/design/module/entrypoints.md, docs/design/module/observability.md. Module owners: @alex-jw-brooks @linyueqian @gcanlin Routing: @alex-jw-brooks via module of the changed files, semantic router, CODEOWNERS; @linyueqian via module of the changed files, semantic router, CODEOWNERS; @gcanlin via module of the changed files @NickCao, 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 Direct under experiment |
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
PR description
Realtime /v1/realtime turn-based auto truncation no longer estimates token budget with Qwen3-Omni-specific audio math. It builds the chat prompt, renders it through the engine renderer with max_total_tokens=None, and truncates conversation history until the rendered prompt_token_ids length fits the model budget. The same rendered EngineInput is then passed to engine.generate, so admission and execution share one token count. A new CPU unit test pins truncation driven by renderer expansion rather than raw text tokens.
Change flow
flowchart TD
A["[EXISTING] response.create handler"]:::existing
B["[CHANGED] cancel then _prepare_engine_input_with_auto_truncation"]:::changed
C["[NEW] _render_engine_input via render_cmpl_async"]:::new
D["[CHANGED] generate uses rendered EngineInput"]:::changed
E["[NEW] test_realtime_prompt_preflight"]:::new
A --> B
B --> C
C --> D
E -.-> C
classDef existing fill:#e5e7eb,stroke:#6b7280,color:#111827
classDef changed fill:#fef3c7,stroke:#d97706,color:#451a03,stroke-width:2px
classDef new fill:#dcfce7,stroke:#16a34a,color:#052e16,stroke-width:2px
classDef removed fill:#fee2e2,stroke:#dc2626,color:#450a0a,stroke-width:2px
CI at
2ce95e6014a5(2026-09-29T20:10:57.237082+00:00): required check(s) blocking:buildkite/vllm-omni(missing).
See inline comments below.
🤖 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!
Omni ReviewBot: finding feedback[p1] Cancels in-flight response before admission can fail —
This finding appears in the bot's COMMENT review, but GitHub could not place it as an inline diff comment. If you are the PR author and disagree, react 👎 to this comment. The disagreement will be shown to the maintainer; it does not approve or merge the PR. |
Omni ReviewBot: finding feedback[p1] PR Test Plan is a subset of the covering CPU entrypoints guard — Changed production path This finding appears in the bot's COMMENT review, but GitHub could not place it as an inline diff comment. If you are the PR author and disagree, react 👎 to this comment. The disagreement will be shown to the maintainer; it does not approve or merge the PR. |
…ation Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Nick Cao <ncao@redhat.com>
Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Nick Cao <ncao@redhat.com>
Run prompt preflight before cancelling an in-flight response so a rejected response.create leaves the current generation intact. Add a handler regression for an over-limit replacement. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Nick Cao <ncao@redhat.com>
88ff5eb to
b5b7f27
Compare
| max_model_len = getattr(self.engine.model_config, "max_model_len", None) | ||
| persistent = response.input is None | ||
| items: list[Any] = s.items if response.input is None else response.input | ||
| prompt = await self._build_full_prompt( |
There was a problem hiding this comment.
I don't think this will work generically, because_build_full_prompt still hardcodes AUDIO_PLACEHOLDER for qwen3omni. The render engine input eventually applies vLLM's multimodal processor (i.e., wraps around this), so because of that, it wont be able to do the multimodal token expansion correctly
| instructions=response.instructions, | ||
| items=items, | ||
| ) | ||
| engine_input = await self._render_engine_input(prompt) |
There was a problem hiding this comment.
Have you benchmarked this? Since rendering is a lot more complex than the arithmetic from before and will redo a lot of the multimodal processing, I think this + rerunning rendering later on will actually cause a lot of scaling issues
| parts_text = [] | ||
| for part in item.content: | ||
| if part.type == "input_audio" and part.audio: | ||
| parts_text.append(AUDIO_PLACEHOLDER) |
There was a problem hiding this comment.
Not sure if it belongs in this PR or separately since things are already hardcoded, but this should probably use get_placeholder_str, because it's implemented on SupportsMultimodal here. So we should be able to look up the unexpanded placeholder for any modality through that, e.g., for qwen3omni it's this
| instructions=response.instructions, | ||
| items=items, | ||
| ) | ||
| engine_input = await self._render_engine_input(prompt) |
There was a problem hiding this comment.
Similar to above, if we're calling rendering, I think it would be more ideal to try to reuse previous rendering. E.g., from the first render of everything, we should have the spans of expanded audio placeholders, and from there, be able to determine what needs to drop without iterative re-rendering with the caveat that things need to be correctly updated after dropping stuff unless you do one final render (since mm placeholders etc will be stale)
|
Replaced by #8339 |
Purpose
So it works with all models not only qwen3 omni.
Test Plan
vLLM Version: 0.30.0
vLLM-Omni Commit: b5b7f27
Test Result