[BugFix][SFA] Fix padded-index LSE and empty shards for A5 DCP - #16656
Conversation
|
👋 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! |
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 critical fixes for the A5 SFA DCP (Data-Parallel Context Parallel) implementation to resolve issues with padded indices and incorrect softmax normalization. By bounding LSE computations to valid prefixes and ensuring proper initialization of skipped queries, the patch improves the reliability of the attention mechanism. Additionally, it optimizes memory layout for prefill operations and formally enables the SFA DCP capability for A5 hardware. 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
|
There was a problem hiding this comment.
Code Review
Suggested PR Title:
[Attention][Feature] Support SFA DCP with LSE and padded indicesSuggested PR Summary:
### What this PR does / why we need it?
This PR enhances the Sparse Flash Attention (SFA) MLA kernel to support returning Softmax LSE when using padded indices under DCP. It introduces a binary search to find the end of valid sparse indices and ensures proper initialization of outputs and LSE values (setting LSE to -inf) for queries with no selected keys. Additionally, it ensures gathered KV cache tensors are contiguous after splitting and registers the `SFA_DCP_REPLICATED_INDEXER` hardware capability.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
- Added `test_sparse_flash_attention_padded_indices_lse` to test SFA with padded indices and LSE.
- Added `test_sfa_dcp_prefill_passes_contiguous_gathered_cache` to verify contiguous gathered cache.
paddy-admin
left a comment
There was a problem hiding this comment.
the new .contiguous() shouldn't impact perf for A3 since it's overlapping with indexer processing, still some headroom left.
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Signed-off-by: chengruiqi (C) <c00913489@china.huawei.com>
830afad to
60c6c32
Compare
Signed-off-by: chengruiqi (C) <c00913489@china.huawei.com>
Signed-off-by: chengruiqi (C) <c00913489@china.huawei.com>
ZT-AIA
left a comment
There was a problem hiding this comment.
Make this modification and subsequently add it to the operator description document.
Merging main kept the old SFA_DCP_REPLICATED_INDEXER name in platform.py while hardware_profile.py already uses SFA_C8_DCP_REPLICATED_INDEXER from vllm-project#16656, which made pre-commit mypy fail. Co-authored-by: yjyang62 <yjyang62@users.noreply.github.com> Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Rebase the PR onto latest main as a linear history so CI's `git rebase $BASE_SHA` no longer replays old commits onto the vllm-project#16832 tree that deleted mrv2_utils.py. Keep default-V2 selection, the Ascend feature blacklist, and the SFA C8 DCP hardware capability check from vllm-project#16656. Co-authored-by: yjyang62 <yjyang62@users.noreply.github.com> Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Rebase the PR onto latest main as a linear history so CI's `git rebase $BASE_SHA` no longer replays old commits onto the Keep default-V2 selection, the Ascend feature blacklist, and the SFA C8 DCP hardware capability check from vllm-project#16656. Co-authored-by: yjyang62 <yjyang62@users.noreply.github.com> Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Rebase the PR onto latest main as a linear history so CI's `git rebase $BASE_SHA` no longer replays old commits onto the Keep default-V2 selection, the Ascend feature blacklist, and the SFA C8 DCP hardware capability check from vllm-project#16656. Co-authored-by: yjyang62 <yjyang62@users.noreply.github.com> Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Rebase the PR onto latest main as a linear history so CI's `git rebase $BASE_SHA` no longer replays old commits onto the Keep default-V2 selection, the Ascend feature blacklist, and the SFA C8 DCP hardware capability check from vllm-project#16656. Co-authored-by: yjyang62 <yjyang62@users.noreply.github.com> Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Rebase the PR onto latest main as a linear history so CI's `git rebase $BASE_SHA` no longer replays old commits onto the Keep default-V2 selection, the Ascend feature blacklist, and the SFA C8 DCP hardware capability check from vllm-project#16656. Co-authored-by: yjyang62 <yjyang62@users.noreply.github.com> Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Rebase the PR onto latest main as a linear history so CI's `git rebase $BASE_SHA` no longer replays old commits onto the Keep default-V2 selection, the Ascend feature blacklist, and the SFA C8 DCP hardware capability check from vllm-project#16656. Co-authored-by: yjyang62 <yjyang62@users.noreply.github.com> Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Rebase the PR onto latest main as a linear history so CI's `git rebase $BASE_SHA` no longer replays old commits onto the Keep default-V2 selection, the Ascend feature blacklist, and the SFA C8 DCP hardware capability check from vllm-project#16656. Co-authored-by: yjyang62 <yjyang62@users.noreply.github.com> Signed-off-by: Cursor Agent <cursoragent@cursor.com>
What this PR does / why we need it?
A5 SFA DCP decode can select fewer keys on a rank than its local KV length. The sparse index tensor then contains a valid prefix followed by
-1padding. Using the KV length as the softmax extent includes invalid entries and produces incorrect per-rank normalization for the DCP merge.This patch ports the focused fixes from our standalone SFA operator experiment into the existing custom operator:
n2Sizebefore output initialization.SFA_DCP_REPLICATED_INDEXERon A5 while retaining the platform capability check.The current custom operator already accepts PA_BSND with
return_softmax_lse, so no host-tiling change is needed on this base. No Mooncake changes or bulk operator-source refresh are included.Does this PR introduce any user-facing change?
Enables the A5 SFA DCP capability and fixes its BF16 KV-cache path. The validated target is full GLM5.2 with W4A8 weights and both SFA C8 and indexer C8 disabled, TP8/DCP8/EP, eager execution, no MTP or PD disaggregation. This PR does not claim C8 support.
The full-model experiment also applied #16325 (tested revision
ec6e301d183c5da1023652744b4e9d6395172b86) for indexer metadata. Those changes are not included here; the newer revision of that PR has not been validated as part of this work.How was this patch tested?
4c5ee3320, not the exact operator build used in that experiment. The port has not yet been recompiled or rerun on NPU. Historical results are not evidence of a completed regression for this exact commit.git diff --checkpassed. The repository-widebash format.sh cicould not run because pre-commit is not installed in this local environment. New unit/NPU tests have not been executed in this PR preparation.Performance, graph mode, MTP, C8 and PD combinations remain outside the completed validation scope.