Repository navigation
[CI/Build] Fix vLLM 0.33 AMD regressions - #9883
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 be related to model: MinimaxH3. Model owners: @david6666666 @yenuo26 @congw729 Routing: @david6666666 via model owner; @yenuo26 via CI owner, CODEOWNERS; @congw729 via CODEOWNERS @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>
amy-why-3459
left a comment
There was a problem hiding this comment.
Thanks for tracking these down. The four fixes are small and well-scoped; a few code-level notes:
-
AITER loader path (
aiter_mha_cache.py) – see inline: the glob result is unordered and can produce duplicate parent dirs. Minor, but worth tightening since this list is used to build the loader path that decides cache acceptance. -
MOSS
libdevice = tldevice– the kernels rely onlibdevice.mul_rn(round-to-nearest is the whole point of that line) andlibdevice.exp. Please make sure the exact-head AMD READY run actually executes the MOSS local-attention tests that previously failed to compile (the 19 tests mentioned), so we knowmul_rnresolves on the HIP libdevice and not only that the module imports. On CUDA this should be behavior-preserving sincetldevicedispatches to the same CUDA libdevice. -
test_fused_gate_up_matches_separate– switching totorch.testing.assert_closemakes sense for fused vs separate GEMMs. The default tolerances depend on dtype (e.g. bf16 rtol≈1.6e-2), which is fairly loose; consider passing explicitrtol/atolmatching the dtype used in the test so the contract is visible and a real regression in the fused weight packing can't slip through under the defaults. -
tools/__init__.py– makingtoolsa regular package looks right, and since[tool.setuptools.packages.find]only includesvllm_omni*it won't get shipped as a top-leveltoolspackage in the wheel. The regression test covers the repo-root-first ordering, which matches how CI runs. Just double-check nothing in CI relied on namespace-package merging oftoolsacross multiple path entries.
No blocking issues from my side on the code itself; the PR body already notes that exact-head AMD READY is still needed.
Signed-off-by: andyluo7 <43718156+andyluo7@users.noreply.github.com>
Signed-off-by: andyluo7 <43718156+andyluo7@users.noreply.github.com>
amy-why-3459
left a comment
There was a problem hiding this comment.
Incremental look at 349fbf1 (gate the MOSS round-to-nearest multiply to CUDA):
- Gating
libdevice.mul_rnbehind aCUDA_RN_OPSconstexpr resolves the HIP concern from my previous pass, and importingtldevice as libdevicedirectly is cleaner than the module-level alias. - On non-CUDA platforms the fallback
v * current_probabilityis a plain multiply that feeds straight into the followingtl.fmachain, so the compiler is free to contract it the same way the original comment warns about for SM89. That means fused vs. unfused bitwise parity is no longer pinned on ROCm. If the MOSS parity tests (fused/unfused orassert_closewith zero tolerance) run on the AMD lane, it would be worth either confirming they still pass there or relaxing/skipping the bitwise check for HIP with a short comment, so a future flaky failure isn't misread as a kernel regression.
amy-why-3459
left a comment
There was a problem hiding this comment.
Incremental look at 13a4c72 (deterministic TheRock library discovery):
- Collecting the OpenBLAS parent directories into a set and sorting them removes both the duplicate entries (one per SONAME file) and the filesystem-order dependence, and the updated test covers both cases. Looks good to me.
- Minor note: the resulting order between multiple
_rocm_sdk_*packages is now plain lexicographic (_rocm_sdk_librariesbefore_rocm_sdk_runtime). That's fine for determinism. If one package's copy should take precedence when both shiplibrocm-openblas, a short comment saying the alphabetical order is intentional (or an explicit priority) would make that clear to the next reader.
Signed-off-by: andyluo7 <43718156+andyluo7@users.noreply.github.com>
andyluo7
left a comment
There was a problem hiding this comment.
Approved after exact-head READY qualification. CUDA #18795 passed 19/19 jobs and AMD/ROCm #15332 passed 48/48 jobs on 4e7f3c9, with zero nonzero exits, soft failures, or retries. R2-01 verified the exact 1,420-node collection and all three shards exited 0.
Signed-off-by: lm10-arg <347490427+lm10-arg@users.noreply.github.com> Signed-off-by: andyluo7 <43718156+andyluo7@users.noreply.github.com> Co-authored-by: lm10-arg <347490427+lm10-arg@users.noreply.github.com> Co-authored-by: andyluo7 <43718156+andyluo7@users.noreply.github.com>
Contributor intent (required human answers)
1. What outcome do you want to achieve?
Fix the CI regressions introduced by the vLLM 0.33 upgrade and restore an approval-ready exact-head READY result.
2. What is the highest-leverage variable for that outcome?
Deterministic exact-head AMD/ROCm READY CI.
3. How will this work move that variable?
It fixes four established compatibility regressions: repository-local
toolsimports being shadowed by vLLM; normal fused-vs-separate GEMM rounding being treated as bitwise inequality; unconditional CUDA-only Triton rounding in MOSS; and TheRock OpenBLAS directories being absent from AITER artifact validation. Focused L1 regressions cover the deterministic import and library-discovery paths, while AMD READY validates the ROCm/PyTorch behavior at the submitted head.4. Whose customer or user needs does this address, and what are those needs?
This addresses customers and users affected by regressions from the vLLM 0.33 upgrade. They need stable AMD/ROCm CI.
Purpose
PR #9648 upgraded vLLM-Omni to vLLM 0.33. Its direct parent passed AMD build #15302 with 42/42 jobs, while the upgrade exposed three blocking failures across AMD builds #15305, #15307, #15310, and #15313. Exact-head AMD READY #15317 then exposed a fourth compatibility regression in its NonBlocking R2 coverage lane; the first portable-binding retry in #15322 confirmed that HIP lacks
mul_rnand requires a compile-time backend split:toolspackage. Because Omni'stools/was only a namespace package, imports such astools.pre_commit,tools.minimax_h3, andtools.sheetsage2_transcriberesolved to vLLM instead of this repository.2.13.0+rocm10.0.0can produce normal floating-point rounding differences between separate and fused gate/up CPU GEMMs. The test required bitwise equality even though its contract is numerical equivalence.mul_rnis unavailable, so this operation must be CUDA-only while ROCm uses normal multiplication.librocm-openblas.so.*under TheRock_rocm_sdk_*packages. AITER artifact validation searched onlytorch/liband/opt/rocm/lib{,64}, reportedlibrocm-openblas.so.0 => not found, rejected the compatible artifact, and cold-compiled it for roughly 1,540 seconds in build #15313.This change:
tldevicebinding and a constexpr backend split so explicit round-to-nearest multiplication is CUDA-only;toolsa regular package and adds a regression that proves it wins over a later dependency package;torch.testing.assert_closefor the gate/up numerical parity assertions while retaining exact QKV assertions; andNo public API, supported configuration, default, or active design contract changes. Documentation and examples are unchanged because this is limited to CI/developer tooling and test semantics.
AI assistance: Codex was used to investigate the CI failures, implement the scoped fixes, add regression tests, run local validation, and draft this description. Actual results and the remaining hardware validation gap are listed below.
Test Plan
vLLM Version: 0.33.0 ROCm CI image
vLLM-Omni Commit:
4e7f3c9a3fa54ad0a4baedc59db4858ba8979e8dLocal L1 commands:
CI-like L1 command in a complete repository environment:
Focused Qwen regression command:
READY acceptance criteria:
toolsimports collect successfully;mul_rnon ROCm while retaining it on CUDA;librocm-openblas.so.0 => not foundor a fallback cold compile;Test Result
tests/buildkite/test_aiter_mha_cache.py: 33 passedtests/tools/test_tools_package.py: 1 passedtests/helpers/tests/test_check_test_marks.py: 49 passedpre-commit across all six changed files: passed all applicable hooks, including Ruff, mypy, test marks, SPDX, forbidden imports, andtorch.cudacheckspython3 -m py_compile <six changed Python files>: passedgit diff --check origin/main...HEAD: passed\n- focused R2-01 pipeline contracts: 4 passed\n- full Buildkite contract files: 240 passed, 7 skipped; one additional test could not importtorchbecause the local Python 3.14 environment does not have it installed\n- pre-commit on the three collection-guard files: passed all applicable hooks, including Buildkite pipeline validationThe MOSS backend split passed py_compile and all applicable pre-commit hooks.
The Qwen regression was not run locally because this Mac does not have the repository Torch/vLLM environment and is using Python 3.14, outside the project's supported Python 3.11-3.13 range. Exact-head READY evidence is recorded below.
\n## Exact-head READY Result\n\n- CUDA #18795: passed on
4e7f3c9a3fa54ad0a4baedc59db4858ba8979e8d; 19/19 jobs finished, with zero nonzero exits, soft failures, or retries.\n- AMD/ROCm #15332: passed on the same exact head; 48/48 jobs finished, with zero nonzero exits, soft failures, or retries.\n- R2-01 collection guard verified exactly 1,420 selected tests; all three shards exited 0. Shard 1 reported 502 passed, 3,188 deselected.\n