Repository navigation
Conversation
|
This PR appears to belong to: docs/design/module/diffusion/diffusion_model_integration.md, docs/design/module/diffusion/index.md. Module owners: @wtomin @RuixiangMa @david6666666 Routing: @wtomin via module of the changed files, module named in the PR description, semantic router, CODEOWNERS; @RuixiangMa via module of the changed files, module named in the PR description, semantic router; @david6666666 via module of the changed files, module named in the PR description, CODEOWNERS @yuweih205, 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. |
…l cannot run Same gate as the shared helper (review on vllm-project#7600): no [B*S, D] allocation or copy on CPU/NPU/ROCm or non-bf16 activations. Signed-off-by: HuangYuwei <yuweih205@gmail.com>
35d7b9a to
a2b44d0
Compare
|
@RuixiangMa Thanks, done in #7560: |
|
Self-review: |
|
Reviewed at a2b44d0 (delta over #7560: Non-blocking: |
…l cannot run Same gate as the shared helper (review on vllm-project#7600): no [B*S, D] allocation or copy on CPU/NPU/ROCm or non-bf16 activations. Signed-off-by: HuangYuwei <yuweih205@gmail.com>
b4590fe to
e1dc249
Compare
0z5a
left a comment
There was a problem hiding this comment.
Cross-architecture validation on an RTX 5090 against bbf831e (torch 2.13.0+cu130, BF16): the two PR attention tests and an additional one-double/one-single-block OvisImageTransformer2DModel.forward test passed (3/3). The model-level test exercised one RoPE-table pack, one fused joint attention call, and one fused single-stream call; fused and eager outputs matched at atol=rtol=0.05.
Median synchronized wall time for the same synthetic transformer, 12 runs per path:
| Image tokens | Total tokens | Eager (ms) | Fused (ms) | Speedup |
|---|---|---|---|---|
| 16 | 32 | 4.933 | 3.874 | 1.27× |
| 64 | 80 | 5.193 | 3.908 | 1.33× |
| 256 | 272 | 4.992 | 3.905 | 1.28× |
| 1024 | 1040 | 5.034 | 3.933 | 1.28× |
These are RTX 5090 synthetic transformer results; I did not run a pretrained Ovis checkpoint or measure H200 performance. I found no issue in the tested path. LGTM.
Omni ReviewBot: no human activity for 7 days@yuweih205 this pull request has had no human commit, comment or review since 2026-09-28. 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. |
Omni ReviewBot routing recordAssigned Strict on zcode (GLM-5.3-Flash) under experiment |
Signed-off-by: HuangYuwei <yuweih205@gmail.com>
bbf831e to
237a36a
Compare
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
Signed-off-by: HuangYuwei <yuweih205@gmail.com>
Signed-off-by: HuangYuwei <yuweih205@gmail.com>
237a36a to
26a3eac
Compare
|
I also do some optimization on these models, will check yours soon. If your work looks better and more brief, my optimization will be superseded |
|
@RuixiangMa Your table-allocation feedback is addressed on the current head ( The six-PR series (#7560, #7594, #7595, #7596, #7597, #7600) has also been restructured: each PR contains the same shared operator, tests and documentation changes, plus only its own model integration. There is no longer a prerequisite PR within this series; any one of the six can merge first, once its review and CI requirements are satisfied. This supersedes the earlier comments saying #7560 had to land first. The shared files are identical across the six heads, and local merge simulations passed for all 15 pairs and all 30 ordered pairs with the first PR squash-merged onto main. |
|
@RuixiangMa One additional numerical point for your re-review: If the concern is that fusion changes floating-point reduction/rounding behavior, operator-level bitwise alignment with a fixed uncompiled eager reference is achievable in principle: we can adjust the kernel's reduction configuration, intermediate precision and rounding points to reproduce that reference. The shared-operator experiment already demonstrated this for the tested vLLM 0.29 CUDA RMSNorm + RoPE contract on H200 (BF16, head dimension 128, epsilon If strict eager operator parity is the preferred acceptance criterion, please let me know; I can adapt and validate that variant for this PR. Equality against an Inductor-compiled full model would require a separate check. |
Signed-off-by: HuangYuwei <yuweih205@gmail.com>
96ec51b to
aba7d60
Compare
Supplemental eager bitwise measurements and queued production checkExisting PR performance results remain as recorded. The retained operator scripts and raw results stay pinned to their original artifact revision. Measured operator case: H200; B1, 256 text + 4096 image tokens, 24 heads, head dimension 128; BF16 activations/weights/RoPE table, full interleaved RoPE, epsilon
Eager CUDA-event medians: six balanced-order samples, 20 warmup calls, 100 calls per sample. These durations cover Q/K(/V) preparation with the packed RoPE table reused; table packing is excluded. They establish operator parity and latency for this case. Prepared production setting: change Actual r3 target: prepared source The three eager arms use the same initialized model/state/inputs:
H200, vLLM Source manifests and reproduction instructions and the eight-worker runner pin the settings. The eight-GPU r3 job is queued in MOVA2.0纯交付分区; full-model parity/performance is not yet established. |
|
Thanks for checking! This optimization was also validated on our internal diffusion workloads before upstreaming to vLLM-Omni. Feel free to take a look at the implementation — happy to discuss any suggestions or potential improvements. |
Fresh eager measurements: bitwise reference parity with retained speedup@RuixiangMa The H200 performance follow-up is now complete. The measured CUDA-reference numerical preset has zero output storage-byte mismatches versus the original eager chain, with a measurable transformer-forward saving. Tested implementation: prepared source Settings: Scope: eager, random-weight, full-width shallow Ovis-Image transformer, 24 heads × 128 dimensions, BF16, 4096 image/video tokens and 512 configured text tokens, B1/B2, seed
The original RMSNorm provider is pinned and checked as Timing: H200 141 GB, vLLM Raw records: B1, B2. See the reproduction procedure and all raw results. The fast arm and its separate timing/parity records are retained in those JSON files. AI assistance: Codex ran and checked the GPU measurements and prepared this additive evidence note. Please review the prepared numerical preset and the measured eager cases when convenient. |
Omni ReviewBot attempt recordReview attempt ended as failed (step 'review' (agent.review_diff): unhandled error: RuntimeError: zcode exited 1 without a result event: statusCode: undefined } Error: Turn execution failed (traceId: d05ee350-2109-49a0-82ba-b1de2e29212a) — check |
Omni ReviewBot attempt recordReview attempt ended as failed (step 'review' (agent.review_diff): unhandled error: RuntimeError: zcode exited 1 without a result event: statusCode: undefined } Error: Turn execution failed (traceId: 161fec01-cd8b-4c51-9818-3de0a54afb59) — check |
Omni ReviewBot attempt recordReview attempt ended as failed (step 'review' (agent.review_diff): unhandled error: RuntimeError: zcode exited 1 without a result event: statusCode: undefined } Error: Turn execution failed (traceId: 7752e74d-c475-4109-900a-9a5208eb50c3) — check |
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
aba7d60a3074(2026-10-10T11:36:40.024473+00:00): required check(s) blocking:buildkite/vllm-omni(missing).
Note: The assigned review arm
strict/zcode/GLM-5.3-Flashcould 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
Ovis-Image attention now builds one packed [cos | sin] table per forward and uses it in every block. Double-stream blocks launch the new joint Triton op so text and image Q/K RMSNorm, the text-then-image concatenation, and RoPE are written straight into the joint Q, K, and V that attention reads. Single-stream blocks run the existing interleaved fused op on the already-concatenated sequence. CUDA bf16 forwards take that path by default (_FUSED_MIN_TOKENS = 0); other devices, dtypes, and geometries keep the eager RMSNorm, cat, and RoPE chain, and VLLM_OMNI_FUSED_QK_NORM_ROPE_MIN_TOKENS can raise the token gate.
Change flow
flowchart LR
pos["[EXISTING] OvisImagePosEmbed<br/>txt then img cos/sin"]:::existing
pack["[NEW] pack_qk_norm_rope_table"]:::new
attn["[CHANGED] OvisImageAttention"]:::changed
joint["[NEW] fused_joint_qkv_norm_rope"]:::new
single["[CHANGED] fused_qk_norm_rope<br/>interleaved joint sequence"]:::changed
eager["[EXISTING] Eager RMSNorm cat RoPE"]:::existing
out["[EXISTING] Attention consumes Q/K/V"]:::existing
pos --> pack
pack --> attn
attn --> joint
attn --> single
attn --> eager
joint --> out
single --> out
eager --> out
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!
Independent merge update (2026-10-08)
Current head:
26a3eac0on mainc548a110; shared operator commits:bf9acacdand24f9e25e(64-bit indexing). The six PRs #7560, #7594, #7595, #7596, #7597, and #7600 now each contain this identical shared commit plus only their own model integration. Any one of the six can merge first. The other five do not import code from that model.The shared operator, test, and documentation files are byte-identical across all six heads. Local pre-commit checks passed (including ruff, mypy, SPDX, and markdownlint). Git merge simulations found no conflicts for all 15 pairs or for all 30 ordered pairs with the first PR squash-merged onto main. This Mac host has no CUDA/PyTorch test environment, so the GPU results below are historical results on earlier revisions; the new heads await CI and any GPU rerun.
Historical synchronization update (2026-09-21)
Current head:
bbf831efa. Merged upstream main1b87115c9ede2d29702c847b6da98f9e854fa593while preserving commit history.At that revision #7560 still landed first. The independent-merge update above supersedes this merge order.
64-bit row addressing to the joint QKV kernel. Added four CUDA regression cases
covering either large-offset stream and both RoPE pairings. Floating-point
arithmetic, model wiring, sequence-parallel behavior, and default gates are unchanged.
ruff, SPDX, test-mark checks, and markdownlint. A Python AST comparison also
verified that the existing model integrations and shared floating-point operations
were preserved (apart from widening address indices).
Operator-level GPU regression update (2026-09-21)
Operator-level correctness and regression tests for the current revision have now passed on real H200 hardware.
tests/diffusion/layers/test_fused_qk_norm_rope.pypassed on [Kernel][Flux.2] Fuse text/image QK RMSNorm + cat + RoPE into one Triton launch #7560 at08dc20e68: 30 passed. The 6 large-storage-offset cases also passed separately, including the 4 new dual-stream cases. Those 6 are included in the 30 and are not counted twice. The shared source and test files are byte-identical across the six current PR heads.bbf831efa: 2 passed, covering fused/eager attention comparisons for both double- and single-stream blocks. These are component/attention-level regressions, not full-model end-to-end tests.Scope: operator-level and model operator-integration regressions are now complete. Pretrained-model end-to-end accuracy and performance ABBA were not rerun. All historical accuracy and performance results below, including the two earlier measurements, are retained with their original revisions and environments; they are not new performance measurements for this revision.
To reproduce the shared operator run, execute from the
tests/directory of #7560 at08dc20e68, with the compatible CUDA/Triton environment and repository test dependencies above. The large-storage-offset cases require slightly more than 4 GiB of free GPU memory:python -m pytest -s -v diffusion/layers/test_fused_qk_norm_rope.py \ -m "core_model and cuda" --run-level=core_model -p no:cacheproviderRun the full operator-integration suite from the
tests/directory of this PR's pinned head:AI assistance for this synchronization: Codex resolved the merge, updated the indexing
regression coverage, ran the local checks and GPU regressions above, and verified the JUnit results.
Purpose
Follow-up to #7560 (Flux.2), same optimization for Ovis-Image (
OvisImageAttention, 6 double +27 single blocks): the double blocks' four RMSNorms + three
torch.cats + two RoPE passes andthe single blocks' two RMSNorms + two RoPE passes each become one Triton launch whose outputs
are the joint
[B, S_txt+S_img, H, D]Q/K/V attention consumes, with no intermediate copies.fused_joint_qkv_norm_rope(text first, as the eagercat); single blockscall
fused_qk_norm_rope(..., interleaved=True)over the already-joint sequence.[cos | sin]table per forward fromOvisImagePosEmbed's[S, D/2]output via theshared
pack_qk_norm_rope_tablehelper, passed to the blocks throughjoint_attention_kwargs(the model forward now passes that argument; the blocks already accepted it).
gate
_FUSED_MIN_TOKENS = 0,VLLM_OMNI_FUSED_QK_NORM_ROPE_MIN_TOKENSstill overrides.Independent branch: shared operator commit plus this model integration, based directly on main.
Test Plan
vLLM Version: 0.28.0 (torch 2.13.0+cu130, Triton 3.7.1), on the last pre-vLLM-0.29 tree with
ovis_image/(byte-identical to main) andfused_qk_norm_rope.pyfrom main — see #7560.vLLM-Omni Commit: branch on
1b6cd28+ #7560. Hardware: 1× NVIDIA H200.tests/diffusion/models/ovis_image/test_ovis_image_fused_qk_norm_rope.py:OvisImageAttentionfused vs eager output, double-stream and single-stream configuration.
AIDC-AI/Ovis-Image-7BthroughOmni(model=..., mode="text-to-image"), true CFG 5.0with a negative prompt, 1024², 50 and 20 steps, 3 prompts × 2 seeds × 2 repeats per arm, gate
off vs on, A B B A, plus a control arm (eager path with vLLM's
RMSNormswapped for a torchsingle-rounding implementation). Wall-clock per
generate()including text encoder and VAE.Test Result
Unit tests: 2 passed on H200 (plus #7560's op suite).
End-to-end, 1024², true CFG 5.0 (B=2 through the transformer):
Fixed cost outside the denoising loop ≈ 0.11 s; peak allocation unchanged (19.5 GiB).
Numerics:
40.0 dB — visually identical.
RMSNormswapped for a torch single-roundingimplementation, nothing else changed) vs eager: 47.6 / 41.9 dB; fused vs control: 47.7 / 40.6 dB.
The rounding order alone accounts for the whole difference, as in [Kernel][Flux.2] Fuse text/image QK RMSNorm + cat + RoPE into one Triton launch #7560.
fused-arm processes (two jobs), four were bitwise identical to each other and one (the first
of the first job) differed from them at 41–52 dB — the same ulp scale as torch.compile's own
compiled-vs-
enforce_eagervariation measured on the eager path (38.6 / 45.8 dB), so itlooks like an Inductor autotuning choice rather than the kernel (our Triton kernel has a fixed
launch configuration and is deterministic). With
enforce_eager=Truethe fused path wasbitwise reproducible across processes.
🤖 Generated with Claude Code
https://claude.ai/code/session_01G8uaASBAbnS8TsFvC5gCUD