Repository navigation
[Bugfix] Expect Qwen3-Omni Thinker prefix cache only on CUDA - #9312
Conversation
ROCm keeps the Thinker prefix cache off in the production deploy, but the runtime test still required the CUDA default and failed AMD CI. Co-authored-by: Grok 4.7 <noreply@x.ai> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: haic0 <149741444+haic0@users.noreply.github.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
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: Wan. Model owners: @bjf-frz @NickCao @yenuo26 Routing: @bjf-frz via model owner; @NickCao via CODEOWNERS; @yenuo26 via CODEOWNERS @hacZ17, 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. |
hsliuustc0106
left a comment
There was a problem hiding this comment.
@hacZ17 — the updated expectation matches the deployment configuration; the remaining evidence gap is the changed runtime test itself.
- When a compatible environment is available, attach the named
test_structured_multistage_config_reaches_runtimeresult for the failing baselinee12f297536d0a5ae6b6c503c26236f0b2fb66719and head93a45d36487442472f8d0f7265f24f0bc55782d3, with versions and command. Acceptance: the ROCm assertion changes from the reported failure to pass while the CUDA-true expectation is preserved. If local hardware is unavailable, retain that explicit exception for maintainer validation rather than treating the 17 config-only tests as execution of this e2e path.
Source-only review; no demonstrated code defect and no hardware tests executed by me.
|
@hacZ17 — current-head maintainer update for The functional fix remains the same: ROCm keeps the Thinker prefix cache off. The follow-up commit refines the E2E assertion to compare the materialized vLLM runtime value with Current-head validation:
I added the |
Signed-off-by: andyluo7 <andy.luo@amd.com>
f0b5603 to
52e003f
Compare
andyluo7
left a comment
There was a problem hiding this comment.
Approved at exact head 52e003fc28dd099c7a3d3337178c56ad5eaac937.
The test now validates that the resolved Thinker cache setting reaches the materialized vLLM runtime config, while platform policy remains covered by the deploy/config tests. No production code or deploy YAML changes.
Exact-head evidence:
- AMD build #14522 passed all 51 jobs.
mi300_2: Omni Model Test Qwen3-Omnicollectedtest_structured_multistage_config_reaches_runtimeand finished with 9 passed, 7 skipped, 14 deselected. - CUDA build #17960 passed all 31 jobs.
Omni · Qwen3-Omni Testcollected the same test and finished with 16 passed, 23 deselected. - GitHub build, pre-commit, CodeQL, DCO, and docs checks passed.
- No unresolved review threads.
Contributor intent
1. What outcome do you want to achieve?
Make the AMD CI Qwen3-Omni job accept the production deploy's platform contract: Thinker prefix caching stays on for CUDA and off on ROCm. Do not change CUDA behavior.
2. What is the highest-leverage variable for that outcome?
test_structured_multistage_config_reaches_runtimerequiresthinker_vllm.cache_config.enable_prefix_caching is Trueon every platform.3. How will this work move that variable?
Expect that runtime flag to equal
current_omni_platform.is_cuda(). That is True on CUDA and False on ROCm, which is the value the ROCm overlay already applies.4. Whose customer or user needs does this address, and what are those needs?
AMD CI and ROCm serving of
vllm_omni/deploy/qwen3_omni_moe.yaml. That deploy keeps Thinker prefix caching off outside the CUDA V1 turn profile that #9203 validated.Summary
Build 14512 job
mi300_2: Omni Model Test Qwen3-Omni(01a169f6-90dd-4c99-8105-3f87f7b85dff) ran currentmaine12f297536d0a5ae6b6c503c26236f0b2fb66719. It is not a stale SHA. The checkpoint loaded, then this assertion failed:#9203 turns stage-0
enable_prefix_cachingon for CUDA V1 turn serving and documents that the non-CUDA platform blocks keep it off.qwen3_omni_moe.yamlsets it true on stage 0 and false underplatforms.rocm(also NPU, MUSA, and XPU). The ROCm overlay is the intended contract, so this change updates the runtime expectation instead of enabling the cache on ROCm. Talker and Code2Wav stay off. CUDA still expects True.Why this is not a duplicate
tests/config/test_config_factory.py::TestDeployConfigLoading::test_thinker_prefix_cache_defaultsalready expects CUDA True and ROCm/NPU/XPU/MUSA False, and that test passes on this base.enable_prefix_caching.Test Plan
vLLM Version: local unit run imported vLLM 0.31.0. The AMD job log reported vLLM 0.32.0. This patch does not change model code.
vLLM-Omni Commit:
93a45d36487442472f8d0f7265f24f0bc55782d3on basee12f297536d0a5ae6b6c503c26236f0b2fb66719.Test Result
Result: 17 passed in 1.14s.
Result: passed, including ruff, mypy for Python 3.10, SPDX, and test-mark checks. The commit hook sign-off check passed.
Not run:
tests/e2e/offline_inference/test_qwen3_omni.py(needs GPUs and the Qwen3-Omni weights). No model-output or accuracy claim. AMD CI was not rerun.