Repository navigation
[BugFix][Model] Fix the GLM-5.3-Flash NoPE and KDA convolution paths on Ascend - #15885
yiminghub2024 wants to merge 3 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses critical compatibility and performance issues for the GLM-5.3-Flash model on Ascend hardware. It introduces robust handling for NoPE (No RoPE) layers, ensures proper graph capture by eliminating host-syncing operations in the KDA convolution path, and corrects logic errors in speculative decoding history management. These changes significantly improve the reliability and correctness of speculative decoding on models falling back to the torch convolution path. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:
If CI fails, you can run linting and testing checks locally according Contributing and Testing. Tip 💡 Consider Linking a Related Issue or RFCYour PR title contains the [BugFix] tag, indicating a bug fix or new feature. Linking a related issue or RFC in the PR description is strongly encouraged — it gives reviewers helpful context and speeds up the review. You can use any of these keywords:
🙏 Thanks for helping us keep the project well-organized! |
There was a problem hiding this comment.
Code Review
Suggested PR Title:
[Ops][BugFix] Support host-sync-free causal conv1d update and fix NoPE layer gatingSuggested PR Summary:
### What this PR does / why we need it?
This PR refactors the causal convolution 1D operations for GLM-5.3-Flash to ensure they are free of host syncs (such as `.item()` calls) during ACL graph capture. It introduces a batched PyTorch fallback expression when the fused `npu_causal_conv1d_custom` operator is unavailable (e.g., on A5 hardware). Additionally, it fixes gating for MLA NoPE layers to prevent them from entering paths that expect a non-empty RoPE cache, and disables QKNorm-RoPE fusion when `rope_dim <= 0`.
Review feedback suggests replacing `index_copy_` with standard PyTorch tensor assignment in `causal_conv1d.py` to ensure safer and more idiomatic execution on NPU backends.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
New unit tests have been added under `tests/ut/` covering the MLA NoPE cache paths, QKNorm-RoPE fusion pass gating, and GLM-5.3-Flash causal conv1d updates.|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
61f79b2 to
6487bf5
Compare
…on Ascend Linearize onto current main so CI's rebase no longer conflicts in tests/ut/conftest.py. Keep only the npugraph_ex/torchair CPU-UT mocks on top of main's conftest. Signed-off-by: yiminghub2024 <202503791+yiminghub2024@users.noreply.github.com>
6487bf5 to
29c58af
Compare
The linearized push dropped the whole module while __init__.py still imports it, so mypy failed with import-untyped. Keep the intended 19-line KDA bind removal and put the rest of the file back. Signed-off-by: yiminghub2024 <202503791+yiminghub2024@users.noreply.github.com>
The previous E2E run sat on Initialize containers for >70m with no logs available. A new SHA cancels that stuck run via workflow concurrency (cancel-in-progress). Signed-off-by: yiminghub2024 <202503791+yiminghub2024@users.noreply.github.com>
|
Retriggered CI with an empty commit ( The previous E2E run (https://github.com/vllm-project/vllm-ascend/actions/runs/34471051271) assigned runners for the three
|
|
The sibling test in the same file passed. This PR does not touch DeepSeek-V4; the miss is a noisy DSpark acceptance-rate check, not the GLM-5.3 NoPE/KDA change. Remaining selected tests are still running — I will |
|
/rerun Rerun (failed jobs only):
|
|
I reviewed the GLM-5.3 KDA causal-conv ordinary non-spec prefill portion of this PR with official GLM-5.3-Flash layer0 BF16 q/k/v conv weights. This is component/wrapper evidence only; it is not a full-model, E2E, MTP/decode/speculative, production, all-rank/all-layer, or whole-PR readiness claim. Evidence summary:
Coordination note: PR #16251 also touches the ordinary non-spec GLM KDA causal-conv path and calls the same custom op through a different metadata/staging integration surface. The overlap appears partial rather than cleanly duplicate: #15885 carries fallback/native availability behavior, load-time packing, and explicit pack-order/layout validation, while #16251 routes through GDN metadata and non-contiguous state staging/writeback. I am not recommending which whole PR to merge here; this note is limited to the causal-conv ordinary-prefill evidence for #15885. Remaining non-scientific/review items observed outside the numerical result include CRLF line endings in the causal-conv-relevant files at the tested PR head, plus normal PR-wide review/merge-state handling. |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
[BugFix][Model] Fix the GLM-5.3-Flash NoPE and KDA convolution paths on Ascend
What this PR does / why we need it?
Four things GLM-5.3-Flash needs before it runs correctly on Ascend, and one bug
that reaches well past it.
GLM-5.3-Flash applies no rope, so two paths that assume one had to be taught to
step aside: the QKNorm/rope fusion pass, which fused a rope that is not there,
and the MLA rope-cache operators, which a NoPE layer must not call.
Its KDA layers convolve through
causal_conv1d_update. The per-request PyTorchpath read
query_start_locwith.item()once per request, which is rejectedoutright while an ACL graph is being captured, so decode-FULL capture aborted;
it also looped in Python once per KDA layer on every decode step. Route to the
fused
npu_causal_conv1d_customoperator where the hardware registers it, andto a batched torch expression where it does not -- A5 withholds
RUNTIME_CUSTOM_OPS(#7157), so custom ops are off there entirely.The last commit is the one worth reading. Both torch paths read
num_accepted_tokensas how many of this step's tokens to convolve. Itdescribes the step before: which drafts the sampler keeps is only known once
the model has run. A steady decode accepts one token, so seven of every eight
draft-verify rows fell out and reached the recurrent layers as raw projections.
Their logits could not match a draft, acceptance sat at zero past the first
position, and generation came out garbled.
Upstream's kernel uses the count for one thing, the column a request's history
starts at:
and widens the state it writes back to
width - 1 + (seqlen - 1), which is theroom that rewind reads from. Both paths now do the same: convolve every row,
take the count as the read offset, and store the history shifted by one, so
that the window ending at this step's a-th token sits at column a - 1.
The per-request path in
vllm_ascend/ops/causal_conv1d.pyis the fallback forevery model whose hardware withholds the fused operator, Qwen3-Next GDN and
Kimi KDA among them, and it had no rewind at all: it only ever read and wrote
the first
width - 1columns of a row that is allocatednum_specwider.Speculative decoding on those models is affected the same way.
Does this PR introduce any user-facing change?
No new options or configuration. Speculative decoding on models that fall back
to the torch conv1d path now produces correct output and a usable acceptance
rate where it previously produced neither.
How was this patch tested?
Unit tests, 35 across three files:
pytest -sv tests/ut/models/test_glm5next_causal_conv1d.py \ tests/ut/attention/test_mla_nope_cache_paths.py \ tests/ut/compilation/test_qknorm_rope_fusion_pass.pyThe conv1d tests previously checked the batched expression only against the
per-request one, which is what let a mistake shared by both go unseen. They now
also check against the operator's definition written out directly: that every
row of a verify step is convolved, and that reading the stored row at offset
a - 1lands on the window ending with this step's a-th token, for everya.On hardware, GLM-5.3-Flash TP8 on Ascend A5 with DFlash2 and seven draft
tokens, per-position acceptance:
0.000at every position0.583 0.403 0.278 0.181 0.167 0.111 0.069Output went from garbled to coherent on the same prompt.