Repository navigation
Conversation
Signed-off-by: Linze-Shi <linzeshi0@gmail.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. |
|
This PR appears to belong to: docs/design/module/diffusion/diffusion_model_integration.md. Module owners: @Bounty-hunter @fhfuih @RuixiangMa Routing: @Bounty-hunter via semantic router; @fhfuih via semantic router; @RuixiangMa via semantic router @LinzeShi, 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. |
|
@vllm-omni-review-bot I would like an automated review |
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
Dong1017
left a comment
There was a problem hiding this comment.
Reviewed 0771468e3633 (single commit, unchanged since opening). Test-only (+104/−4); custom_op.py is byte-identical to main.
Closes the systematic-coverage gap flagged in #7382: 6-way branch selection (native/cuda/rocm/npu/xpu/musa) with argument-identity forwarding via op(...), inherited fallbacks (xpu→native, rocm→cuda, musa→cuda), override precedence, missing-backend NotImplementedError, and a dispatch-at-construction assertion (predicates poisoned with side_effect=AssertionError post-__init__). The MoTRMSNorm regression now drives the real norm(x) path. 6+3+3+4+3 = 19.
Traced all cases against the production custom_op.py; the mock targets the module global production actually reads, so the dispatch logic under test is real. Independently reproduced the author's run (H200 host, vllm 0.29.0 / torch 2.13.0+cu130, 2026-09-20): 19 passed in 15.25s from an archive of this exact head — machine-local evidence, since this repo's CI runs no unit-test lane here. The SPDX edit (vLLM → vLLM-Omni) matches this repo's own check_spdx_header.py.
Boundary: every new case mocks the platform — nothing here exercises real CUDA/NPU kernels or qualifies a device path, consistent with the PR description.
Two non-blocking notes inline.
Signed-off-by: Linze-Shi <linzeshi0@gmail.com>
Apply the mkdocs.yml change from upstream commit 8953d08 (vllm-project#8065). Disable the three optional inventories returning HTTP 429 while retaining strict documentation checks. Signed-off-by: Linze-Shi <linzeshi0@gmail.com>
Signed-off-by: Linze-Shi <linzeshi0@gmail.com>
Omni ReviewBot: no human activity for 7 days@LinzeShi this pull request has had no human commit, comment or review since 2026-09-23. 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. |
|
@Dong1017 I addressed both notes and added the unmocked smoke test. Could you take another look? Thanks! |
|
@LinzeShi Thanks for the update. At I independently ran the focused file on H200 with vLLM 0.30/PyTorch 2.13, using a scoped pytest config: 19 passed, 1 skipped. The tensor cases ran on CPU; the new native-platform smoke correctly skipped on the real CUDA platform. Your reported CPU-only 20-pass run therefore remains author evidence for that smoke. No further change requested from this focused follow-up. |
|
@hsliuustc0106 Dong1017 has reviewed the update and confirmed no further changes are needed. Could you take a look or suggest a maintainer for the final review? |
Omni ReviewBot routing recordAssigned Strict on cursor (cursor-grok-4.6-high) under experiment |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 120s (try 2 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 600s (try 3 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; falling back to direct/cursor/auto). |
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
Changes since the previous review
- 0 new inline finding(s); 0 finding(s) below.
CI at
4fe204a1ad40(2026-10-10T14:59:42.917937+00:00): required check(s) blocking:buildkite/vllm-omni(missing).
Note: The assigned review arm
strict/cursor/cursor-grok-4.6-highcould not complete this review, so it was produced by the fallback armdirect/cursor/auto. It is excluded from the routing experiment.
Full review analysis
PR description
This pull request only updates tests/diffusion/layers/test_custom_op.py. A new mock_platform fixture replaces current_omni_platform with a five-predicate mock, and new cases check constructor dispatch, argument identity, inherited xpu/rocm/musa fallbacks, override precedence, and NotImplementedError when a backend method is missing. An unmocked smoke test constructs _NativeOnlyOp and expects op(x) to equal x * 2 when no accelerator predicate is selected; that case skips on an accelerator platform. The existing MoTRMSNorm regression now calls norm(...) after setting gen_weight to 2, instead of calling forward_xpu directly.
Change flow
flowchart LR
platform["[EXISTING] CustomOp platform predicates"]:::existing
fixture["[NEW] mock_platform fixture"]:::new
cases["[NEW] Dispatch, fallback, override, and missing-backend cases"]:::new
smoke["[NEW] Unmocked native op(x) smoke"]:::new
mot["[CHANGED] MoTRMSNorm norm(x) regression"]:::changed
platform --> fixture --> cases
platform --> smoke
fixture --> mot
classDef existing fill:#e5e7eb,stroke:#6b7280,color:#111827
classDef changed fill:#fef3c7,stroke:#d97706,color:#451a03,stroke-width:2px
classDef new fill:#dcfce7,stroke:#16a34a,color:#052e16,stroke-width:2px
classDef removed fill:#fee2e2,stroke:#dc2626,color:#450a0a,stroke-width:2px
🤖 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!
Purpose
Add the focused
CustomOptest coverage requested in #7382: constructor dispatch, argument forwarding, backend overrides and inherited fallbacks, and missing backend implementations. TheMoTRMSNormregression exercises the normal module call path.An unmocked native-platform smoke test also checks
op(x)againstx * 2, covering real dispatch and bound-method invocation. It skips when an accelerator backend is selected.This PR changes tests only; it does not qualify device kernels.
Test Plan
Test Result
Python 3.12.4 in both environments:
The CUDA run skips the native-platform smoke test as intended; the remaining tests use CPU tensors. Both runs report 14 existing
torch.jit.script_methoddeprecation warnings. Changed-file pre-commit checks passed.