Repository navigation
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 introduces high-performance fused AscendC kernels for the mHC (multi-hyper-connection) operations in GLM-5.3-Flash, significantly improving inference performance by reducing eager dispatch overhead. It also adds comprehensive support for the model, including documentation, end-to-end testing, and critical compatibility fixes for KV cache management and upstream vLLM API changes. 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
|
github-actions
Bot
added
documentation
|
👋 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. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for serving the GLM-5.3-Flash model on Ascend NPUs, adding documentation, tutorials, and critical compatibility fixes for MLA-NoPE, Mamba-family layers, and upstream vLLM imports.
Suggested PR Title:
[Attention][Feature] Support GLM-5.3-Flash serving on Ascend NPUsSuggested PR Summary:
### What this PR does / why we need it?
This PR adds comprehensive support for serving the GLM-5.3-Flash model on Ascend NPUs. It introduces tutorials, updates feature guides, and implements critical compatibility and performance enhancements. Key changes include:
- Integrating fused mHC (multi-hyper-connection) operators (`npu_hc_pre_v2` and `npu_hc_post`) to reduce host-side overhead.
- Fixing MLA-NoPE chunked-context paged gather when `qk_rope_head_dim == 0` to avoid zero-width operand issues.
- Correcting `bind_kv_cache` dispatch to ensure Mamba-family layers unpack raw allocations properly.
- Resolving upstream vLLM import and signature compatibility issues, such as `skip_gather` in the logits processor and `storage_block_size` in `MLAAttentionSpec`.
### Does this PR introduce _any_ user-facing change?
Yes, it adds support for serving the GLM-5.3-Flash model on Ascend NPUs.
### How was this patch tested?
Unit tests have been added to verify MLA-NoPE chunked-context paged gather, upstream import compatibility, KV cache spec compatibility, KDA gate, and mHC AscendC operators.Review Feedback:
- Critical Bug: In
vllm_ascend/ops/triton/mamba/causal_conv1d.py, the functioncausal_conv1d_update_npu_DISABLEDmust be renamed tocausal_conv1d_update_nputo match the import inpatch_triton.py, otherwise the optimized NPU Triton implementation will fail to load. - Performance Issues: In
vllm_ascend/ops/triton/kda/kda.py, avoid calling.item()onslot[0](lines 203 and 224) as it triggers host-device synchronization and breaks graph capture in the speculative decoding hot path. Indexinitial_statedirectly withslot[0]instead. - Test Robustness: In
tests/ut/ops/test_mhc_ascendc.py, avoid hardcoding absolute paths to external model configuration files. Define standard configuration constants directly in the test file to make it self-contained.
| return x[:, indices].permute(1, 0, 2) | ||
|
|
||
|
|
||
| def causal_conv1d_update_npu_DISABLED( |
There was a problem hiding this comment.
The function is named causal_conv1d_update_npu_DISABLED in this file, but vllm_ascend/patch/worker/patch_triton.py attempts to import causal_conv1d_update_npu. This mismatch causes the import to fail silently (caught by a try-except block) and prevents the optimized NPU Triton causal conv1d update from being used, which is a critical performance issue. Please rename the function to causal_conv1d_update_npu to match the import.
| def causal_conv1d_update_npu_DISABLED( | |
| def causal_conv1d_update_npu( |
| committed = initial_state[ | ||
| int(slot[0].item()) | ||
| ].clone() |
There was a problem hiding this comment.
Calling .item() on slot[0] (which is an NPU tensor) triggers a host-device synchronization. This breaks graph capture and degrades performance in the speculative decoding hot path. Since PyTorch supports indexing with 0-D/1-D tensors on the same device, you can index initial_state directly with slot[0] without calling .item().
committed = initial_state[slot[0]].clone()| ) | ||
| o_full[:, pos] = o_step[:, 0] | ||
| # Restore the state committed at the accepted boundary. | ||
| initial_state[int(slot[0].item())] = committed |
There was a problem hiding this comment.
Similarly to the state capture above, calling .item() here triggers a host-device synchronization. Index initial_state directly with slot[0] to keep the operation entirely on the device and graph-safe.
| initial_state[int(slot[0].item())] = committed | |
| initial_state[slot[0]] = committed |
| CONFIG_PATH = "/mnt/public/models/GLM-5.3-Flash-BF16/config.json" | ||
| DEVICE = "npu" | ||
|
|
||
| CFG = json.load(open(CONFIG_PATH))["text_config"] | ||
| HIDDEN = CFG["hidden_size"] # 4096 | ||
| HC_MULT = CFG["hc_mult"] # 4 | ||
| MIX_HC = (2 + HC_MULT) * HC_MULT # 24 | ||
| SINKHORN = CFG["hc_sinkhorn_iters"] # 20 | ||
| RMS_EPS = CFG["rms_norm_eps"] # 1e-5 | ||
| HC_EPS = CFG["hc_eps"] # 1e-6 |
There was a problem hiding this comment.
Hardcoding an absolute path to an external model configuration file (/mnt/public/models/...) makes this unit test environment-dependent and will cause it to fail with FileNotFoundError in CI/CD or on other developers' machines. Since the configuration parameters are standard constants, they should be defined directly in the test file to make the test self-contained and robust.
DEVICE = "npu"\n\nHIDDEN = 4096\nHC_MULT = 4\nMIX_HC = (2 + HC_MULT) * HC_MULT # 24\nSINKHORN = 20\nRMS_EPS = 1e-5\nHC_EPS = 1e-6
Replace the ~340 eager dispatches per layer of mhc_pre_torch/mhc_post_torch
(including 117 Sinkhorn-iteration dispatches) with two fused AscendC kernels:
npu_hc_pre_v2 (mix projection + RMS-normalised gates + 20-round Sinkhorn +
stream contraction in ONE launch) and npu_hc_post. Keeps the upstream
MHCPreOp/MHCFusedPostPreOp return contract and fuses the layer input RMSNorm.
Falls back to the torch path when the operator is unavailable or shapes are
outside the envelope (hc_mult<=4, d in {4096,7168}, bf16 x / fp32 hc params).
This adds the ops module only; the patch_triton binding is a separate commit.
Signed-off-by: Liears <Liears@users.noreply.github.com>
29 checks: hc_pre/hc_post/fused_post_pre vs the torch reference, fallback branches (unsupported shapes, env kill switch), and the skip branch. Signed-off-by: Liears <Liears@users.noreply.github.com>
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
a4dbee0 to
8abab51
Compare
github-actions
Bot
removed
merge-conflicts
documentation
|
Addressed the review findings in 1754639:
|
Inline the GLM-5.3-Flash config constants (verified equal to the shipped config.json) instead of reading /mnt/public/models/... at runtime, and derive the repo root from __file__ instead of hardcoding /vllm-workspace, so the test runs on any checkout and in CI. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Liears <liears@users.noreply.github.com>
Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Liears <liears@users.noreply.github.com>
1 participant
What does this PR do?
Replaces ~340 eager dispatches per layer of
mhc_pre_torch/mhc_post_torch(117 of them from the 20-round Sinkhorn normalisation) with two fused AscendC kernels:npu_hc_pre_v2: mix projection + RMS-normalised gates + 20-round Sinkhorn + stream contraction in ONE launchnpu_hc_post: comb @ residual + post ⊙ x4 launches per layer instead of ~340. py-spy measured
mhc_pre_torchat 43% of worker host time on GLM-5.3-Flash.Keeps the upstream
MHCPreOp/MHCFusedPostPreOpreturn contract, fuses the layer input RMSNorm, and falls back to the torch path when the operator is unavailable or shapes are outside the envelope (hc_mult<=4, d in {4096,7168}, bf16 x / fp32 hc params). Env switchGLM53_HC_ASCENDC(default on).New file only (ops module + unit tests); the patch wiring is a separate PR.
Performance
GLM-5.3-Flash W8A8, 8x Atlas A2, TP8, eager: mHC host share 43% → <4%. Unit tests cover numeric parity vs the torch reference (29 checks, within 0.5 bf16 ulp).
Does this PR introduce any user-facing change?
No API change; opt-out env switch preserved.
🤖 Generated with Claude Code