Repository navigation
Conversation
|
This PR appears to belong to: docs/design/module/diffusion/offloader.md, docs/design/module/diffusion/diffusion_model_integration.md, docs/design/module/diffusion/index.md. Module owners: @fhfuih @Bounty-hunter @david6666666 Routing: @fhfuih via module of the changed files, CODEOWNERS; @Bounty-hunter via module of the changed files; @david6666666 via module of the changed files @andyluo7, 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: no human activity for 7 days@andyluo7 this pull request has had no human commit, comment or review since 2026-09-17. 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. |
01d34af to
0ffe975
Compare
Omni ReviewBot triage noteResolved as of |
Signed-off-by: andyluo7 <andy.luo@amd.com>
|
Self-review at source head Checked six PR-specific Omni function/docs/performance/multi-replica selections against current CUDA scopes and one/two/four-GPU AMD queues. The no-async command retains 300 minutes within a 310-minute step. JUnit/log/result/benchmark/image/GPU/cleanup evidence and nonempty execution gates remain. Inherited #7606 changes configure both quality-arm startup budgets and the strictest configured ROCm LPIPS limit; cumulative dependencies remain explicit. Thirty-five focused contracts, marker/loader/control-flow checks and applicable hooks passed. Actual current model/docs/performance/LPIPS execution and repeated qualification remain; prior-head passes or rendering cannot certify this head. Also inspected the inherited config contract correction: the intended thinker budget is 512 tokens for the existing 200-word assertion. All affected config test files match; source16/integration22 existing stability contracts and changed-file hooks pass. The full config-factory runtime test and current-head GPU coverage remain pending. This new source head awaits publication. |
Omni ReviewBot: supersededThe CI failure noted on |
|
@andyluo7 this PR is labeled Could you please take a look and push an update to get CI green? Once the checks pass we can proceed with review/merge. Thanks! |
Omni ReviewBot routing recordAssigned Strict on cursor (cursor-grok-4.6-high) under experiment |
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
1 actionable finding(s).
CI at
f67956155862(2026-10-10T09:39:26.485009+00:00): required check(s) blocking:buildkite/vllm-omni(failed). Observed Buildkite:buildkite/vllm-omni-npu-ci(failed),buildkite/vllm-omni-intel-ci(passed),buildkite/vllm-omni-amd-ci(failed), and 1 more.
Full review analysis
Scan:
| Category | Result |
|---|---|
| Tests / verification | 1 finding(s) below |
| Security | no finding reported |
| Docs / comments | no finding reported |
| Behavior / compatibility | no finding reported |
| Correctness | no finding reported |
Validated:
- [resolved] gpu_memory_utilization: None is omitted from yaml_engine_args (stage_config.py:1071-1072) matching colocate 2GiB pattern; 80GiB KV only on qwen3_omni_moe overlay, not multi_replicas_4gpu.
- [resolved] VLLM_CI_EXPECTED_GPU_COUNT defaults to 1 in rocm_ci_evidence.py:36 but template injects agent_pool[-1:] (test-template-amd-omni.j2:45-46,122); residual: script still lies if run outside the template.
- [resolved] pytest|tee exit codes: run-amd-test.sh:5,79 uses pipefail + ERR trap so Function Expansion/Accuracy still fail the job without pytest-result; residual: those two jobs still lack the nonempty-executed gate the new jobs have.
- [resolved] sampling_params_list is forwarded (client.py:1270-1271); LONG_OUTPUT only covers test_text_to_audio_long_output_001, not other expansion audio cases.
- [resolved] default_sampling_params deep-merges (stage_config.py:706-728) so overlay seed does not drop temperature 0.9; residual: test_amd_nightly_stability.py:59 asserts the unmerged overlay dict, not load_deploy_config.
- [resolved] LTX2 quality model uses LTX2Vocoder without BWE (ltx2_components.py:107,120-127); BWE path still autocast and has GPU test test_ltx2_vocoder_cuda.py:40-50.
1 actionable finding(s).
Verdict: REQUEST CHANGES
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!
| # Function expansion includes a 300-word generation case with | ||
| # a 200-word minimum. A 150-token cap deterministically | ||
| # truncated that case to 113 words in AMD build #11997. | ||
| "default_sampling_params": {"max_tokens": 512, "ignore_eos": False}, |
There was a problem hiding this comment.
[P1] This overlay raises shared CI thinker default_sampling_params.max_tokens from 1…
Evidence and suggested fix
This overlay raises shared CI thinker default_sampling_params.max_tokens from 150 to 512 on every ci/qwen3_omni_moe.yaml materialization (tests/helpers/stage_config.py:333), not a request-local override. The only 300-word case already sends sampling_params_list=LONG_OUTPUT_SAMPLING_PARAMS with thinker max_tokens 512 (tests/e2e/online_serving/test_qwen3_omni_expansion.py:43 and :513), which replaces stage params in serving_chat._to_sampling_params_list. Suites that load the same overlay and omit request max_tokens therefore inherit 512, including CUDA ready Omni · Qwen3-Omni Test (timeout 20m), NPU ready, tests/e2e/online_serving/test_qwen3_omni.py, accuracy/WER, websocket, and the rest of the expansion module. Revert the overlay to 150 (or split a function-only overlay) so those contracts stay on the old default budget.
Evidence: Trigger: tests/helpers/stage_config.py:333 "default_sampling_params": {"max_tokens": 512, "ignore_eos": False}, raises thinker max_tokens from 150 to 512 on every ci/qwen3_omni_moe.yaml materialization. The only case that needed 512 already overrides locally: tests/e2e/online_serving/test_qwen3_omni_expansion.py:43 "max_tokens": 512, inside LONG_OUTPUT_SAMPLING_PARAMS and :513 "sampling_params_list": LONG_OUTPUT_SAMPLING_PARAMS,. Unchanged by this diff, present in the PR-time tree: vllm_omni/entrypoints/openai/serving_chat.py:1287 final_sampling_params_list.append(target_cls(**sampling_params)) — request sampling_params_list replaces stage params, so overlay 512 is unused by that case; serving_chat.py:1342 max_tokens is NOT computed dynamically — it uses the deploy YAML default.; serving_chat.py:1451 params = self._apply_request_overrides(default_params, request) / :1455 sampling_params_list.append(default_params.clone()) — omitted request max_tokens keeps YAML defaults on every stage. Adverse effect: unchanged overlay consumers inherit the new 512 cap, e.g. tests/e2e/online_serving/test_qwen3_omni.py:24 _CI_DEPLOY = get_deploy_config_path("ci/qwen3_omni_moe.yaml") and :127-134 mix request has no max_tokens; tests/e2e/accuracy/qwen3_omni/test_qwen3_omni.py:64 same overlay; .buildkite/cuda/test-ready.yml:168 timeout 20m bash -c " wrapping that online suite. This PR itself documents 150 as a binding truncation (stage_config.py:331-332 A 150-token cap deterministically truncated that case to 113 words), so the shared raise lets ready/accuracy/websocket runs emit up to 3.4× thinker tokens (talker still capped at 1000) versus the previous CI budget the 300-word request-local override already covers.
Suggestion: "default_sampling_params": {"max_tokens": 150, "ignore_eos": False},
Omni ReviewBot: finding feedback[p1] This overlay raises shared CI thinker default_sampling_params.max_tokens from 1… — See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement. |
Omni ReviewBot: no human activity for 13 days@andyluo7 this pull request has had no human commit, comment or review since 2026-10-04. 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. |
Keep the existing MI300 nightly jobs and add the Omni function, documentation, and performance lanes. Point the WER guard at the current Qwen3-Omni accuracy test. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: haic0 <149741444+haic0@users.noreply.github.com>
f679561 to
1b83c9d
Compare
|
Rebased onto current The nightly file now keeps the jobs already on main (Wan2.2, Hunyuan, Qwen2.5-Omni docs, and the rest) and adds the Omni function, documentation, and performance lanes. CUDA doc artifacts keep both The WER guard now reads Local contract tests passed: |
|
Closing as superseded by #9369, which landed the focused and qualified MI325 tiny-diffusion multi-GPU nightly coverage extracted from this PR. The remaining broad performance, multi-replica, quantization, tooling, and runtime changes require separately scoped and independently qualified PRs. |
Expand ROCm Omni function, Documentation, and performance coverage.
This revision incorporates #8348's Documentation audio-budget candidate at
2a7eeef877f61549f856733ccffb94a31bd5c72f. In predecessor CUDA #16473, stream001 scored 0.74180 against the unchanged 0.8 gate despite active seeded ASR fallback. Four retained long-answer WAVs each lasted exactly 79.8275 seconds. The Documentation fixture now uses the existing long-form Function budgets: 3,072 talker tokens and 6,144 code2wav tokens. Its prompt, 512-token thinker, talker seed 42, remaining sampling settings, quality gate, and artifact paths are preserved. Runtime repair is pending new-commit evidence.The preceding artifact repair uploads generated WAV/JSON alongside execution evidence. VeRL and all three MiniCPM source targets each have two passing AMD observations. Tiny Base #13090 passed 158 executed tests with six skips and 14 verified artifacts, completing its second source observation. Preserve historical c30 #13061/#13064, each 65/65, and all earlier results for their recorded commits.
Separate failures remain open. Predecessor AMD #13086 Function Expansion passed 25 tests and failed the async large-image audio case at similarity 0.4167; its failing WAV was not retained. Isolated diagnostic #13094 now establishes Z-Image LPIPS 0.18095 against 0.15, with retained baseline/quantized PNGs. Flux2 finished with lost container contact and exit -7 after repeatedly reaching its 150 GiB memory limit; sampled OOM and OOM-kill counters stayed zero, so the terminal cause remains unconfirmed. Current d7 AMD #13093 also has an AITER smoke setup error from a TCPStore port collision (EADDRINUSE), distinct from these numerical/audio failures.
Validation: each dependency delta changes only
tests/examples/online_serving/test_qwen3_omni.py. Actual fixture resolution verifies only the two nested audio caps change, with thinker 512, seed 42, and shared overlays preserved. Changed-file hooks, mypy, and test-mark checks passed. Integration pipeline/evidence contracts passed 40 tests. Actual Minijinja 2.3.1 rendering retains 34 READY, 30 MERGE, and 18 NIGHTLY jobs plus initialization/image, 84 total, with the previous render hash unchanged. Final model, quality, performance, artifact, actual Nemotron inference, and unchanged-commit repetition gates remain. Human review is required.