Repository navigation
Conversation
|
Exact-head self-review for
No additional author-fixable finding is known. Fresh AMD READY/MERGE/nightly for |
|
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, model owner; @Isotr0py via module named in the PR description; @princepride via module named in the PR description @haic0, 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. |
75b9f3a to
2c08263
Compare
|
Updated-head implementation review for
Requesting exact-head READY/MERGE CI. Human approval and terminal inspectable AMD/CUDA results remain required; no merge is requested before those gates pass. |
|
@vllm-omni-review-bot Please review the updated test/CI contracts at 7554bd4, especially JoyImage default-backend coverage, MiniCPM-o NVIDIA versus ROCm expectations, and JUnit checkout paths. Human approval and exact-head CI qualification remain required. |
Omni ReviewBot routing recordAssigned Strict on zcode (GLM-5.3-Flash) under experiment |
|
Exact-head CI checkpoint for
A signed-in Buildkite operator should retry only that HTTP-429 job after rate limiting clears. Please retain healthy jobs and changed-target evidence; do not restart the full build or treat aggregate red as a JoyImage/MiniCPM regression. |
|
Follow-up at current head 7776e11: CUDA #16901 exposed a relative-import collection failure in the new JoyImage GPU file. Fixed with an absolute tests.diffusion.models.joy_image helper import and an import-resolution regression. Twelve focused pipeline/import tests and changed-file pre-commit pass. The old 7554bd4 builds are diagnostic history, not current-head qualification. No production behavior or tolerances changed. @vllm-omni-review-bot Please review the current head; human approval and exact-head CI remain required. |
|
Review evidence updated for rebased head
#13506 is explicitly pre-rebase evidence. It does not qualify |
|
Current-head checkpoint: preserved the author-added MiniMax ROCm no-GPU guard and fast-forwarded to d99864a. GPU-backed AMD CI still runs the float64 solve-rule assertion. Refreshed READY once with MERGE already present to register qualification at this head; older runs are diagnostic history. Please hold the branch steady while exact-head CI completes. Human approval remains required. |
d99864a to
504bebc
Compare
|
@yenuo26 @congw729 @NickCao Could you review the rebased current head All seven intentional commits remain in this one PR; the diff is limited to seven test/CI files, with no production-code changes. Please focus on:
Current-head pre-commit, docs, and DCO have passed. Exact-head Buildkite qualification is being requested now; pre-rebase builds are diagnostic history, not qualification for this head. The author's focused-test results are recorded in the existing self-review comment. Human approval and inspectable AMD READY/MERGE plus CUDA hardware results remain required before merge. Please keep the branch steady while qualification runs unless a concrete failure or review finding requires a change. This is a review request, not a request to merge before those gates pass. |
|
Published two follow-up commits on current head
Validation: all 13 focused pipeline/import tests passed using the dependency-light Additional qualification blocker observed after the earlier triage: both dedicated JoyImage jobs on pre-fix AMD #13499 ended with exit 124. The dense AITER module built successfully in 1370.8s / 1416.9s; seven test cases then passed, but the masked varlen module started with too little time left in the 25-minute command budget. These two commits do not alter that dedicated-lane budget. A bounded timeout/prebuilt-kernel strategy still needs agreement before AMD can be claimed qualified; padding parity was not reached in those AMD jobs. No production code, tolerance, LAPACK workaround, MiniCPM contract, existing job cancellation, or PR merge was changed. GPU CI and human approval remain pending. |
|
Published one additional commit to existing #8643 (no new PR):
Qualification is still pending: obtain one passing AMD JoyImage run on this exact final head before merging #8643. CUDA success does not qualify ROCm/AITER, and previous-head timeout jobs did not reach the masked/default-backend parity cases. This moves cold compilation off the routine AMD READY/MERGE critical path; it does not establish that compilation is faster or that the provisional timeout is sufficient. Revisit blocking AMD coverage once exact-module prebuilds or measured cold runtimes make its budget reliable. The push was a normal fast-forward, and GitHub/fork branch both confirm the new SHA. No CI labels were cycled, builds canceled, or full nightly runs manually started. Existing |
|
Triggered final-head CI for Exact SHA independently verified from each Buildkite build JSON:
AMD bootstrap logs confirm selection of all three suites. Live job records show exactly one JoyImage GPU job, Final-head docs and DCO have passed. GitHub pre-commit and Python 3.11/3.12 wheels are queued. AMD/CUDA/NPU validation is pending. Human review is still required; existing reviewers are NickCao, yenuo26, and congw729, with no submitted reviews or unresolved inline threads at this check. Before merge, inspect the exact-head changed-target results, including all ten AMD JoyImage cases and JUnit, rather than relying on the nonblocking nightly aggregate. Reuse successful results and retry only actionable transient failures. No code/base/head changes, review approval, or merge were performed for this trigger. |
Co-authored-by: GPT-5.6 Sol <noreply@cursor.com> Signed-off-by: haic0 <149741444+haic0@users.noreply.github.com>
Signed-off-by: andyluo7 <andy.luo@amd.com>
…lback Signed-off-by: andyluo7 <andy.luo@amd.com>
Signed-off-by: andyluo7 <andy.luo@amd.com>
Signed-off-by: andyluo7 <andy.luo@amd.com>
Signed-off-by: andyluo7 <andy.luo@amd.com>
Co-authored-by: GPT-5.6 Sol <noreply@cursor.com> Signed-off-by: haic0 <149741444+haic0@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
FlashAttention removes padded self-attention queries whereas SDPA computes them. Compare all image tokens and only valid text tokens without changing tolerance or default-backend assertions. Perturb padded text to verify it cannot influence valid outputs. Signed-off-by: andyluo7 <andy.luo@amd.com>
Exclude JoyImage from the broad AMD READY model command so its cold AITER compilation is owned by the blocking READY/MERGE JoyImage jobs. Add a regression test preserving the broad selector, timeout, exclusions and blocking grade, and assert CUDA still includes JoyImage. Signed-off-by: andyluo7 <andy.luo@amd.com>
Remove duplicate dedicated JoyImage GPU lanes from AMD READY and MERGE while retaining the READY broad-model exclusion. Run the complete default-AITER inference suite once in nonblocking nightly coverage with isolated build caches, JUnit artifacts, and provisional 90m job / 80m command budgets. Add regressions for READY/MERGE absence and unique nightly ownership, preserving CUDA selection, backend assertions and existing CPU coverage. Exact-head AMD runtime qualification remains required before merging this attention-test change. Signed-off-by: andyluo7 <andy.luo@amd.com>
38fafcd to
46318fb
Compare
|
Proceeding with the revised qualification plan at
@NickCao @yenuo26 @congw729 Please review the current test/CI patch and the documented evidence reuse when available. Human approval remains required. Please keep the branch steady during qualification unless a concrete failure or review finding requires a change. This is not a request to merge before the remaining gates pass. |
ROCm exposes GPUs through the CUDA namespace, so the Ada/Hopper capability check could admit HIP devices and fail in torch._scaled_mm with HIPBLAS_STATUS_NOT_SUPPORTED. Reject HIP before checking NVIDIA capability so the existing AuK pipeline keeps its model-dtype fallback and GPU tests skip unsupported execution. Add CPU regressions for HIP rejection without a capability query, validated Ada/Hopper acceptance, unsupported architecture boundaries, and unavailable CUDA. Preserve all existing FP8 numerical and graph tests. AI-assisted-by: OpenAI Codex Signed-off-by: andyluo7 <andy.luo@amd.com>
…-project#8643) Signed-off-by: haic0 <149741444+haic0@users.noreply.github.com> Signed-off-by: andyluo7 <andy.luo@amd.com> Co-authored-by: GPT-5.6 Sol <noreply@cursor.com> Co-authored-by: andyluo7 <andy.luo@amd.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
Fix AMD CI contracts and guard AuK's NVIDIA-only FP8 execution on ROCm:
torch.inference_mode()). Keep TORCH_SDPA comparisons and platform-default ROCm FLASH_ATTN/AITER assertions.fp8_supported()predicate before querying NVIDIA capability. Production uses its existing model-dtype fallback on ROCm; the ten NVIDIA-only FP8 GPU cases skip there. CPU regressions retain Ada/Hopper acceptance and unsupported-device boundaries. FP8 arithmetic and NVIDIA numerical/graph coverage are unchanged.--durations=50, fail-closed pytest, and unique checkout-relative JUnit artifacts.Root cause and current-main diagnosis
AMD build #13504 ran at current-main SHA
1e0c4f3d54a91c30de97aa332e772dfe7338f52cand had four deterministic failed jobs:test_startup_precapture_uses_shared_arena_and_freezes_admission,test_large_batch_and_frozen_wrapper_stay_eager, andtest_failed_capture_blocks_even_eager_retry. On HIP, production correctly chose eager fallback before those CPU tests reached mocked NVIDIA-capture expectations.test_delta_factors_apply_the_vdn_solve_rulebecause the AMD CPU wheel lacks LAPACK for float64 Cholesky. The test-only ROCm path now keeps all operands and references on GPU; production is unchanged.test_transformer_shape_and_masked_forwardfailed under the non-serving gradient contract, and the 45-minute job timed out before pytest printed its failure summary.Earlier #13484/#13499 runs exposed the same JoyImage inference-contract and cold-AITER budget issues. The JoyImage/MiniMax/MiniCPM fixes do not broaden skips, relax tolerance, restore CPU LAPACK, or change their production routing. The AuK follow-up separately rejects the unsupported HIP FP8 path.
AuK FP8 follow-up
The supplied AMD #13507 Model Test log reports ten AuK FP8 cases failing with
HIPBLAS_STATUS_NOT_SUPPORTED; the following Mammoth SDPA command passed all 40 cases. Upstream #8442 (e623cef98) introduced the AuK code/tests and the rebase inherited them unchanged. ROCm exposestorch.cuda, so a CUDA device/capability check alone admitted an unsupported E4M3torch._scaled_mmpath.The fix rejects
torch.version.hip is not Nonebefore querying NVIDIA capability. Existing production fallback and test skips use the same predicate. No broad file exclusion, numerical-tolerance change, FP8 arithmetic change, or new CI job is introduced.Local validation of this follow-up:
git diff --checkpass.Reproduction and qualification commands from repository root, using a matching vLLM/vLLM-Omni environment and dev dependencies (including pytest-mock); no model download is needed:
Exact revision
Base:
1e0c4f3d54a91c30de97aa332e772dfe7338f52cHead:
4998aa272cbe82984eb11ae54d28d3a26d1d74ceThe first ten intentional commits were true-rebased onto base
1e0c4f3dat head46318fb4.git range-diffmatched all ten commits, every commit retainsSigned-off-by:, AI attribution is preserved, and the aggregate binary patch is byte-identical before/after rebase (f3a40bd5d77a40785fac23234361561edde3ee85f345ef773315d76174d5aba9). No merge commit was introduced. One additional signed-off AuK guard/regression commit,4998aa272, is a direct child of46318fb4; those ten rebased commits remain untouched.Pre-rebase AMD evidence
AMD build #13506 is exact-head evidence for pre-rebase head
38fafcd021b61b23a073fce2b9cdaac2b69acafd, not qualification for the new head:pytest-{0..4}-of-5.xml, and uploaded JUnit.artifacts/joy-image-gpu/pytest.xml.shared_memorywarning after its successful pytest summary; artifact upload then succeeded. It is observational/noncausal follow-up evidence, not a test failure.Rebased-head validation
.venv/bin/python -m pytest tests/buildkite/test_amd_pipeline.py tests/buildkite/test_amd_bootstrap.py tests/buildkite/test_amd_suite_selection.py tests/buildkite/test_rocm_dockerfile.py -v-> 64 passed.[1540, 1627, 1653, 1596, 1617]; all nonzero and pairwise disjoint, with union exactly equal to unsharded.torch.cuda, and Buildkite schema. Manualmypy-3.12,git diff --check, DCO/trailer audit, and diff-preservation checks passed.A broader local mixed CPU/GPU invocation is not claimed as passing: this host has mismatched vLLM 0.31.0 / vLLM-Omni 0.30.1 and reports
UnspecifiedOmniPlatform, so platform-construction cases failed locally. The valid ROCm runtime evidence is #13506, and the rebased head still needs fresh AMD qualification.Merge qualification still required
This PR is open and ready for review, but not merge-ready. Under the revised validation plan, require fresh final-head AMD READY and MERGE execution whose Buildkite commit equals
4998aa272cbe82984eb11ae54d28d3a26d1d74ce, including both five-shard sets, MiniMax solve-rule execution and affected MiniCPM cases. Require the AuK CPU regressions to pass on ROCm, the ten unsupported FP8 GPU cases to skip on HIP, and retained NVIDIA FP8 numerical/graph cases to execute successfully. Hosted checks, CUDA/NPU checks, and human approval also remain required.Reuse #13506's ten-case JoyImage default-AITER pass with readable JUnit/durations as verified equivalent-rebase evidence: the original ten-commit series and its aggregate patch match across the rebase, its eight test/CI files remain identical, and the new base did not change JoyImage, diffusion attention, CI routing or dependency/image pins. This is not a claim that the final SHA itself ran the JoyImage GPU lane. The subsequent AuK-only commit does not modify JoyImage, shared attention, CI routing or dependency/image pins, so the documented evidence-reuse scope remains unchanged. A full fresh AMD nightly is not required solely for this byte-identical rebase. Rerun only targeted JoyImage coverage if relevant code/runtime dependencies or actual ROCm/AITER image provenance change, or a maintainer requests exact-head hardware evidence.
The
nightly-testlabel was removed,readywas refreshed once withmerge-testpreserved, and previous-head READY/MERGE qualification was registered as AMD #13507 / CUDA #16916 / Intel #9789 / NPU #8635. Those builds cover the previous head, not the AuK follow-up. No healthy builds were canceled or queued GitHub Actions runs restarted.The only production behavior change is rejecting AuK's unsupported HIP FP8 opt-in and using its existing model-dtype fallback. NVIDIA FP8 arithmetic, kernels and numerical/graph assertions are unchanged; no new accuracy or performance claims are made. Timeout budgets remain temporary headroom while cold-runtime evidence accumulates.
AI assistance: Cursor agents helped investigate CI, preserve/rebase the commit series, run validation, and draft this description. OpenAI Codex drafted the AuK HIP guard and CPU regressions, ran isolated logic/static checks, and prepared this description update. The human contributor remains responsible for reviewing every changed line and validating final behavior.