Repository navigation
Conversation
|
This PR appears to belong to: docs/design/module/input_output_modality_contracts.md, docs/design/module/ar_runtime.md, docs/design/module/cache_management.md. Module owners: @tzhouam @alex-jw-brooks @amy-why-3459 Routing: @tzhouam 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 @0z5a, 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. |
MrlixiangWE
left a comment
There was a problem hiding this comment.
One blocking compatibility issue remains in the vLLM 0.29 fallback. Separately, #7820 rewrites these same two scheduler call sites for the vLLM 0.30 API and currently conflicts with this branch. Please state the intended landing order and rebase or supersede this compatibility layer accordingly so conflict resolution does not restore the broken 0.29 fallback.
| """Support both manager APIs without requiring scheduler stubs to bind a helper.""" | ||
| if hasattr(type(manager), "accept_tokens"): | ||
| return bool(manager.accept_tokens(request, new_token_ids)) | ||
| if not manager.should_advance(request): |
There was a problem hiding this comment.
The vLLM 0.29 fallback needs to preserve the reasoning-aware contract used by the released scheduler. That path calls should_advance(request, new_token_ids=...), then trim_reasoning_for_advance, and only feeds the non-empty post-reasoning suffix to the grammar. Here the no-argument gate can derive the wrong delta after async/spec rejection, and even when it detects the boundary, forwarding the full block includes the reasoning prefix and end marker. A speculative block ending with [..., </think>, {] can therefore be rejected, after which the AR scheduler marks the request FINISHED_ERROR. Please pass the sampled delta, trim before acceptance, treat an empty suffix as accepted, and add a vLLM 0.29 boundary regression through the production caller; the current Boolean mocks do not exercise this case.
There was a problem hiding this comment.
Fixed in 92be014: forward new_token_ids, trim the reasoning prefix before grammar acceptance, and accept an empty suffix. Production AR caller regressions cover mixed reasoning-end/JSON tokens and an end-marker-only delta. This should land before #7820; its 0.30 upgrade can then use native accept_tokens and remove the 0.29 fallback while retaining the boundary coverage.
|
Validation for 92be014 (before: ed8016c): vLLM 0.29.0, full MiniCPM-o-4_5 three-stage pipeline, two L20s (Stage 0 on one card; Stages 1/2 on the other). Fresh A/P/P/A processes, 7 JSON requests each, first request per process excluded: 12 measured requests per variant. All 28 outputs are valid JSON and match exactly (11 tokens).
This is a correctness fix; the small timing difference on a shared host is not evidence of a meaningful speed improvement. All four processes completed. This replaces the earlier single-pair result; interrupted attempts are excluded. Regression: the original helper fails both reasoning-boundary cases; the fix passes all three cases through the production AR caller. Core/worker CPU suites: 557 passed, 1 skipped on 0.29.0; 554 passed, 4 skipped on the development build. Other pre-commit checks passed; mypy matches the original head’s 57 existing diagnostics, with no new diagnostics. |
Signed-off-by: 0z5a <dezhen.lu@student.uni-tuebingen.de>
Signed-off-by: 0z5a <dezhen.lu@student.uni-tuebingen.de>
8e03b43 to
f3cec17
Compare
Signed-off-by: 0z5a <dezhen.lu@student.uni-tuebingen.de>
f3cec17 to
0cdb876
Compare
Signed-off-by: 0z5a <dezhen.lu@student.uni-tuebingen.de>
Signed-off-by: 0z5a <dezhen.lu@student.uni-tuebingen.de>
0cdb876 to
6c10baa
Compare
Signed-off-by: 0z5a <dezhen.lu@student.uni-tuebingen.de>
Signed-off-by: 0z5a <192209249+0z5a@users.noreply.github.com>
Signed-off-by: 0z5a <192209249+0z5a@users.noreply.github.com>
Current A100 validation at
719bcf95b0f8(2×A100 40GB; Torch 2.13.0+cu130, vLLM 0.30.0).The installed vLLM 0.30 manager API passed seven cases; three legacy-API cases were skipped. Full BAGEL JSON-schema requests and high concurrency remain pending.
The instance was externally stopped during the remaining test queue. All completed results above were saved off-instance; unfinished measurements remain pending.
Purpose
Keep structured-output advancement compatible with both vLLM manager APIs: vLLM 0.30.0 supplies
accept_tokens; vLLM 0.29.0 usesshould_advance, reasoning-prefix trimming and the request grammar. Forward the sampled token delta, accept an empty post-reasoning suffix and preserve AR grammar rejection.This branch is rebased after merged #7820. Current main already calls the shared grammar-rejection method from both schedulers; this patch updates that method only. Scheduler test stubs with an instance-bound
accept_tokensremain supported.Validation
tests/core/schedThe same three timeout/log-capture tests fail on main and candidate in the vLLM 0.29.0 host environment. The full Qwen3-VL run used the same GPU and memory budget for both variants: seven greedy schema requests each, 7/7 valid identical JSON outputs per variant, first request excluded from timing. The installed engine is 0.29.0, so both runs used the same 0.29-compatible Omni source and the candidate received this PR's exact helper and scheduler dispatch. The complete rebased 0.30.0 source tree was not started in that environment.
The earlier PR head
92be0140completed a real three-stage MiniCPM-o-4_5 run on two L20s, with 28/28 valid identical JSON outputs. Neither timing difference on these shared hosts establishes a useful speed improvement. See task-local evidence for the measurement details. This PR stays draft.