Comfy-Org/comfy-kitchen

Fork 0

mirror of https://github.com/Comfy-Org/comfy-kitchen.git synced 2026-11-04 23:37:42 +00:00

[PR #199] [MERGED] [BugFix][Ascend] Gate RoPE tests on operator capabilities and clarify support #214

New issue

Closed

opened 2026-10-26 22:21:38 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:21:38 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-kitchen/pull/199
Author: @Big2Wheel
Created: 9/25/2026
Status: ✅ Merged
Merged: 10/2/2026
Merged by: @comfyanonymous

Base: main ← Head: fix/ascend-rope-capability-gates

📝 Commits (3)

85870f6 fix(ascend): gate RoPE tests on operator capabilities
28b7f85 fix(ascend): sort backend imports to unblock CI
f28f13e Fix import order.

📊 Changes

2 files changed (+33 additions, -18 deletions)

View changed files

📝 README.md (+13 -10)
📝 tests/test_ascend_rope.py (+20 -8)

📄 Description

Summary

Follow-up to merged #198, which consolidated #153, #154, #155 and #161 on top of #151. This PR changes tests, documentation, and backend import ordering to satisfy repository-wide lint; it does not change backend registration rules or inference implementations.

Addresses the two remaining review findings:

Changes

Fix the entry-point import-order lint error inherited from merged main, which blocked Linux and Windows CI before pytest. Full-repository ruff check . now passes.
Retain the Ascend-device gate and add separate RoPE / RMS-RoPE capability marks to execution, alias-safety and selection tests.
Mark unsupported-contract parameter cases individually: ordinary rotary cases require RoPE; bad_scale requires RMS-RoPE.
Keep operator-schema detection tests active when applicable rather than skipping the whole module because rotary support is missing.
Mark all eight Ascend RoPE/RMS-RoPE README rows as conditional; explain the rotary schema and RMSNorm requirements.
Name both npu_dynamic_quant and npu_quant_matmul in the INT8 linear description.

Validation of this follow-up

Single Ascend 910B3, physical card 4, same environment as the report below.

Scenario	Result
NPU regression: Ascend backend/RoPE, backend selection, ConvRot W4A4, INT8, input activation, constraints	277 passed, 172 skipped, one reference RMSNorm dtype warning
Simulated missing `npu_rotary_mul`	2 passed, 82 skipped
Simulated older rotary schema without `rotary_mode`	2 passed, 82 skipped
Simulated missing `npu_rms_norm`, rotary still available	49 passed, 35 skipped
Simulated torch-npu absent: RoPE module, backend selection and constraints	44 passed, 5 skipped
Ruff check / format check on changed Python; git diff --check	Passed

The compatibility scenarios ran in separate Python processes. The relevant torch-npu attribute was removed (or the rotary schema replaced with a minimal schema lacking rotary_mode) before importing comfy_kitchen and collecting tests. The no-torch-npu case used TORCH_DEVICE_BACKEND_AUTOLOAD=0 and sys.modules["torch_npu"] = None; nothing was uninstalled from the shared container. These are simulations on the current runtime, not tests on separately installed older torch-npu releases.

Normal regression command:

ASCEND_RT_VISIBLE_DEVICES=4 OMP_NUM_THREADS=4 PYTHONPATH="$PWD" \
python -m pytest -q tests/test_ascend_rope.py tests/test_ascend_backend.py \
  tests/test_backends.py tests/test_convrot_w4a4.py tests/test_int8.py \
  tests/test_int8_input_act.py tests/test_constraints.py

Additional validation evidence for merged #198

We attempted to post the report below as a follow-up comment on #198, but GitHub rejected the comment because that discussion is locked. It is included here to keep the completed workflow and CPU-fallback evidence accessible to reviewers. These performance measurements validate the earlier #198 implementation, not a new speedup introduced by this maintenance PR.

Repeated warm workflow timings, pre-PR baseline, CPU fallback evidence, and W4A4 ablation

Follow-up validation: repeated warm workflows and pre-PR CPU fallback

Thanks for merging! Here are the subsequent single-NPU measurements, including a pre-PR baseline and a W4A4-only ablation. These supplement the original smoke-test table; the old smoke times should not be used for performance comparisons.

Environment and timing protocol

One Ascend 910B3 (physical card 4), batch size 1; PyTorch 2.10.0+cpu, torch_npu 2.10.0.post4, CANN 9.1.0, vLLM-Omni v0.28.0 container.
Tested PR revision: 8cb9fdac282aee8f5bcdcb444a5f4000e17bbd27. Pre-PR baseline: d86e8d71cc3f1aeeaa0fbbe31bf993405a27643d (already includes #151). The merged revision is d16dfcf812d27b8b353b312f7ba7472928f79068; its Ascend implementations and tests are identical to the tested PR revision. These are not newly rerun measurements of the merge commit.
Same ComfyUI revision in both arms: 45b3121ccff27bc6020985738ad999b96b4b9a02.
ASCEND_RT_VISIBLE_DEVICES=4 OMP_NUM_THREADS=4, --cache-none --disable-pinned-memory; fixed prompts/seeds/settings. No other validation jobs ran concurrently on the test card.
Each complete workflow/configuration: one warm-up followed by three measured runs. Models were not switched within each group. PR and baseline groups were run sequentially, not interleaved AB/BA.
Workflow seconds below are server history execution_start → execution_success, including request-time model loading/transfers, text encoding, sampling, decoding and saving, but excluding service startup and client polling. Denoising-loop time uses NPU synchronization around KSAMPLER.sample; it includes host dispatch/callback overhead and is not pure device-kernel time.
With --cache-none, model-load calls still occur each run. This is a warmed runtime/filesystem benchmark, not an entirely model-resident benchmark.

PR-enabled results: all four workflows

All 16 runs (4 warm-ups + 12 measured runs) completed successfully. Times are seconds.

Workflow	Warm-up	Measured 1	Measured 2	Measured 3	Workflow median	Denoising-loop median
Z-Image Turbo INT8 ConvRot	12.438	10.274	10.093	10.317	10.274	5.652
Z-Image Turbo BF16	13.586	13.287	13.289	13.222	13.287	6.779
Wan2.2 T2V 14B	78.930	75.770	75.309	76.149	75.770	55.360
Krea2 Turbo mixed INT4	336.127	334.990	334.592	335.456	334.990	329.788

Exact main checkpoints and output settings:

Workflow	Diffusion checkpoint(s)	Output / sampling
Z-Image INT8	`z_image_turbo_int8_convrot.safetensors`	1920 × 1088 image; 8 steps; CFG 1; res_multistep/simple; seed 493240645134828
Z-Image BF16	`z_image_turbo_bf16.safetensors`	Same dimensions, sampling and seed as the INT8 workflow
Wan2.2	`wan2.2_t2v_high_noise_14B_fp16.safetensors` + `wan2.2_t2v_low_noise_14B_fp16.safetensors`	640 × 640; 81 frames at 16 FPS (5.0625 s playback); 4 steps (2 high + 2 low); CFG 1; Euler/simple; initial seed 832596732254118
Krea2	`krea2_turbo_int4_mixed.safetensors`	1024 × 1024 image; 8 steps; CFG 1; Euler/simple; seed 20260907

Supporting weights are unchanged from the PR description: Z-Image uses qwen_3_4b.safetensors / ae.safetensors; Wan uses umt5_xxl_fp16.safetensors / wan_2.1_vae.safetensors and matching high/low wan2.2_t2v_lightx2v_4steps_lora_v1.1_*.safetensors at strength 1; Krea uses qwen3vl_4b_bf16.safetensors / qwen_image_vae.safetensors.

The INT8-vs-BF16 rows compare different checkpoint configurations; their difference is not a same-checkpoint PR speedup measurement.

Complete pre-PR baseline: BF16 and Wan

Both workflows already complete on NPU before this PR. No CPU-fallback warning was observed in these baseline runs.

Workflow	Pre-PR measured workflow seconds	Pre-PR median	PR median	Observed latency reduction
Z-Image BF16	14.058, 14.072, 14.110	14.072	13.287	5.58%
Wan2.2	77.836, 78.224, 78.981	78.224	75.770	3.14%

Pre-PR vs PR denoising-loop medians: Z-Image BF16 7.701 → 6.779 s; Wan 57.779 → 55.360 s. These are observed small-sample differences, not claims of statistical significance or universal gains.

INT8 baseline: confirmed CPU fallback, not a complete workflow timing

On the pre-PR revision, real Z-Image INT8 and Krea mixed-precision requests selected eager INT8 linear and reached aten::_int_mm. The runtime explicitly warned that this operator is unsupported on NPU and falls back to CPU. A separate profiler probe also recorded aten::_to_cpu and copies; dispatch checks found no PrivateUse1 kernel for _int_mm, while ordinary mm had one.

Real workflow	First captured integer matmul inputs	Synchronized duration of that single call
Z-Image INT8	`[32,3840] × [3840,11520]`	1.846 s
Krea mixed INT4: an INT8 layer	`[4135,6144] × [6144,1536]`	61.156 s

After allowing that real call to finish, the diagnostic deliberately stopped each request (VALIDATION_PROBE_COMPLETE). These are not complete baseline workflow times, steady-state operator benchmarks, or native model failures. No end-to-end speedup ratio is calculated for these two workflows. Only some integer operations were confirmed to run on CPU—not the entire model. NPU input/output tensor devices alone do not rule out an internal CPU fallback. The PR INT8 path uses native NPU quant matmul.

Krea W4A4-only ablation

To isolate W4A4 without reintroducing the mixed-INT8 CPU fallback, a separate configuration retained the PR's INT8/RoPE/etc. but disabled only Ascend convrot_w4a4_linear registration. This is not the full pre-PR baseline.

Configuration	Measured workflow seconds	Workflow median	Denoising-loop median
PR with W4A4 routed to eager	338.199, 338.397, 338.217	338.217	333.622
Full PR	334.990, 334.592, 335.456	334.990	329.788

Observed workflow latency reduction is only 0.95%. With three runs per arm and non-interleaved groups, this is insufficient to claim a meaningful/general W4A4 end-to-end speedup.

The profiler confirmed that eager W4A4's floating-point matmul already runs on NPU (aclnnMatmul_MatMulCommon_MatMulV2); the PR uses INT32 quant matmul (aclnnQuantMatmulV5_QuantBatchMatmulV3_QuantBatchMatmulV3). Eager W4A4 must not be described as a CPU implementation. The previously withdrawn 134 s / 2.73x result from the numerically different implementation remains inapplicable.

Output checks and scope

Each PR-enabled workflow's three measured outputs were identical after decoding (within that workflow, not INT8 vs BF16).
All three pre-PR BF16 images, all three pre-PR Wan videos (all 81 decoded frames each), and all three W4A4-eager Krea images matched the corresponding full-PR reference output exactly: RGB MAE=0, max absolute difference=0.
The separate integer probe and captured W4A4 sample comparison passed rtol=0, atol=0. This is not a guarantee for every input, intermediate tensor or seed.
Profiler output is used only as call-path evidence, not as a performance benchmark; it emitted a RECORD-stop warning and a Level0 AIC-metrics warning, although the required labels/kernel records were exported.
The existing ComfyUI-INT4-Fast mixed-INT8 routing shim was kept unchanged in both arms and is not part of this PR. These results do not imply that every unmodified third-party ComfyUI node works merely by installing this backend.

In short: this validation supports native NPU execution instead of INT8 CPU fallback, modest observed BF16/Wan latency reductions, and correctness of the tested W4A4 path—not a large W4A4 speedup claim.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/comfy-kitchen/pull/199 **Author:** [@Big2Wheel](https://github.com/Big2Wheel) **Created:** 9/25/2026 **Status:** ✅ Merged **Merged:** 10/2/2026 **Merged by:** [@comfyanonymous](https://github.com/comfyanonymous) **Base:** `main` ← **Head:** `fix/ascend-rope-capability-gates` --- ### 📝 Commits (3) - [`85870f6`](https://github.com/Comfy-Org/comfy-kitchen/commit/85870f68f062a4f15dc98f0a8bc771d9e8d796ae) fix(ascend): gate RoPE tests on operator capabilities - [`28b7f85`](https://github.com/Comfy-Org/comfy-kitchen/commit/28b7f850cbc4edc0e3e0605e026252c5bd3a53c1) fix(ascend): sort backend imports to unblock CI - [`f28f13e`](https://github.com/Comfy-Org/comfy-kitchen/commit/f28f13e5792c46129ea89f63889c51cd994c190b) Fix import order. ### 📊 Changes **2 files changed** (+33 additions, -18 deletions) <details> <summary>View changed files</summary> 📝 `README.md` (+13 -10) 📝 `tests/test_ascend_rope.py` (+20 -8) </details> ### 📄 Description ## Summary Follow-up to merged #198, which consolidated #153, #154, #155 and #161 on top of #151. This PR changes tests, documentation, and backend import ordering to satisfy repository-wide lint; it does not change backend registration rules or inference implementations. Addresses the two remaining review findings: - [RoPE test capability gating](https://github.com/Comfy-Org/comfy-kitchen/pull/198#discussion_r4100561205). - [Conditional support documentation](https://github.com/Comfy-Org/comfy-kitchen/pull/198#discussion_r4100561200). ### Changes - Fix the entry-point import-order lint error inherited from merged main, which blocked Linux and Windows CI before pytest. Full-repository `ruff check .` now passes. - Retain the Ascend-device gate and add separate RoPE / RMS-RoPE capability marks to execution, alias-safety and selection tests. - Mark unsupported-contract parameter cases individually: ordinary rotary cases require RoPE; `bad_scale` requires RMS-RoPE. - Keep operator-schema detection tests active when applicable rather than skipping the whole module because rotary support is missing. - Mark all eight Ascend RoPE/RMS-RoPE README rows as conditional; explain the rotary schema and RMSNorm requirements. - Name both `npu_dynamic_quant` and `npu_quant_matmul` in the INT8 linear description. ## Validation of this follow-up Single Ascend 910B3, physical card 4, same environment as the report below. | Scenario | Result | |---|---| | NPU regression: Ascend backend/RoPE, backend selection, ConvRot W4A4, INT8, input activation, constraints | **277 passed, 172 skipped**, one reference RMSNorm dtype warning | | Simulated missing `npu_rotary_mul` | **2 passed, 82 skipped** | | Simulated older rotary schema without `rotary_mode` | **2 passed, 82 skipped** | | Simulated missing `npu_rms_norm`, rotary still available | **49 passed, 35 skipped** | | Simulated torch-npu absent: RoPE module, backend selection and constraints | **44 passed, 5 skipped** | | Ruff check / format check on changed Python; git diff --check | Passed | The compatibility scenarios ran in separate Python processes. The relevant torch-npu attribute was removed (or the rotary schema replaced with a minimal schema lacking `rotary_mode`) **before importing comfy_kitchen and collecting tests**. The no-torch-npu case used `TORCH_DEVICE_BACKEND_AUTOLOAD=0` and `sys.modules["torch_npu"] = None`; nothing was uninstalled from the shared container. These are simulations on the current runtime, not tests on separately installed older torch-npu releases. Normal regression command: ```bash ASCEND_RT_VISIBLE_DEVICES=4 OMP_NUM_THREADS=4 PYTHONPATH="$PWD" \ python -m pytest -q tests/test_ascend_rope.py tests/test_ascend_backend.py \ tests/test_backends.py tests/test_convrot_w4a4.py tests/test_int8.py \ tests/test_int8_input_act.py tests/test_constraints.py ``` ## Additional validation evidence for merged #198 We attempted to post the report below as a follow-up comment on #198, but GitHub rejected the comment because that discussion is locked. It is included here to keep the completed workflow and CPU-fallback evidence accessible to reviewers. **These performance measurements validate the earlier #198 implementation, not a new speedup introduced by this maintenance PR.** <details> <summary>Repeated warm workflow timings, pre-PR baseline, CPU fallback evidence, and W4A4 ablation</summary> ## Follow-up validation: repeated warm workflows and pre-PR CPU fallback Thanks for merging! Here are the subsequent single-NPU measurements, including a pre-PR baseline and a W4A4-only ablation. These supplement the original smoke-test table; the old smoke times should not be used for performance comparisons. ### Environment and timing protocol - One Ascend 910B3 (physical card 4), batch size 1; PyTorch 2.10.0+cpu, torch_npu 2.10.0.post4, CANN 9.1.0, vLLM-Omni v0.28.0 container. - Tested PR revision: `8cb9fdac282aee8f5bcdcb444a5f4000e17bbd27`. Pre-PR baseline: `d86e8d71cc3f1aeeaa0fbbe31bf993405a27643d` (already includes #151). The merged revision is `d16dfcf812d27b8b353b312f7ba7472928f79068`; its Ascend implementations and tests are identical to the tested PR revision. These are not newly rerun measurements of the merge commit. - Same ComfyUI revision in both arms: `45b3121ccff27bc6020985738ad999b96b4b9a02`. - `ASCEND_RT_VISIBLE_DEVICES=4 OMP_NUM_THREADS=4`, `--cache-none --disable-pinned-memory`; fixed prompts/seeds/settings. No other validation jobs ran concurrently on the test card. - Each complete workflow/configuration: **one warm-up followed by three measured runs**. Models were not switched within each group. PR and baseline groups were run sequentially, not interleaved AB/BA. - **Workflow seconds** below are server history `execution_start` → `execution_success`, including request-time model loading/transfers, text encoding, sampling, decoding and saving, but excluding service startup and client polling. Denoising-loop time uses NPU synchronization around `KSAMPLER.sample`; it includes host dispatch/callback overhead and is not pure device-kernel time. - With `--cache-none`, model-load calls still occur each run. This is a warmed runtime/filesystem benchmark, not an entirely model-resident benchmark. ### PR-enabled results: all four workflows All 16 runs (4 warm-ups + 12 measured runs) completed successfully. Times are seconds. | Workflow | Warm-up | Measured 1 | Measured 2 | Measured 3 | Workflow median | Denoising-loop median | |---|---:|---:|---:|---:|---:|---:| | Z-Image Turbo INT8 ConvRot | 12.438 | 10.274 | 10.093 | 10.317 | **10.274** | 5.652 | | Z-Image Turbo BF16 | 13.586 | 13.287 | 13.289 | 13.222 | **13.287** | 6.779 | | Wan2.2 T2V 14B | 78.930 | 75.770 | 75.309 | 76.149 | **75.770** | 55.360 | | Krea2 Turbo mixed INT4 | 336.127 | 334.990 | 334.592 | 335.456 | **334.990** | 329.788 | Exact main checkpoints and output settings: | Workflow | Diffusion checkpoint(s) | Output / sampling | |---|---|---| | Z-Image INT8 | `z_image_turbo_int8_convrot.safetensors` | 1920 × 1088 image; 8 steps; CFG 1; res_multistep/simple; seed 493240645134828 | | Z-Image BF16 | `z_image_turbo_bf16.safetensors` | Same dimensions, sampling and seed as the INT8 workflow | | Wan2.2 | `wan2.2_t2v_high_noise_14B_fp16.safetensors` + `wan2.2_t2v_low_noise_14B_fp16.safetensors` | 640 × 640; 81 frames at 16 FPS (**5.0625 s playback**); 4 steps (2 high + 2 low); CFG 1; Euler/simple; initial seed 832596732254118 | | Krea2 | `krea2_turbo_int4_mixed.safetensors` | 1024 × 1024 image; 8 steps; CFG 1; Euler/simple; seed 20260907 | Supporting weights are unchanged from the PR description: Z-Image uses `qwen_3_4b.safetensors` / `ae.safetensors`; Wan uses `umt5_xxl_fp16.safetensors` / `wan_2.1_vae.safetensors` and matching high/low `wan2.2_t2v_lightx2v_4steps_lora_v1.1_*.safetensors` at strength 1; Krea uses `qwen3vl_4b_bf16.safetensors` / `qwen_image_vae.safetensors`. The INT8-vs-BF16 rows compare different checkpoint configurations; their difference is not a same-checkpoint PR speedup measurement. ### Complete pre-PR baseline: BF16 and Wan Both workflows already complete on NPU before this PR. No CPU-fallback warning was observed in these baseline runs. | Workflow | Pre-PR measured workflow seconds | Pre-PR median | PR median | Observed latency reduction | |---|---|---:|---:|---:| | Z-Image BF16 | 14.058, 14.072, 14.110 | 14.072 | 13.287 | **5.58%** | | Wan2.2 | 77.836, 78.224, 78.981 | 78.224 | 75.770 | **3.14%** | Pre-PR vs PR denoising-loop medians: Z-Image BF16 **7.701 → 6.779 s**; Wan **57.779 → 55.360 s**. These are observed small-sample differences, not claims of statistical significance or universal gains. ### INT8 baseline: confirmed CPU fallback, not a complete workflow timing On the pre-PR revision, real Z-Image INT8 and Krea mixed-precision requests selected eager INT8 linear and reached `aten::_int_mm`. The runtime explicitly warned that this operator is unsupported on NPU and falls back to CPU. A separate profiler probe also recorded `aten::_to_cpu` and copies; dispatch checks found no PrivateUse1 kernel for `_int_mm`, while ordinary `mm` had one. | Real workflow | First captured integer matmul inputs | Synchronized duration of that single call | |---|---|---:| | Z-Image INT8 | `[32,3840] × [3840,11520]` | 1.846 s | | Krea mixed INT4: an INT8 layer | `[4135,6144] × [6144,1536]` | 61.156 s | After allowing that real call to finish, the diagnostic deliberately stopped each request (`VALIDATION_PROBE_COMPLETE`). **These are not complete baseline workflow times, steady-state operator benchmarks, or native model failures. No end-to-end speedup ratio is calculated for these two workflows.** Only some integer operations were confirmed to run on CPU—not the entire model. NPU input/output tensor devices alone do not rule out an internal CPU fallback. The PR INT8 path uses native NPU quant matmul. ### Krea W4A4-only ablation To isolate W4A4 without reintroducing the mixed-INT8 CPU fallback, a separate configuration retained the PR's INT8/RoPE/etc. but disabled only Ascend `convrot_w4a4_linear` registration. **This is not the full pre-PR baseline.** | Configuration | Measured workflow seconds | Workflow median | Denoising-loop median | |---|---|---:|---:| | PR with W4A4 routed to eager | 338.199, 338.397, 338.217 | 338.217 | 333.622 | | Full PR | 334.990, 334.592, 335.456 | 334.990 | 329.788 | Observed workflow latency reduction is only **0.95%**. With three runs per arm and non-interleaved groups, this is insufficient to claim a meaningful/general W4A4 end-to-end speedup. The profiler confirmed that eager W4A4's floating-point matmul already runs on NPU (`aclnnMatmul_MatMulCommon_MatMulV2`); the PR uses INT32 quant matmul (`aclnnQuantMatmulV5_QuantBatchMatmulV3_QuantBatchMatmulV3`). **Eager W4A4 must not be described as a CPU implementation.** The previously withdrawn 134 s / 2.73x result from the numerically different implementation remains inapplicable. ### Output checks and scope - Each PR-enabled workflow's three measured outputs were identical after decoding (within that workflow, not INT8 vs BF16). - All three pre-PR BF16 images, all three pre-PR Wan videos (all 81 decoded frames each), and all three W4A4-eager Krea images matched the corresponding full-PR reference output exactly: RGB MAE=0, max absolute difference=0. - The separate integer probe and captured W4A4 sample comparison passed `rtol=0, atol=0`. This is not a guarantee for every input, intermediate tensor or seed. - Profiler output is used only as call-path evidence, not as a performance benchmark; it emitted a RECORD-stop warning and a Level0 AIC-metrics warning, although the required labels/kernel records were exported. - The existing `ComfyUI-INT4-Fast` mixed-INT8 routing shim was kept unchanged in both arms and is **not part of this PR**. These results do not imply that every unmodified third-party ComfyUI node works merely by installing this backend. In short: this validation supports native NPU execution instead of INT8 CPU fallback, modest observed BF16/Wan latency reductions, and correctness of the tested W4A4 path—not a large W4A4 speedup claim. </details> --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:21:38 +00:00

closed this issue
added the
pull-request
label

No labels

pull-request

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Comfy-Org/comfy-kitchen#214

No description provided.

Rows
Columns