Repository navigation
[BugFix] Fix replicated SFA indexer cache metadata under DCP - #16174
Ruiqiu-Zheng wants to merge 5 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
👋 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! |
|
CI trigger note: this PR changes |
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 a critical metadata alignment issue between the independent SFA indexer and the replicated DCP cache layout. By ensuring the indexer consumes the correct replicated cache addresses, it prevents physical layout mismatches that previously led to incorrect address mapping. The changes introduce a composition layer at the SFA boundary to maintain ownership and integrity of the cache metadata during forward passes. 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
|
![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
Suggested PR Title:
[Attention][Feature] Support replicated DCP indexer handoff and compose indexer cache metadataSuggested PR Summary:
### What this PR does / why we need it?
This PR introduces support for replicated Decode Context Parallel (DCP) indexer handoff in the SFA attention backend. It adds the `compose_indexer_cache_metadata` function to use SFA's retained replicated DCP addresses for the independent cache, bypassing local-slot C8 reshape optimizations when DCP is active. This ensures proper metadata composition at the SFA boundary.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Added unit tests in `tests/ut/attention/test_indexer.py` to verify that the metadata builder skips local reshape groups under DCP and that `compose_indexer_cache_metadata` correctly uses the pre-gather replicated DCP view.I have no further feedback as there are no review comments to address.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9f77ccd76
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def _use_c8_reshape_optim(self) -> bool: | ||
| """Whether this indexer can use the LI C8 cache-write operator.""" | ||
| return self.enable_sparse_li_c8 and get_ascend_config().c8_reshape_optim_enabled | ||
| return self.enable_sparse_li_c8 and not self._dcp_active and get_ascend_config().c8_reshape_optim_enabled |
There was a problem hiding this comment.
_dcp_active in direct-construction fixtures
When the existing LI-C8 tests construct AscendSFAIndexerBackend via __new__ (tests/ut/ops/test_mla.py in test_write_cache_scatter_path and test_write_cache_reshape_optim_path), they set enable_sparse_li_c8 but not the newly required _dcp_active. Both tests now raise AttributeError here before reaching their cache-write assertions; either make this lookup tolerate legacy/direct construction or initialize the field in those fixtures so the existing unit suite remains green.
AGENTS.md reference: AGENTS.md:L362-L364
Useful? React with 👍 / 👎.
|
/e2e tests/e2e/pull_request/four_card/context_parallel/test_accuracy.py::test_models_dcp_full_feature_accuracy[dsv3_2_sfa_dcp_replicated_indexer] tests/e2e/pull_request/four_card/context_parallel/test_accuracy_v2.py::test_dsv3_2_sfa_pcp_dcp_model_runner_v2_graph_accuracy |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| slot_mapping_sfa = self._get_sfa_kv_slot_mapping(attn_metadata) | ||
| indexer_attn_metadata = self._get_indexer_attn_metadata() | ||
| if indexer_attn_metadata is not None: | ||
| indexer_attn_metadata = compose_indexer_cache_metadata( |
There was a problem hiding this comment.
indexer_attn_metadata 不应该依赖sfa的metadata，要解耦
There was a problem hiding this comment.
已按建议解耦，更新在 d96bbe229e1dcf5c1dd77b26ed9a346680ad3b03。现在 replicated-DCP 的 block table / slot mapping 由 AscendSFAIndexerMetadataBuilder 在 indexer 侧构造和持有，SFA forward 不再通过 SFA attention metadata 去 compose indexer_attn_metadata。PCP ordered slots、DSA-CP padding 以及 LI-C8 + DCP 兼容行为都保留，并补了 focused UT。当前 pre-commit 正在新 HEAD 上重跑。
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Signed-off-by: Ruiqiu Zheng <191817791+Ruiqiu-Zheng@users.noreply.github.com>
Signed-off-by: Ruiqiu Zheng <191817791+Ruiqiu-Zheng@users.noreply.github.com>
Signed-off-by: Ruiqiu Zheng <191817791+Ruiqiu-Zheng@users.noreply.github.com>
Signed-off-by: Ruiqiu Zheng <191817791+Ruiqiu-Zheng@users.noreply.github.com>
Signed-off-by: Ruiqiu Zheng <191817791+Ruiqiu-Zheng@users.noreply.github.com>
403e555 to
0d4bd0c
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
2 participants
What this PR does
#15669 split the SFA indexer into an independent
AscendSFAIndexerBackendwith its own physical cache and metadata builder. Under replicated SFA DCP, the common attention metadata is restored to the local SFA KV view before the independent indexer metadata is built, while the indexer cache remains physically replicated. That can make the indexer's block table / slot mapping address a different physical layout than its own cache.This patch keeps ownership at the independent indexer boundary and composes the retained SFA DCP cache addresses before the indexer forward:
slot_mappingand replicated block table;pcp_slot_mappingproduced by [Feature][MRV2] sfa support dcp +pcp #15809, without adding another PCP reorder or collective;No key-domain sharding, GlobalTopK, score-publication ABI, or new collective is introduced.
Validation
Current upstream already provides dependency-complete CI surfaces for MRV2 PCP+DCP graph execution (#15809) and for replicated DSA-DCP + LI C8 + MTP3 full-decode graph accuracy. Candidate-specific CI is intentionally left to the PR CI rather than a separate overlay environment.
Current PR-head validation
Current PR head:
24f178d7d06439a19f53c69e728e951a30b1303f.#29228: pre-commit and dependency-complete CPU UT passed. The run-levelci-gateis still blocked because the PR does not yet have the maintainer-onlyready-preciselabel, so its selected NPU matrix was not triggered./e2erun #220 passed on the current PR code for both vLLM refs (v0.28.0andb2f685834a6456197e7033966fdef52a23f1abcd). It covered:These results support candidate-specific upstream CPU and targeted A3 NPU correctness only. Formal PR selected-test/ci-gate closure still requires
ready-precise, and merge still requires the repository's human approval policy.This is a correctness fix; no performance or production-gain claim is made.
Signed-off-by: Ruiqiu Zheng 191817791+Ruiqiu-Zheng@users.noreply.github.com