[amy-why-3459]

Contributor intent (required human answers)

Source: the contributor asked to check whether vLLM-Omni can run Drama 3 on NPU, adapt it, and open a pull request.

1. What outcome do you want to achieve?

Run Fish Audio Drama 3 (fishaudio/drama-3) on Ascend NPU with vLLM-Omni, and submit that adaptation as a pull request.

2. What is the highest-leverage variable for that outcome?

NPU execution of Drama 3. The default deploy uses Model Runner V2, CUDA graphs, Triton sampling, FlashInfer GDN, and block-FP8 experts, and the NPU deploy loader rejected Model Runner V2.

3. How will this work move that variable?

Allow Model Runner V2 on the NPU AR worker, keep the Ascend GDN implementation, run the tower and sampler eagerly off CUDA, and load block-FP8 experts through the Ascend MoE method with a CPU expansion that avoids the failing in-place NPU copy.

4. Whose customer or user needs does this address, and what are those needs?

Operators who want to serve Fish Audio Drama 3 on Ascend NPUs. They need the existing speech API to start on NPU instead of being rejected at deploy-config load.

Purpose

Before: fish_omni.yaml selects Model Runner V2 for stage 0, and loading that deploy on NPU raised Model Runner V2 is not supported. The CUDA GDN/FlashInfer core and Triton tower kernels are not the Ascend path. CUDA's block-FP8 MoE method selects no Ascend kernel.

After: the NPU AR worker builds the Omni V2 runner (with torch.cuda aliased to torch.npu). Stage 0 stays on V2 and eager; the DAC stage stays on V1 on the next card. GDN is left on the Ascend patch. Tower norms and frame sampling use the eager path when the device is not CUDA. Routed experts use AscendFp8Config. On 910B those experts are expanded to bfloat16 on the CPU, because the in-place device copy raises aclnnInplaceCopy 561103, and the FP8 source is released before the dense copy is allocated.

User-facing notes are in recipes/fishaudio/Drama-3.md and the online TTS guide. The supported-models table is unchanged: this is not a finished serving configuration.

Test Plan

vLLM Version: 0.31.0 (container quay.io/atlas-ci/vllm-ascend:v0.31.0)

vLLM-Omni Commit: a1c9b9e11 on amy/drama3-npu (base 03c334818)

• CPU/unit: pytest of the new eager-kernel tests, bsgemm-moe CUDA and NPU routing tests, and the NPU deploy-config tests. 18 passed. The interpreter then aborted with corrupted size vs. prev_size (exit 134) during process teardown; the same abort happens after a successful in-process import in this CANN environment.
• NPU smoke, 2x Ascend 910B1 64GB, ASCEND_RT_VISIBLE_DEVICES=6,7, checkpoint /dataset/models/drama-3:
  • Stage 0 logs Using MR v2 OmniARModelRunner on NPU.
  • Weight load of stage 0 completed (51.8 s).
  • Expert post-process then failed: NPU out of memory with 58.53 GiB already allocated while transposing the expanded bfloat16 expert weight (Tried to allocate 1.00 GiB).
• Not run: a completed speech request, CUDA CI, or a quality/RTF comparison. No throughput claim.

Test Result

Unit tests: 18 passed, then the known CANN shutdown abort.

Serving: the runner and weight load start. A 64 GB 910B cannot hold the bfloat16 expert expansion vLLM-Ascend uses for this block-FP8 checkpoint (Atlas 950 MXFP8 is not selected on 910B). Tensor parallel remains unsupported in the model. The recipe records that limit.

Made with Cursor

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	⚠️ Failed 2026-10-19T10:23:45.134948Z	a1c9b9e	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

[vllm-omni-review-bot]

This PR appears to belong to: docs/design/module/ar_runtime.md, docs/design/module/model_integration.md.

Module owners: @gcanlin @fake0fan @Gaohan123

Routing: @gcanlin via module of the changed files, CODEOWNERS; @fake0fan via module of the changed files; @Gaohan123 via module of the changed files

@amy-why-3459, 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]

Omni ReviewBot: superseded

The CI failure noted on a1c9b9e1190e refers to an earlier head; the pull request now points at 39349e9cf9d1.

[vllm-omni-review-bot]

Omni ReviewBot triage note

Automated triage of commit 2be24ec85b1c produced:

• Priority: high. Prompt maintainer attention is suggested.

These are automated triage suggestions only — the final decision belongs to the maintainers.

[Dong1017]

Independent check of 9676bae8336a2127a668778e3dcdcd358e7fe7f0 against da572cddb38383deae9caef33b8d490e292ef071:

The frozen-head tests/model_executor/models/fish_omni/test_fish_omni_kernels_eager.py passed 11 tests in 0.20s, with CUDA_VISIBLE_DEVICES='' and Torch 2.13.0+cu130. This exercises the PR's CPU eager implementations for Gemma RMSNorm/residual, tower embedding/norm, sigmoid gating, top-k/top-p sampling (including ID mapping and shared top-k), and QK RMSNorm/RoPE/gate splitting against their reference contracts.

The 21-file source review found no code-level blocker: the AR worker selects the Ascend-aware V2 runner, the Fish Omni state supplies Ascend attention metadata while retaining the model-owned request lifecycle, and the DAC generation worker explicitly rejects V2.

The CPU result does not validate NPU dispatch, block-FP8 expert expansion, TP2+EP, or a completed speech request. No NPU card was free for independent device validation. The PR description's load/OOM results identify the older a1c9b9e11 revision, so they should remain scoped there. Please attach a TP2+EP load and at least one completed speech request for this head to demonstrate the current integration.

[amy-why-3459]

NPU Seed-TTS result (2x Ascend 910B1)

Measured on amy/drama3-npu at 7726e82, container why_vllm_omni_v0.32.0 (quay.io/atlas-ci/vllm-ascend:v0.32.0, A2 build). Stage 0 is tensor-parallel 2 plus expert parallel on physical cards 6 and 7; the DAC stage shares the second card. Checkpoint: /dataset/models/drama-3.

Seed-TTS-Eval English (1088 lines). Seed 42 shuffle, first 64 lines, the same 64 at every concurrency. Each line clones its own reference (ref_audio + ref_text) through streaming /v1/audio/speech. Two warmup requests are excluded. Accuracy is Whisper large-v3 word error rate. Speaker similarity was not scored.

Concurrency	Audio s/s	TTFA p50	TTFA p99	RTF mean	WER
1	0.569	1.26 s	1.47 s	1.79	3.33%
16	3.19	4.14 s	26.5 s	5.00	2.52%
32	4.68	5.77 s	7.83 s	5.93	2.05%
64	7.20	14.0 s	14.7 s	9.20	1.88%

64/64 requests completed at every concurrency. Median WER is 0. Most non-zero items are Whisper writing digits or abbreviations differently ("ninety five" / "95", "Saint" / "St."). One concurrency-1 take stopped after "Later, he expelled the Jews of"; the same line is complete at 16, 32, and 64, with only Strassburg heard as Strasbourg.

Two local compatibility edits, not in 7726e82, were required before this v0.32 image would serve:

• Stage 1 follows the deploy model_runner: v1. vLLM 0.32 treats an unset VLLM_USE_V2_MODEL_RUNNER as V2, and the NPU generation worker rejects that.
• NPUOmniARModelRunner.prepare_inputs forwards num_active_loras. The v0.32 Ascend runner requires it.

Raw JSON: /home/why/var/drama3-seedtts-v032-20261020/c{1,16,32,64}.json.

[amy-why-3459]

@FayeSpica PTAL

[natureofnature]

[P2] Keep preempted NPU frame history on the CPU

When a Drama 3 request is preempted after generating frames, the new NPU model state inherits on_request_preempted, whose host-copy branch only handles history.is_cuda. A native NPU tensor takes the clone() branch and stays on NPU; resuming the request then concatenates that history with CPU token/prompt tensors, causing a device mismatch rather than continuing the speech stream. Ascend's MRv2 CUDA API wrapper does not change Tensor.is_cuda. I suggest explicitly copying non-CPU history/count to host with the appropriate synchronization before retaining them. A regression should generate frames on NPU, preempt, reuse the old slot, and resume in another slot, preserving the saved codes and completing the request. The existing resume test uses CPU buffers and misses this path.

[natureofnature]

Reviewed the current diff and the NPU runner, model-state lifecycle, eager kernels, FP8 loading, deployment and shared CUDA paths at fc5f29fec6c3ee34d024c231799303ed90ad1f4b. The earlier TP guard, collection, graph-argument and post-construction guard follow-ups are addressed; one NPU preemption issue is recorded separately.

Agree with @Dong1017's request for current-head device evidence: the published NPU measurements use 7726e82 plus local edits, and the latest follow-up reports config tests only. I suggest validating this head on the documented 2×910B TP2+EP PIECEWISE profile before merging: startup through graph capture, streaming voice-clone requests with overlapping prefill/decode, and preemption/slot-reuse/resumption after generated frames. Requests should finish without device/collective errors and preserve the resumed frame history. The shared runner changes also need the applicable CUDA regression lane to pass. Wheel, pre-commit and CodeQL are green; NPU and CUDA Buildkite are still pending. This was a static review; I did not run repository code or hardware tests.

【Conclusion: Found blocking code issues and validation gaps in commit fc5f29f.】
【Action recommended: Copy preempted NPU frame history to CPU; verify slot reuse/resumption and TP2+EP streaming on this head, and pass the CUDA regression lane before merging.】

[amy-why-3459]

Addressed the NPU preemption finding in b016b3885.

The non-CUDA branch now copies history and count to CPU with non_blocking=False, copy=True. The transfer completes before the slot can be reused, and CPU inputs still get independent snapshots. The existing CUDA pinned-memory/event path is unchanged.

I reproduced the reported CPU/NPU torch.cat device mismatch on the previous head using native NPU history buffers. After the fix, the new NPU regression and the existing model-state suite pass: 20 passed, exit 0. The regression fills device history with known frame codes, preempts, checks both saved tensors are on CPU, clears the old slot, resumes into a different slot, verifies the original codes and frame count, and exercises completion cleanup. It uses the existing lightweight model fixture, not model inference.

Command: python -m pytest -q tests/platforms/npu/test_fish_omni_preemption.py tests/model_executor/models/fish_omni/test_fish_omni_model_state.py.

Environment: why container, image quay.io/ascend/vllm-omni:v0.31.0rc1 (ID 9b5206722497), vLLM 0.31.0, Python 3.12.13, Ascend 910B3. Imports were directed to the modified checkout. All applicable pre-commit hooks passed.

The broader validation request remains open: this is a device-backed lifecycle regression, not TP2+EP PIECEWISE graph capture or streaming voice-clone validation. The available why container uses vLLM 0.31 and the previously documented checkpoint path is unavailable there; full serving validation was not run. The CUDA regression lane also remains to be verified on the new head. These unit-test results do not establish merge readiness for the full integration.

[amy-why-3459]

Looked at b016b388 (CPU host snapshot for non-CUDA preemption) relative to the last reviewed tip.

The production change matches the resume contract: add_request builds host numpy prompt rows from _SuspendedRequest.codes(), and the non-CUDA branch has no CUDA event to order a later slot reuse, so a blocking .to("cpu", copy=True) is the right substitute for the pinned+Event CUDA path. The CPU assertions on _suspended device type and the on_requests_finished cleanup are useful.

One issue in the new NPU regression is inline: the imported make_state fixture is not registered for that module, so the hardware test will not collect as written.

[amy-why-3459]

make_state is a @pytest.fixture defined in tests/model_executor/models/fish_omni/test_fish_omni_model_state.py. Importing it here does not register it for this module (tests/conftest.py only loads tests.helpers.fixtures.*), so pytest will raise fixture 'make_state' not found when this file is collected on NPU CI.

Prefer a local fixture that applies the same OmniModelState monkeypatches and returns the factory, or move the shared factory into tests/helpers / a conftest that both modules load. Loading the other test module via pytest_plugins would also double-collect those CPU tests.

[amy-why-3459]

I checked this by running only python -m pytest -q tests/platforms/npu/test_fish_omni_preemption.py in the why container with a real Ascend 910B3: 1 passed, exit 0, on the checkout used for fe4d073. Pytest discovers decorated fixture objects imported into a test module's namespace; conftest.py plugin registration is not required for that case. The command collected only the NPU test, not the CPU module's tests. The combined regression run also passed (43 tests). I kept the shared fixture instead of duplicating its setup or adding pytest_plugins. This is device-backed lifecycle coverage, not a full serving test.

[amy-why-3459]

Moved the Ascend-specific implementations into the NPU platform in fe4d0731b.

• The Mamba prefill_end compatibility implementation now lives in platforms/npu/mamba_compat.py. StageEngineCoreProc calls the generic prepare_stage_engine() hook before the upstream constructor; the default is a no-op and only the NPU platform installs the compatibility shim. The shared engine no longer imports Ascend.
• QK/RoPE constructor patching and post-construction validation moved to platforms/npu/models/fish_omni.py.
• NPUFishOmniModelState moved to platforms/npu/models/fish_omni_model_state.py.
• Ascend FP8/linear selection and the CPU expert-expansion workaround moved to platforms/npu/quant/fish_omni.py. Shared model code keeps lazy NPU dispatch; generic eager kernels and host history snapshots remain shared.

Validation in the why container (image quay.io/ascend/vllm-omni:v0.31.0rc1, ID 9b5206722497, vLLM 0.31.0): 43 passed across Mamba compatibility/platform dispatch, Fish Omni config/model-state/quantization, and native-NPU preemption tests. The NPU preemption file also passed standalone (1 passed), verifying the imported fixture is discovered. Pre-commit, including mypy and documentation checks, passed.

Added an engine-construction ordering regression, but it cannot collect in this container because vLLM 0.31 lacks vllm.outputs.RequestError. The directly tested platform hook and shim pass; full engine integration, TP2+EP PIECEWISE streaming and CUDA device regression remain unverified here. Updated the model integration and stage runtime design notes to document the platform ownership and hook timing.

[amy-why-3459]

Incremental review (tip fe4d0731 vs last reviewed b016b388)

Covered only the tip commit: Move Drama 3 adapters into the NPU platform (17 files). Earlier tips already reviewed; this is the relocation of Ascend-only helpers out of model_executor/models/fish_omni/.

What looks correct

1. Boundary. Eager QK-RoPE install/require, NPUFishOmniModelState, block-FP8 CPU expert expansion, and the Ascend prefill_end mamba shim now live under vllm_omni/platforms/npu/. CUDA/CPU call sites keep delayed imports gated on device_type == "npu", so importing fish_omni on non-Ascend hosts still avoids vllm_ascend.

2. prepare_stage_engine hook. Moving the mamba compat install from StageEngineCoreProc.__init__ onto OmniPlatform.prepare_stage_engine (NPU override; default no-op) is the right layer. The new unit test asserts platform compat runs before EngineCoreProc construction.

3. Shim behavior preserved. _install_prefill_end_shim still skips methods that already accept prefill_end / **kwargs, and the relocated tests cover old / named / var-keyword signatures plus no-double-wrap.

4. Quant / model_state factories. ExpertFp8Config.get_quant_method and create_fish_omni_model_state still branch on NPU and call into the platform modules; tests were updated to patch the new locations.

Points to watch

1. Exception narrowing. The old stage-engine path caught a broad Exception around the Ascend mamba import; the new mamba_compat path only swallows ImportError. That is usually what we want (surface real install bugs), but if any non-ImportError failure was previously soft-ignored on mixed hosts, it will now abort stage construction. Confirm that is intentional.

2. Process-wide Qwen3NextAttention.__init__ patch is unchanged in semantics (still process-wide, idempotent via _fish_omni_npu_eager). Fine for a Drama 3 stage process; just noting it moved, not redesigned.

3. Docs (model_integration.md / stage_runtime.md) correctly point at the new platform hook — good for the next reader.

No blockers on this tip for the relocation itself. Still outstanding from earlier reviews: a completed NPU speech request / load evidence on a current head (not the older OOM revision in the PR description).

[amy-why-3459]

Following up on the incremental review of fe4d0731: the exception narrowing is intentional. The compatibility installer runs only through the NPU platform hook. Missing imports remain optional across Ascend versions; other import-time initialization failures must propagate rather than silently continuing with a potentially broken allocator. Other platforms use the no-op hook.

2658a1b4f documents this contract and adds regression coverage for both outcomes: ImportError is ignored, while the original RuntimeError is propagated. No runtime behavior was changed in this follow-up.

Validation: python -m pytest -q tests/platforms/npu/test_mamba_compat.py — 4 passed, exit 0, in the why container using the updated checkout (image quay.io/ascend/vllm-omni:v0.31.0rc1, vLLM 0.31.0). All applicable pre-commit hooks, including mypy, passed.

The review's current-head NPU speech-request/load evidence requirement remains open. This focused compatibility test does not validate TP2+EP PIECEWISE serving or replace the CUDA regression lane; no new end-to-end inference claim is made.

[amy-why-3459]

Incremental review (tip 2658a1b4 vs last reviewed fe4d0731)

Covered only the tip commit: Document and test Mamba import failure handling (2 files).

What looks correct

1. Exception narrowing clarified. The docstring now states that only missing imports are soft-ignored across Ascend versions, and that other init failures intentionally propagate on NPU. That answers the earlier note about the old stage-engine except Exception vs the new ImportError-only path.

2. Regression coverage. test_mamba_import_only_ignores_missing_dependencies asserts ImportError is swallowed and RuntimeError (or any non-ImportError from the Ascend patch import) re-raises the same instance. That locks the intended behavior.

No further concerns on this tip. Still outstanding from earlier: a completed NPU speech request / load evidence on a current head.

[amy-why-3459]

Incremental review (tip 7fcc21ba vs last reviewed 2658a1b4)

Covered only the tip commit: Honor stage runner selection in NPU workers (2 files). The intervening 58a0e507 merge-from-main is ignored.

What looks correct

1. Timing. NPUARWorker.init_device builds NPUOmniARModelRunner vs NPUARModelRunner from self.use_v2_model_runner after worker __init__. Overriding the flag in OmniNPUWorkerBase.__init__ (post-super) is therefore early enough for construction, and also aligns any Ascend lifecycle methods that read the same attribute later. That matches the mixed V2 AR / V1 DAC deploy story.

2. Source of truth. Reading model_config.use_v2_model_runner (default False) matches how NPUGenerationWorker already rejects V2 from the stage config, so AR and generation agree on which knob wins over Ascend/vLLM’s global default.

3. Test. test_stage_runner_overrides_upstream_default covers upstream {False,True} × stage {False,True,missing} and asserts the worker flag is True only when the stage sets it True. That locks the regression without needing a real Ascend card.

Points to watch

1. Parent __init__ side effects. If Ascend’s NPUWorker.__init__ already branches on use_v2_model_runner for anything beyond the attribute itself (caches, profiler wiring, etc.), those branches still see the upstream default and are not re-run after the override. Worth a quick check against the vllm-ascend version you deploy with; construction itself is fine because Omni overrides init_device.

2. Still outstanding from earlier: a completed NPU speech request / load evidence on a current head (the PR description’s OOM numbers are still from the older a1c9b9e11 revision).

No blockers on this tip for the stage-runner override.