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. |
d7cdd26 to
73a881a
Compare
|
Self-review: |
73a881a to
4033cd9
Compare
|
Independent verification (H200, vllm 0.29.0 / torch 2.13.0+cu130 / Triton 3.7.1; e2e on
Findings — nothing blocking (details inline): the SP check in the z_image helper duplicates the packer's new RFC #7382 — fits as a consumer-adoption PR (Goal 3, no kernel surface added; the Phase-1 list names only H3/Boogu, covered for later consumers by the RFC's "subsequent work" clause). Coordination — nothing blocking: linking RFC #7382/#7417/#7422 in the description would help public-entry reviewers, and this PR can follow #7560's rebase to the canonical |
8829773 to
6dc3f91
Compare
Omni ReviewBot: no human activity for 7 days@yuweih205 this pull request has had no human commit, comment or review since 2026-09-21. 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: 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 cursor (cursor-grok-4.6-high) under experiment |
Signed-off-by: HuangYuwei <yuweih205@gmail.com>
ee9f924 to
5522ee9
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>
5522ee9 to
02984be
Compare
|
@hsliuustc0106 Both of your review comments are 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. |
|
@hsliuustc0106 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>
c89e0b3 to
ffdb710
Compare
Reproducible eager bitwise case: retained tiny-model resultsThe already measured tiny-model eager configuration has equal gate-off/gate-on output on the tensor checked by the original harness, with fusion active at all four attention sites. Its retained single-GPU transformer-forward timing is 6.647760 → 6.167152 ms, a 7.229623% saving. The original script, source overlay and 17 SP/rank records have now been recovered. These are September 17 measurements already summarized in this PR; no new GPU measurement is being reported, and the existing full pretrained-model performance results are retained. Configuration change for this observed case: use the tiny random-weight constructor below, eager execution without ZImageTransformer2DModel(
all_patch_size=(2,), all_f_patch_size=(1,), in_channels=16,
dim=512, n_layers=2, n_refiner_layers=1,
n_heads=4, n_kv_heads=4, cap_feat_dim=64,
).eval()Exact settings:
Equality scope: the old harness checks the first returned image tensor only, after the complete tiny-model forward. It converts the BF16 tensor to FP32 and applies The original rounded SP timings remain:
Timing uses CUDA-event medians, three warmups per arm, then A B B A with ten complete forwards per segment. These small-model timings are not end-to-end image-generation timings. The existing pretrained-model 1.7% / 1.9% performance figures remain separate; those pretrained outputs are not bitwise equal to the eager baseline. Reproduction uses base PYTHONPATH="$RUN_DIR" ZSP_REPORT="$RUN_DIR/report.json" "$PYTHON" "$RUN_DIR/zimage_sp_check.py" '2:ulysses,4:ulysses,8:ulysses,2:ring'The retained harness SHA256 is AI assistance: Codex recovered and checked the existing harness/source hashes/raw records and prepared this additive reproduction note; no GPU case was rerun for this note. |
Pretrained eager generation: bitwise parity and measured acceleration@hsliuustc0106 The numerical attribution and performance follow-up are complete. Matching the CUDA RMSNorm reduction order preserves bitwise output while retaining a full-request speedup in the tested Z-Image generation cases. Tested implementation: prepared source Configuration: pretrained Original:
Wall request latency includes text encoding, all denoising steps, VAE and PIL image output; loading the model is excluded. One complete warmup request per arm precedes three original/exact/exact/original rounds, six timed requests per arm. Debug traces, tensor saves, provider hooks and counter wrappers are absent from timed calls. The complete VAE float output and RGB uint8 image are compared by storage bytes before and after timing. Every arm repeats exactly. Actual fused calls are 0 for original and 272/952 for fusion at 8/28 steps. Every timed exact request is faster than every paired original sample in these two cases. This establishes parity and performance for the pinned checkpoint, prompt, seed and eager configurations above. The preceding same-input investigation located the first difference at QK RMSNorm FP32 reduction: raw QKV and RoPE coefficients matched; CUDA and default Triton summation orders differed. Reproducing CUDA’s order closed the eager full-generation gap. The current fast arm’s separate parity and timing records are retained in the raw JSON. Raw full-generation parity and ABBA samples, harness, and the reproduction procedure and all raw results. AI assistance: Codex ran and checked the GPU investigation and performance 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 (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). |
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
- 1 new inline finding(s); 0 finding(s) below.
CI at
ffdb71041bae(2026-10-10T18:07:13.168845+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 change turns Z-Image's per-site Q/K RMSNorm and interleaved RoPE into one CUDA Triton launch of the shared fused_qk_norm_rope operator. Each forward packs one [cos | sin] table from row 0 of the padded cos/sin pair, matching the existing half-head RoPE broadcast, and passes it through the noise refiner, context refiner, and main blocks. The token gate defaults to 0, so CUDA bf16 sites fuse unless VLLM_OMNI_FUSED_QK_NORM_ROPE_MIN_TOKENS is raised; other devices and dtypes keep the eager chain. The same head also carries the shared joint QKV kernel, packer, and operator tests so this model PR can merge without waiting on the other consumers.
Change flow
flowchart LR
A["[EXISTING] Z-Image x, cap, and unified sites"]:::existing
B["[NEW] pack row-0 cos/sin table"]:::new
C["[CHANGED] ZImageAttention fused gate"]:::changed
D["[CHANGED] fused_qk_norm_rope Triton launch"]:::changed
E["[EXISTING] vLLM RMSNorm then RoPE"]:::existing
F["[NEW] Z-Image fusion tests"]:::new
A --> B --> C
C --> D
C --> E
D --> F
E --> F
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
See inline comments below.
🤖 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!
| # RMSNorm -> RoPE chain; fuse by default (the fused path won at every size | ||
| # measured on H200 for this chain, see Flux.2 / Boogu-Image) and keep the | ||
| # gate for VLLM_OMNI_FUSED_QK_NORM_ROPE_MIN_TOKENS overrides. | ||
| _FUSED_MIN_TOKENS = 0 |
There was a problem hiding this comment.
[P2] Default Z-Image fusion changes sampled images
Evidence and suggested fix
_FUSED_MIN_TOKENS = 0 packs a table for every CUDA bf16 forward, so ZImageAttention replaces vLLM RMSNorm plus apply_rope_to_qk with the Triton reduction. The in-repo attention check only allows atol/rtol 0.05, which still passes when Q/K differ by far more than one ulp. This PR's own Z-Image-Turbo record reports eager-versus-fused PSNR median 33.5 dB and minimum 21.6 dB, including a visible pose change, and the later CUDA-order bitwise preset is explicitly not in public head ffdb710. Set the Z-Image default above real sequence lengths, or land that measured reduction preset, so the fused launch does not become the default image contract.
Independent merge update (2026-10-08)
Current head:
02984be8on 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:
ee9f92405. 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.ee9f92405: 4 passed, covering row-zero semantics, sequence-parallel table construction, and fused/eager attention comparisons. 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, for the single-stream Z-Image transformer (
ZImageAttentionin the 30 mainblocks and the noise/context refiners): the per-site two RMSNorms + two RoPE passes become one
launch of the shared
fused_qk_norm_rope(..., interleaved=True)op (#6982), writing the rotatedQ/K attention consumes directly.
[cos | sin]table per attention site per forward (x,cap,unified), builtfrom row 0 of the padded
[B, S, D/2]cos/sin exactly asRotaryEmbeddingapplies them(
_prepare_half_head_dim_cos_sinusescos[0]for every batch element), in the activation dtype.(
_sp_planshards onlyunified_prepare's outputs), and at the unified site the table is packedafter that sharding, so
cos/sinare already this rank's shard — the same coefficients theeager chain rotates this rank's tokens with. All three sites fuse under SP; verified on 2 GPUs
below. Any unsupported dtype/geometry still keeps the eager chain. Default gate
_FUSED_MIN_TOKENS = 0; env override unchanged.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
z_image/andfused_qk_norm_rope.pyfrom main — see #7560.vLLM-Omni Commit: branch on
1b6cd28+ #7560. Hardware: 1× NVIDIA H200.tests/diffusion/models/z_image/test_z_image_fused_qk_norm_rope.py: table followscos[0],gate, CPU skip, and a table is still packed under a sequence-parallel forward context;
ZImageAttentionfused vs eager output.both refiners) with real NCCL groups and the model's own
_sp_planhooks, world 1 vs world 2(
ulysses_degree=2), gate off vs on in the same process, plus a runtime counter asserting thefused op really runs (and never runs in the gate-off arm).
Tongyi-MAI/Z-Image-TurbothroughOmni(model=..., mode="text-to-image"), 1024²,8 steps (Turbo default) and 28 steps, 3 prompts × 2 seeds × 2 repeats per arm, gate off vs on,
A B B A, wall-clock per
generate()including text encoder and VAE.Test Result
Unit-test inventory: the module now contains 4 tests: CPU fallback,
row-zero/gate/dtype behavior, table packing under a sequence-parallel forward context,
and fused-vs-eager attention. Three require CUDA and Triton.
Review follow-up validation (2026-09-20): the current module returned
1 passed, 3 skipped on CPU (Python 3.12.3, vLLM 0.29.0, torch 2.13.0+cu130,
diffusers 0.40.0, transformers 5.14.1). All three skips explicitly require CUDA.
The environment-variable reference now includes Z-Image's default gate of
0.All applicable changed-file pre-commit hooks passed, including markdownlint and typos.
The CUDA unit suite was not rerun for this documentation-only follow-up; the H200
end-to-end and sequence-parallel measurements below are the original implementation
validation, not new measurements on the documentation follow-up.
End-to-end, 1024²:
Peak allocation unchanged (21.6 GiB). The saving is modest because Z-Image only has the
single-stream chain (two norms + two RoPE per block, no cats) and 30-head × 128 blocks where
attention/MLP dominate; it is nevertheless free.
Sequence parallelism (H200, tiny random-init transformer: 2 main layers + both refiners, so 4
attention sites per forward). Real NCCL groups and the model's own
_sp_planhooks; gate off vson in the same process; a runtime counter asserts the fused op actually runs (and never runs in the
gate-off arm):
All four sites (both refiners and the two main layers) take the fused path on every rank of every
configuration. The "vs world-1" column is the same number to every digit in the gate-off and
gate-on arms (e.g. 0.004646282005149976 for ulysses 2), i.e. it is sequence parallelism's own
reduction order, not the fused path.
_sp_plandeclares noauto_pad, so a sequence that does notdivide evenly is rejected before any of this; the shards are exact and
cos/sinare split by thesame rule as the hidden states.
"Bitwise identical" here is this small configuration: the ≤1-ulp rounding difference described
under Numerics does not survive to the output at this depth. The comparison across parallelism
settings is the point — sequence parallelism changes neither the relationship between the two arms
nor the size of the saving. Absolute times are from that small model and are not an end-to-end
figure.
Numerics: arms bitwise deterministic across runs; eager vs fused images PSNR median 33.5 dB
(24 pairs), minimum 21.6 dB on one prompt/seed where the fox's pose shifts slightly — the same
≤1-ulp rounding-order difference of vLLM's
rms_normvs the kernel analysed in #7560, amplifiedby sampling.
Control arm. A third arm runs the eager path with vLLM's
RMSNormswapped for a torchimplementation with the kernel's single-rounding order (
sitecustomizepatch in the worker,nothing else changed). That arm vs eager: PSNR median 33.2 dB, min 20.8 dB (24 pairs) — the
same spread as fused vs eager (33.5 / 21.6). Fused vs the control arm: 33.2 / 23.6 dB. The
rounding order alone accounts for the whole difference.
🤖 Generated with Claude Code
https://claude.ai/code/session_01G8uaASBAbnS8TsFvC5gCUD
AI assistance for this review follow-up: Codex updated the gate documentation, checked the four-test inventory, ran the CPU and pre-commit checks above, and refreshed this validation record.