Repository navigation
Conversation
Signed-off-by: lm10-arg <347490427+lm10-arg@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 belong to: docs/design/module/diffusion/index.md. Module owners: @david6666666 @Isotr0py @princepride Routing: @david6666666 via module named in the PR description; @Isotr0py via module named in the PR description; @princepride via module named in the PR description @lm10-arg, 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. |
Signed-off-by: andyluo7 <43718156+andyluo7@users.noreply.github.com>
andyluo7
left a comment
There was a problem hiding this comment.
Reviewed exact head 7da98c09d6bd95af6839738795d6ec00718ab149 as andyluo7. I found no blocking code or CI-contract issue in the focused six-file diff.
What I checked:
- native ROCm
MI325andcards_2/cards_4marks are added only to qualified multi-GPU tiny-diffusion cases while existing CUDA L4/B200 marks are preserved - LTX2 remains CUDA-only after AMD #14621 reproduced a MIOpen
conv1dNo suitable algorithmfailure in all three LTX2 multi-GPU variants - the two nightly selectors match their collection and execution commands, use the correct
mi300_2/mi300_4pools, stay nonblocking, and fail closed at exact counts 12 and 11 - pipeline-contract and marker regression tests cover selector exclusions, exact counts, artifacts, and the LTX2 CUDA/ROCm split
- scoped pre-commit/static validation and
git diff --checkpassed
Exact-head AMD #14623 results:
- 2-GPU job
01a17429-4c31-42e0-8d76-376b2b0d0655: exact collection 12; 12 passed and 17 subtests passed; exit 0; no retry;soft_failed=false; four artifacts uploaded - 4-GPU job
01a17429-4c32-4e29-be59-671e4d412808: exact collection 11; 11 passed and 12 subtests passed; exit 0; no retry;soft_failed=false; four artifacts uploaded
The shutdown logs contain Python resource_tracker warnings, and the 4-GPU job contains TCPStore shutdown warnings, but both jobs completed normally with no live worker left in Buildkite. The three other nonblocking failures in the aggregate nightly build are separate Qwen/Hunyuan jobs and do not exercise this PR's changed paths.
CUDA #18071, NPU #9815, wheel builds, pre-commit, DCO, CodeQL, and docs are green. This is a COMMENT review, not an approval; independent approval and the author self-review requested by ReviewBot remain separate gates.
andyluo7
left a comment
There was a problem hiding this comment.
Approving exact head 7da98c0 after independent review and exact-head CI qualification. The AMD 2-GPU and 4-GPU target lanes passed cleanly with no retries or soft failures; CUDA, NPU, GitHub Actions, DCO, CodeQL, pre-commit, wheel, and documentation checks are green. The aggregate AMD soft failures are unrelated nonblocking nightly jobs documented separately.
Summary
1. What outcome do you want to achieve?
Improve the reliability and regression detection of multi-GPU diffusion inference on AMD Instinct GPUs by establishing automated ROCm nightly CI coverage, while preserving existing NVIDIA CUDA test coverage.
2. What is the highest-leverage variable for that outcome?
Consistent, meaningful multi-GPU test coverage on AMD GPUs. Running representative 2-GPU and 4-GPU diffusion workloads regularly is critical to detecting parallelism and compatibility regressions before they impact users.
3. How will this PR move that variable?
4. Whose customer or user needs does this address, and what are those needs?
This PR strengthens AMD ROCm test coverage and helps improve the long-term reliability of vLLM-Omni's multi-GPU diffusion support.
Current-head AMD build #14621 collected all 26 candidate cases and passed every non-LTX2 case. The three LTX2 multi-GPU variants failed in the existing audio-vocoder path because MIOpen could not select a
conv1dalgorithm on MI325. This PR therefore keeps those variants CUDA-only; the LTX2 ROCm runtime fix remains separate from this CI-only scope.Scope split from #7688
An overlap review of #7688 against current
mainfound that its broad Qwen function and documentation lanes now duplicate existing coverage, while its performance, multi-replica, quantization, tooling, and runtime changes require independent qualification.This PR intentionally extracts only the highest-confidence tiny-diffusion multi-GPU AMD coverage. It does not carry over:
rocm_ci_evidence.pyPR #7688 is intentionally left unchanged and can be closed after this focused replacement merges.
CI contract
The nightly jobs target only
tests/model_tests/diffusion/test_common_offline.pywith native ROCm selectors:full_model and diffusion and rocm and MI325 and cards_2 and not nvidia_onlyfull_model and diffusion and rocm and MI325 and cards_4 and not nvidia_onlyBoth use
--run-level core_modelso the workloads retain tiny weights. The existingassert-pytest-collection.shguard fails closed if either exact collection changes.Validation
Base at validation:
68f0c78ee4648c906990ac7f262ff616c93f9be3Current reviewed head:
7da98c09d6bd95af6839738795d6ec00718ab149Passed on exact-head AMD build #14623:
soft_failed=false; four expected artifacts uploaded.soft_failed=false; four expected artifacts uploaded.Both jobs emitted Python
resource_trackercleanup warnings at interpreter shutdown; the 4-GPU job also emitted TCPStore shutdown warnings. Buildkite recorded both jobs as clean exits with no retries or soft failures, and no worker remained running after job completion.The aggregate nightly build also contains three unrelated nonblocking failures in Qwen3-Omni AITER smoke, Qwen2.5-Omni documentation examples, and HunyuanImage3 pixel accuracy. Those jobs do not execute the changed tiny-diffusion paths or the two added jobs; the individual exact-head target jobs above are the qualification evidence for this PR.
Other current-head CI passed:
Passed locally:
pre-commithooks for all six changed filesgit diff --checkLocal pytest collection could not run because this Mac environment does not have
torch; the exact-head AMD collection and runtime gates are satisfied by build #14623 above.Need help on this PR? Tag
@codesmith-botwith what you need.