Repository navigation
[Hardware][Ascend] Optimize Wan VAE spatial padding - #8694
scestola-h wants to merge 1 commit into
Conversation
|
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 belong to: docs/design/module/diffusion/index.md. Module owners: @princepride @david6666666 @Isotr0py Routing: @princepride via module of the changed files, CODEOWNERS; @david6666666 via module of the changed files; @Isotr0py via module of the changed files @scestola-h, 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: three questions on the performance claim@scestola-h this PR reads as a performance or value claim:
Before the full evidence checklist, three short questions:
When you answer, the evidence that settles it is: base and head SHA, hardware, model, workload, warm-up and repeat count, mean or percentiles with their spread, and a correctness/quality-equivalence signal; an end-to-end claim also needs stage attribution. |
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
2 actionable finding(s).
CI at
72f91143cee1(2026-10-10T16:04:28.873339+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
The change adds an Ascend branch to the shared Wan VAE lossless fast path. When decoder parameters are already on NPU, the installer rebinds only WanCausalConv3d: temporal cache assembly stays an explicit concat and pad, and height/width padding is passed into F.conv3d so the spatial padded activation is not materialized. The first call for each module, input shape, dtype, and layout runs that formulation against the pre-padded convolution, keeps the result only when the two are bitwise equal, and otherwise caches a fallback to the Diffusers forward. CUDA still installs the existing fused decoder path, and the A3 ready step plus the decode benchmark now cover this NPU path.
Change flow
flowchart TD
A["[EXISTING] Wan VAE decode"]:::existing --> B["[CHANGED] Registry allows Ascend"]:::changed
B --> C["[CHANGED] Installer binds causal Conv3D"]:::changed
C --> D["[CHANGED] Spatial pad inside F.conv3d"]:::changed
D --> E["[EXISTING] Bitwise verdict or Diffusers fallback"]:::existing
D --> F["[NEW] A3 causal-conv exactness tests"]:::new
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!
|
|
||
|
|
||
| def test_registry_hook_skips_non_cuda_platform_and_non_wan_vaes(mocker) -> None: | ||
| def test_registry_hook_supports_npu_and_skips_other_platforms_and_non_wan_vaes(mocker) -> None: |
There was a problem hiding this comment.
[P1] CPU diffusion guard for the registry change is not in the reported runs
Evidence and suggested fix
The modified registry test is selected by the ready-pipeline step "Simple · Diffusion Test" in .buildkite/cuda/test-ready.yml, whose command is pytest -sv tests/diffusion -m 'core_model and cpu' (no --run-level). test_wan_vae_fastpath_install.py is marked core_model and cpu, so that step is the guard for this change. The PR Test Plan and Test Result report the new A3 causal-conv cases and the changed-file pre-commit hooks (Ruff, mypy, markers, SPDX, import and torch.cuda policy, Buildkite schema, typos). They do not report this CPU selector, and they do not say the CPU diffusion suite was left unrun. Please run that command, or state the gap explicitly.
There was a problem hiding this comment.
The registry coverage is marked core_model + cpu, so it is selected by pytest -sv tests/diffusion -m 'core_model and cpu'. I attempted that exact selector locally, but the available container has a stale vLLM dependency and collection stops before selection (MultiModalProcessingResult is absent). I have documented that limitation in the PR body rather than claiming a pass; the ready-pipeline Buildkite result is still required before merge.
| return _skip(level, f"vae_parallel_mode={parallel_mode!r} is not supported") | ||
|
|
||
| device = next((p.device for p in decoder.parameters()), torch.device("cpu")) | ||
| npu_spatial_padding_only = device.type == "npu" |
There was a problem hiding this comment.
[P2] Ascend-only binding depends on parameter device, not the NPU platform
Evidence and suggested fix
The registry turns the fast path on when current_omni_platform.is_npu() is true, but install_wan_vae_fastpath sets npu_spatial_padding_only only when the first decoder parameter's device type is already npu. A CPU-resident VAE on an NPU platform therefore receives the full CUDA replacement map (WanDecoder3d, residual blocks, resample, upsample, and RMSNorm), and channels_last is not refused. Those replacements are not a no-op on NPU: rms_norm_fastpath still returns its restructured vector_norm path when the Triton kernels decline, and a channels_last install also converts convolution weights. The new CPU test calls the registry hook with is_npu=True on a default CPU VAE and only asserts is_installed, so it accepts that full bind. The A3 installer test does check the causal-conv-only set, but only after .to("npu"). Pipeline construction is not in this sparse checkout, so it is not proven that serving always installs after the VAE is on NPU. Gate the limited bind and the channels_last refusal on the NPU platform as well as parameter device, and assert report.patched in the CPU registry test for a module that is not on NPU.
There was a problem hiding this comment.
Fixed in 0080a6fa8. The registry now passes an explicit NPU-only mode based on current_omni_platform.is_npu(), so installation is safe even while the VAE parameters are still on CPU. In that mode it binds only WanCausalConv3d, never installs the CUDA-oriented encoder/RMSNorm/resample rewrites, and rejects channels_last. The CPU registry test now asserts the exact patched set is {WanCausalConv3d} and that the encoder installer is not called.
Omni ReviewBot: finding feedback[p1] CPU diffusion guard for the registry change is not in the reported runs — See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement. |
72f9114 to
0080a6f
Compare
|
Performance evidence after rebasing onto current main (
The focused current-head A3 run also passed all 12 BF16/FP32 cache/no-cache cases on both first-verification and cached-verdict calls. |
|
Author self-review of
AI coding assistants helped with the rebase review, tests, and wording; I reviewed all changes and ran the reported validation. |
|
Ran this PR's own
On this stack (CANN 9.1.0) the win is ~6% of VAE decode: the spatial |
0080a6f to
3f75d40
Compare
Omni ReviewBot: no human activity for 10 days@scestola-h this pull request has had no human commit, comment or review since 2026-10-16. 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. |
3f75d40 to
63f2379
Compare
|
Rebased onto current main after #8968 added XPU support to the shared Wan VAE fast path. The conflict resolution preserves the complete CUDA/XPU decoder and encoder paths and keeps this PR limited to the lossless causal-Conv3D spatial-padding path on NPU. New head: |
psv666
left a comment
There was a problem hiding this comment.
Status on head 63f23797a (base 13c1e8228): no new code findings; the Ascend binding scope is unchanged and correct.
- Ascend path (
causal_conv_forwardNPU branch,_conv_verdict_key,_conv_with_spatial_padding, the NPU install mode and the registrynpu_spatial_padding_only=is_npucall) is byte-identical to0080a6fa8, the tree I measured on 8x910B1 on 2026-10-16; #8968 only widened the registry's full-path branch to XPU, so that result still applies here. - Merge state is CONFLICTING/DIRTY: the only textual conflict is the
_conv_with_spatial_paddingdocstring inforwards.py(main's #9424 wording vs this PR), so the rebase is trivial, not semantic. - When resolving, keep
verify_spatial_padding=Truefor Ascendlossless; #9424 skips the first-call bitwise probe only underchannels_last, which Ascend already rejects. - One confirmation run after rebase: Wan2.2 TI2V VAE, bf16,
--fast-path off,lossless, 480x480/33 and 832x480/81 frames, single 910B1 (the base moved through #9424 memory changes).
Co-authored-by: Diego Didona <diego.didona@huawei.com> Signed-off-by: Samuel Cestola <samuel.cestola@h-partners.com>
63f2379 to
26185ff
Compare
|
Thanks for the exact conflict analysis. Rebased onto current main ( For the requested post-rebase hardware confirmation: our available pool is Atlas A3 ( |
psv666
left a comment
There was a problem hiding this comment.
Rebase verified on 26185ff1b (base 3a936aee8).
- Patch-level compare against the head I previously reviewed (
63f2379): the only content delta is the_conv_with_spatial_paddingdocstring resolution; every other hunk difference is a base line-number offset. verify_spatial_padding=Trueis retained for Ascend: NPU installs atlevel="lossless", soFastPathConfig.channels_lastis False and the newchannels_last and not verify_spatial_paddingearly return inforwards.py:358is never taken. The bitwise probe still runs.- NPU binding is still
WanCausalConv3donly (35 modules patched in this run),channels_lastis still refused on NPU (install.py:169), and CUDA/XPU keep_REPLACEMENT_FORWARDSunchanged.
Local re-run, one 910B1, ab_snap:v31 (torch 2.10.0+cpu, torch-npu 2.10.0.post4), Wan2.2-TI2V-5B VAE, bf16, warmup 1 / iters 5, arm order alternating per round:
| size / frames | off ms | lossless ms | ratio | repeats |
|---|---|---|---|---|
| 480x480 / 33 | 1042.9 | 990.3 | 1.053x | 3 |
| 832x480 / 81 | 4311.6 | 4055.7 | 1.063x | 2 |
torch_equal=True with max abs diff 0 at both sizes. Peak allocated on lossless is +0.02 GiB (4.045 -> 4.062 and 6.255 -> 6.275), the same sign as your +0.06 GiB.
Versus my 2026-10-16 run on 0080a6fa8 (1038.1 -> 986.3 ms, 1.052x): +4.8 ms off and +4.0 ms lossless, i.e. +0.4-0.5% in both arms with the ratio unchanged. A uniform shift in both arms is host-level drift, not an arm-specific regression, and it is within what I see between sessions on this shared machine. The new 832x480/81 point shows the benefit growing slightly with workload size (6.3% vs 5.3% wall time).
Remaining gaps are unchanged from my previous review and are not new blockers: Ascend exposes only the decoder lossless level (channels_last and the encoder fast path stay CUDA/XPU-only), spatial-shard decode stays excluded, the first call per (module, shape) still pays the bitwise probe, and this evidence is a single 910B1 die rather than the eight A3 dies in your table.
Purpose
Extend the shared Wan VAE fast path from #7056 with a lossless Ascend path for
causal Conv3D spatial padding.
Diffusers materializes temporal and spatial padding with
F.padbefore everyWan causal Conv3D. On Ascend, keeping temporal/cache assembly explicit but
passing the height/width padding to
F.conv3davoids that spatial padded copy.The first call for each module/input-shape/dtype/layout runs both formulations
and compares them bitwise. A mismatch returns the reference result and caches a
fallback verdict for that shape.
The installer intentionally binds only
WanCausalConv3don NPU. CUDA and XPUretain their complete existing fast paths unchanged,
channels_lastremainsCUDA/XPU-only, compiled or training calls use the Diffusers implementation,
and spatial-shard decode remains excluded.
This adapts the Helios-local prototype by Diego Didona (
cuoredicervo,ef47d374e) to the existing shared fastpath instead of adding a model-specificmonkeypatch.
Test Plan
Hardware: eight Ascend A3 dies (physical IDs 8–15), one die per run. Runtime:
PyTorch 2.10.0 + torch-npu 2.10.0.post4, Diffusers 0.40.0,
npu-smi25.2.3.Focused A3 coverage:
WanCausalConv3d;channel/spatial shapes;
path for exact equality.
Real-weight A/B command (run independently on dies 8–15):
Each reported die time is the minimum of five synchronized measurements after
one warmup. The summary reports mean ± population standard deviation and range
across the eight dies.
Local checks: complete changed-file pre-commit suite (Ruff, mypy 3.10, test
markers, SPDX, forbidden imports, torch.cuda policy, Buildkite schema, typos)
plus Markdown lint.
The exact ready-pipeline CPU selector requested in review is
pytest -sv tests/diffusion -m 'core_model and cpu'. The added registry test isselected by it; the local host does not have the vLLM-Omni test environment, so
the result will come from the PR's Buildkite run rather than being claimed as a
local pass.
vLLM Version: 0.29.0 runtime package with the PR's self-contained Wan VAE
fastpath overlaid; the checkout targets the current vLLM 0.31 dependency.
vLLM-Omni validation Base / Commit:
156ddb3f4/0080a6fa8f4cf95b4ae74d98dd165331b286e4daCurrent PR head:
26185ff1b, rebased onto3a936aee8on 2026-10-30.The latest conflict was documentation-only: upstream #9424 lets tolerance-based
CUDA/XPU
channels_lastconvolutions skip the bitwise spatial-padding probe toreduce peak memory. The resolution retains that behavior while keeping
verify_spatial_padding=Truefor Ascendlossless. The rebase's changed-filehooks, mypy, Buildkite validation, Python compile, diff checks, and pinned
Markdown lint all pass.
Test Result
Focused A3 validation passed: the installer patched 16
WanCausalConv3dmodules and no other module type; all 12 numerical cases werebitwise exact on the first and cached calls.
offlosslesstorch_equal=True, bit-equalThe original model-local prototype also measured 12.099 s → 11.883 s (1.8%)
for a 264-frame Helios end-to-end run with byte-identical output. That result is
supporting context only; the table above is the validation of the shared
fastpath implementation in this PR.
The eight-die table was collected before rebasing over the encoder fast-path
work in #8067. On the rebased head, a fresh die-15 A/B measured 1977.0 ms →
1835.1 ms (1.077x) with
torch_equal=True, bit-equal output, and maximumabsolute difference 0. The current-head focused run also passed all 12 numerical
cases and confirmed that only
WanCausalConv3dmodules were patched.AI assistance: Used AI coding assistants to help adapt the existing Wan VAE
fast-path architecture, draft tests and documentation, and prepare the PR text.
I reviewed the changes and validated them on Ascend A3.