Repository navigation
[Rebase] Rebase to vllm 0.28.0 - #6606
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
This PR appears to be related to model: minicpm. Model owners: @y-null @tzhouam, 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. |
f035d40 to
8191ddf
Compare
…tion Review finding (P1, GPT-5.6-sol xhigh review of #6606): the make_request_output override accepts ec_transfer_params — added so the 6th positional from upstream process_outputs would not misroute — but every construction path dropped it: the pooling delegation and the normal path forwarded only kv_transfer_params, and _new_request_output had no parameter for it on either branch. vLLM v0.28.0 forwards the field end to end (v1/engine/output_processor.py -> RequestOutput .ec_transfer_params), so with an encoder-cache connector enabled the engine's transfer handle reached omni and died there — the frontend saw ec_transfer_params=None and remote encoder-cache transfer broke. All three paths now forward it (pooling delegation, upstream-delegating _new_request_output, and the no-detokenizer generation-stage direct RequestOutput build). Regression test covers the two construction paths. Also fixes the stale Dockerfile.ci header (P3 from the same review): it still described the v0.27.0 pin==base no-op era; the pin now lands on the v0.28.0 release tag on top of the v0.27.0 base image, wheel decides the vLLM. Signed-off-by: tzhouam <tzhouam@connect.ust.hk> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Gaohan123
left a comment
Gaohan123
left a comment
There was a problem hiding this comment.
Please also modify the related installation docs. Thanks
|
|
||
| @pytest.mark.slow | ||
| @pytest.mark.diffusion | ||
| @pytest.mark.advanced_model |
There was a problem hiding this comment.
The pre-commit hook (tools/pre_commit/check_test_marks.py) requires every test file it checks to carry a run-level marker; this file had slow/diffusion but no level mark, so the rebase's pre-commit gate flagged it. We added advanced_model (L3, run on each merge) to match the sibling e2e offline-inference suites. No functional change — happy to move it to a separate PR if you'd prefer the rebase stay minimal.
| inter_stage_outputs = getattr(model_runner_output, "inter_stage_outputs", None) | ||
| num_nans_in_logits = model_runner_output.num_nans_in_logits | ||
| kv_connector_output = model_runner_output.kv_connector_output | ||
| ec_connector_output = getattr(model_runner_output, "ec_connector_output", None) |
There was a problem hiding this comment.
[P1] Mirror v0.28's deferred KV-block free fence in the Omni scheduler overrides
vLLM 0.28 increments sched_step_seq for each non-empty scheduled step when defer_block_free is enabled, and then increments processed_step_seq and calls _drain_deferred_frees() at the beginning of update_from_output().
This override omits the latter half. Because OmniARScheduler.schedule() delegates to super().schedule(), sched_step_seq advances but processed_step_seq never does, so deferred blocks can accumulate indefinitely under async/PP KV-consumer configurations and eventually exhaust the KV cache.
OmniGenerationScheduler reimplements both schedule() and update_from_output() and currently omits both halves of the fence, which can instead allow blocks to be reused while an earlier GPU batch is still writing to them.
Please mirror the upstream v0.28 fence advancement and draining logic in both Omni schedulers.
There was a problem hiding this comment.
Fixed in 0f3876f. update_from_output now advances processed_step_seq and drains at the top (upstream's placement) in both Omni schedulers, and the generation fast-path schedule() advances sched_step_seq right before _update_after_schedule so last_sched_seq stamping observes the advanced value. Unit tests pin both halves plus the advance-before-stamping order.
| # EC Connector: update state from worker-side EC connector output. | ||
| # Use getattr for safety with test __new__/SimpleNamespace code paths. | ||
| if getattr(self, "ec_connector", None) is not None and ec_connector_output: | ||
| self.ec_connector.update_connector_output(ec_connector_output) |
There was a problem hiding this comment.
[P1] Complete the EC request-finish path before consuming worker-side connector output
Updating the scheduler-side connector from ec_connector_output is only one part of the v0.28 EC contract. Upstream _free_request() also calls ec_connector.request_finished(request) before freeing the encoder cache, honors its delayed-free result, and returns the generated ec_transfer_params.
OmniARScheduler._free_request() currently frees the encoder cache without calling this hook and always returns None as the second tuple item. The generation scheduler also discards the second value returned by _free_request(), and OmniSchedulerMixin._make_omni_engine_output() has no ec_transfer_params field.
Consequently, the output-processor changes in this PR still cannot receive the remote encoder-cache handle, and the connector's delayed-free requirement is ignored. Please preserve the full (kv_transfer_params, ec_transfer_params) result and propagate ec_transfer_params through OmniEngineCoreOutput in both schedulers.
There was a problem hiding this comment.
Fixed in 8705a8c. _free_request now fires ec_connector.request_finished(request) before the encoder cache is freed, ORs the delayed-free result into connector_delay_free_blocks, and returns ec_transfer_params on all return paths; both schedulers keep the second tuple item and _make_omni_engine_output threads it into OmniEngineCoreOutput (the field is inherited from upstream EngineCoreOutput), so the output-processor forwarding from this PR now receives the handle end to end. Tests cover hook order, delayed-free, and the plumbing in both schedulers.
| ) | ||
| prefill_stats = getattr(eco, "prefill_stats", None) | ||
| if prefill_stats is not None: | ||
| req_state.num_cached_tokens = prefill_stats.num_cached_tokens |
There was a problem hiding this comment.
[P2] Preserve num_cache_creation_tokens in the Omni output paths
vLLM 0.28 finalizes prefill statistics with PrefillStats.finalize(kv_cache_manager.estimate_cached_tokens(request)) before emitting them, and propagates the resulting num_cache_creation_tokens into RequestOutput.
Both Omni schedulers currently call request.take_prefill_stats() directly without finalizing the statistics. This no-detokenizer path then copies only num_cached_tokens, and the direct RequestOutput constructor also omits num_cache_creation_tokens.
As a result, prefix-cache creation usage will be reported as zero or None even when new cache entries were created. Please finalize the prefill statistics in both schedulers, copy prefill_stats.num_cache_creation_tokens into the request state here, and pass it to the direct RequestOutput constructor.
There was a problem hiding this comment.
Fixed in ee2ee48. Both schedulers finalize prefill stats with estimate_cached_tokens BEFORE stop handling (so the estimate is taken while the request is still known to the KV cache manager, matching upstream), the output processor copies num_cache_creation_tokens into the request state alongside num_cached_tokens, and the no-detokenizer direct RequestOutput build passes the field through. Tests include a lifecycle guard that fails if finalization runs after _free_request.
…tion Review finding (P1, GPT-5.6-sol xhigh review of #6606): the make_request_output override accepts ec_transfer_params — added so the 6th positional from upstream process_outputs would not misroute — but every construction path dropped it: the pooling delegation and the normal path forwarded only kv_transfer_params, and _new_request_output had no parameter for it on either branch. vLLM v0.28.0 forwards the field end to end (v1/engine/output_processor.py -> RequestOutput .ec_transfer_params), so with an encoder-cache connector enabled the engine's transfer handle reached omni and died there — the frontend saw ec_transfer_params=None and remote encoder-cache transfer broke. All three paths now forward it (pooling delegation, upstream-delegating _new_request_output, and the no-detokenizer generation-stage direct RequestOutput build). Regression test covers the two construction paths. Also fixes the stale Dockerfile.ci header (P3 from the same review): it still described the v0.27.0 pin==base no-op era; the pin now lands on the v0.28.0 release tag on top of the v0.27.0 base image, wheel decides the vLLM. Signed-off-by: tzhouam <tzhouam@connect.ust.hk> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
0d582c2 to
bb55fbd
Compare
The replayed 0.27-era commit carried literal conflict markers inside the duplex multi-session runner; main already ships the deferral in better form (_scenario_module() + lru_cache), so the file returns to main's version exactly. Signed-off-by: tzhouam <tzhouam@connect.ust.hk> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
pre-commit's SPDX hook enforces "Copyright contributors to the vLLM-Omni project" on files a change touches; these files still carried the pre-rename "vLLM project" form. Header lines only, hook-generated. Signed-off-by: tzhouam <tzhouam@connect.ust.hk> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tion Review finding (P1, GPT-5.6-sol xhigh review of #6606): the make_request_output override accepts ec_transfer_params — added so the 6th positional from upstream process_outputs would not misroute — but every construction path dropped it: the pooling delegation and the normal path forwarded only kv_transfer_params, and _new_request_output had no parameter for it on either branch. vLLM v0.28.0 forwards the field end to end (v1/engine/output_processor.py -> RequestOutput .ec_transfer_params), so with an encoder-cache connector enabled the engine's transfer handle reached omni and died there — the frontend saw ec_transfer_params=None and remote encoder-cache transfer broke. All three paths now forward it (pooling delegation, upstream-delegating _new_request_output, and the no-detokenizer generation-stage direct RequestOutput build). Regression test covers the two construction paths. Also fixes the stale Dockerfile.ci header (P3 from the same review): it still described the v0.27.0 pin==base no-op era; the pin now lands on the v0.28.0 release tag on top of the v0.27.0 base image, wheel decides the vLLM. Signed-off-by: tzhouam <tzhouam@connect.ust.hk> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The branch's PSNR pair was an INCOMPLETE revert: 078b3ff raised PSNR_THRESHOLD 27.0 -> 30.0 and added the SDPA fallback gate; the revert 484a878 removed the fallback but never restored the base threshold, leaving this branch 3 dB stricter than main ever since. Nightly evidence: build 2990 (this branch, head ad4f1b5) failed "Diffusion X2I(&A&T) · Accuracy Test" with PSNR below threshold for Qwen/Qwen-Image: got 29.950521, expected >= 30.000000 — inside the 27-30 dB band that only the accidental gate rejects; the same lane failed on build 2988 (old head, same 30.0) and passes on main's nightlies (27.0). Restoring 27.0 removes the divergence and the branch-only failure mode. Signed-off-by: tzhouam <tzhouam@connect.ust.hk> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The official vllm/vllm-openai:v0.28.0 image is out (Docker Hub, 2026-08-26), and our precompiled-wheel pin 2cf0a6915ce5 IS the v0.28.0 tag — the exact condition the file's own protocol names for retiring the reinstall block. Base image moves v0.27.0 -> v0.28.0 and the wheel-pin/reinstall machinery (vLLM wheel fetch, torch-before- flashinfer ordering, nvJitLink ldconfig, numpy re-pin) is removed: it existed only to compensate for a base image and a target vLLM that disagreed, and at the release boundary they agree again. No package drift: the v0.28.0 image is built from the same commit the wheel pin named, with torch==2.13.0 and flashinfer 0.6.16.post3 per its requirements/cuda.txt — the same versions the reinstall block pinned and CI has been green on. Header narrative records the 0.28 cycle for the next rebase. Signed-off-by: tzhouam <tzhouam@connect.ust.hk> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Requested in #6606 review: the installation docs still pinned 0.26.0 everywhere because the 0.27 rebase never touched them. Follows the v0.26.0 rebase precedent (cbab1d4) file-for-file: quickstart, installation index, CUDA and ROCm guides, and the pre-built wheels list. The same 0.27-era drift also left the RELEASE images stale — the exact gap the review loop caught: Dockerfile.cuda's default base was still vllm/vllm-openai:v0.26.0 and Dockerfile.rocm pinned v0.27.0, so the published vllm/vllm-omni:v0.28.0 image would have shipped vLLM 0.26. Both now pin v0.28.0, per the rebase-maintainer reminder embedded in Dockerfile.rocm itself and the cbab1d4 precedent (rebase PRs own these bumps; the 0.27 rebase simply missed them). Every pin names a real artifact: the vllm-0.28.0-cp38-abi3 wheel is on the GitHub release, wheels.vllm.ai/rocm/0.28.0/ holds rocm723 (ROCm 7.2.3, unchanged), and vllm/vllm-openai:v0.28.0 plus vllm/vllm-openai-rocm:v0.28.0 are both on Docker Hub. The wheels list gains 0.28.0 phrased as published-with-this-release (0.27.0 is intentionally absent — PyPI only ever saw 0.27.0rc1). npu.inc.md and Dockerfile.npu* are left alone: NPU upgrades ship as the NPU team's own PR (#6096 precedent). Signed-off-by: tzhouam <tzhouam@connect.ust.hk> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review finding (P1, #6606): vLLM 0.28's defer_block_free contract has two halves — schedule() advances sched_step_seq for every non-empty step, and update_from_output() advances processed_step_seq and drains the deferred frees once the step's GPU writes are known complete. Each Omni scheduler carried only half: - OmniARScheduler.schedule() delegates to super().schedule(), so sched_step_seq advanced — but its reimplemented update_from_output() never advanced processed_step_seq nor drained. Every finished request's blocks parked in deferred_frees forever: unbounded accumulation and eventual KV-cache exhaustion under async/PP KV-consumer configurations. - OmniGenerationScheduler reimplements both. Its fast-path schedule() never advanced sched_step_seq, so _update_after_schedule stamped last_sched_seq with the stale value and _free_request_blocks freed blocks immediately — while an in-flight GPU step could still be writing them. Its update_from_output() also never drained. update_from_output now advances the fence and drains at the top (exactly upstream's placement), and the generation fast path advances sched_step_seq right before _update_after_schedule so last_sched_seq stamping observes the advanced value, as upstream schedule() guarantees. The generation fallback path already got both halves from super().schedule(). Reads are getattr-guarded following this file's existing idiom: the scheduler unit tests drive these methods on __new__-constructed / MagicMock schedulers, and dataclass fields without defaults are invisible to MagicMock(spec=...). Tests pin the contract at the call boundary for both schedulers plus the advance-before-stamping order; the drain logic itself is upstream-tested. Signed-off-by: tzhouam <tzhouam@connect.ust.hk> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review finding (P1, #6606): updating the scheduler-side connector from ec_connector_output was only one part of the v0.28 EC contract. Upstream _free_request() also fires ec_connector.request_finished(request) BEFORE freeing the encoder cache — so the connector can inspect per-request state (e.g. which mm_hashes it recorded during save_caches()) — honors its delayed-free result, and returns the generated ec_transfer_params. OmniARScheduler._free_request() skipped the hook entirely and returned None as the second tuple item on every path; both schedulers' update_from_output() discarded that second value anyway; and _make_omni_engine_output() had no parameter for it. The consequence: the #6606 output-processor forwarding could never receive a remote encoder-cache handle, and a connector's delayed-free requirement was ignored (encoder blocks freed under an in-flight transfer). Now: the AR _free_request fires the hook before encoder_cache_manager.free(), ORs the delayed-free into connector_delay_free_blocks, and returns ec_transfer_params on all four return paths (including the KV-transfer-wait early returns, so already-emitted params are not lost); both schedulers keep the second tuple item (the generation scheduler's _free_request already delegated to upstream's, which does all of this); and the mixin threads ec_transfer_params into OmniEngineCoreOutput, whose field it inherits from upstream. The ec_transfer_params handoff to RequestOutput on the processor side landed earlier (ad4f1b5); this closes the scheduler-side gap so the field can actually be populated end to end. Tests cover the hook order (before encoder-cache free), the delayed-free result, the None-connector path, and the _free_request -> EngineCoreOutput plumbing in both schedulers. Signed-off-by: tzhouam <tzhouam@connect.ust.hk> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review finding (P2, #6606): vLLM 0.28 finalizes prefill statistics with PrefillStats.finalize(kv_cache_manager.estimate_cached_tokens(request)) before emitting them — computing num_cache_creation_tokens, the tokens computed and written to the prefix cache — and propagates the field into RequestOutput. The Omni paths dropped it at all three stations: both schedulers emitted take_prefill_stats() raw (never finalized, creation count stuck at its default 0), the output processor copied only num_cached_tokens out of the arriving prefill_stats, and the no-detokenizer direct RequestOutput build omitted the field. Prefix- cache creation usage was therefore reported as zero or None even when new cache entries were created. Both schedulers now finalize BEFORE stop handling (upstream's placement — _free_request releases the KV blocks, after which estimate_cached_tokens reports 0, so finalizing any later computes a zero creation count for exactly the finishing requests the field is for), the processor copies num_cache_creation_tokens into the request state alongside num_cached_tokens, and the direct build passes it to RequestOutput. The upstream-delegating build already read the state attribute, which OmniRequestState inherits. Tests pin the scheduler-side finalize for both schedulers with a lifecycle guard — the mocked _free_request drops the manager's estimate to 0, so a finalize that ran after the free would compute 0 instead of the expected creation count — plus the direct-build propagation. The MagicMock scheduler factories in the existing suites now pin kv_cache_manager.estimate_cached_tokens to a real int — finalize() asserts on and does arithmetic with the estimate, and an auto-mock return is not ordered against ints. Signed-off-by: tzhouam <tzhouam@connect.ust.hk> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The pre-commit check on #6606 failed at ee2ee48: ruff format (pinned v0.14.10) reformats 4 of the files touched by the three review-fix commits — line joins only, no semantic change. Formatted with the same pinned version; ruff check is clean on the full changed set. Signed-off-by: tzhouam <tzhouam@connect.ust.hk> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The merge-with-main CI on #6606 (vllm-omni build 14192, Simple · Diffusion Test) failed with TypeError: is_layer_skipped() got an unexpected keyword argument 'skip_with_substr' in vllm_omni/quantization/svdquant_config.py. The file arrived on main after this branch's rebase base via #6162 (offline SVDQuant W4A4), written against vLLM v0.27, whose is_layer_skipped took skip_with_substr: bool. v0.28 replaced that flag with match_mode: Literal["exact", "substring", "suffix"]; match_mode="substring" is the verbatim equivalent of the old skip_with_substr=True (any ignored-layer name contained in the prefix). Single call site; no other skip_with_substr use exists in the tree. Branch rebased onto main da4a08b to pick the file up (31 commits replayed conflict-free; before/after net-diff oracle identical), then the one-line adaptation applied on top. Signed-off-by: tzhouam <tzhouam@connect.ust.hk> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ef4f2b5 to
bf03025
Compare
|
Diffusion X2V · MiniMax H3 I2VA accuracy: evidence of a bimodal, environment-selected numeric mode on the v0.28 stack (nightly 2992/2993 triage) The i2va similarity check is producing exactly two deterministic outcomes on
Notes:
Options we see, in preference order: (1) scope video-path determinism the way the audio VAE already does, (2) revisit the 0.97 i2va gate given both modes are visually deterministic outputs ~0.008 apart, or (3) we keep digging for the exact op. Happy to implement whichever you prefer. |
tzhouam
added
ready
Signed-off-by: tzhouam <tzhouam@connect.ust.hk> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: tzhouam <tzhouam@connect.ust.hk> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
4 participants
Purpose
Rebase vLLM-Omni onto the released vLLM v0.28.0 (
releases/v0.28.0; final tagv0.28.0=2cf0a6915c). The 0.28 cycle was quiet for omni: the release-branch delta since the last alignment baseline (cdb8545a91be) is 11 release-branch commits, and module-by-module verification found no omni source adaptations required — the rebase surface is the CI wheel pin plus test hardening. The branch also carries fixes from the 0.27 window that landed after #5976 was squash-merged.Rebase content (0.28 cycle)
docker/Dockerfile.ci: precompiled-wheel pin →releases/v0.28.0(1af5a386cd43). A follow-up commit moving the pin to the final tag (2cf0a6915c: the Gemma-4 FA4 FP8 kernel revert #52987 + an arm64-only Cython CI pin #53358) is finishing validation and will land on this branch shortly.worker_runner,model_executor,model_configfor the branch delta; the final-tag delta re-verifiedworker_runner/model_executor/platform— the FA4 revert's API changes (_largest_kernel_block_withindropsvllm_config;get_supported_kernel_block_sizes_for_config→get_supported_kernel_block_sizes()) have zero references undervllm_omni/, confirmed by grep plus a GPU test sweep of the attention-consuming diffusion paths.tests/diffusion/kernels/mot/test_mot_linear.py: seed the random weights — the unseeded tensors made the TP-consistency assertions flake.tests/diffusion/models/lingbot_world/test_lingbot_world_camera.py: subprocess timeout 3s → 30s (cold-start compile blows the 3s budget on a busy card).Carried from the 0.27 window (post-#5976)
09ed2df28: propagate TP metadata to Bagel MoT experts (mot_qkv_parallel_linear/mot_row_parallel_linear+ a TP-consistency test).270d35ae8: review follow-ups from [Rebase] Rebase to vllm 0.27.0 #5976 (Gaohan123's comments).b3294cf72/e8470ca9b: the mid-cycle alignment to vLLM maincdb8545a91bewith its wheel-pin/deps sync.26af24ac3: DCO remediation for prior branch commits.Validation
112b585d(85 jobs): build 2984 — the non-green set is a strict subset of baseline build 2982 on the previous head; Omni Perf Async Chunk recovered. Build 2986 (same commit, full nightly env): 75 passed, same four known failures, no regressions.Known remaining issues
check-test-ci-coverageis red on an untouched tree (31 files, byte-identical to main) — pre-existing, needs its own fix on main.🤖 Generated with Claude Code