Conversation
Signed-off-by: Pz1116 <zpbzpb123123@gmail.com>
|
👋 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 addresses a potential collision issue in AscendStore where different pipeline-parallel stages could overwrite each other's GVA cache entries due to shared local layer offsets. By incorporating the PP rank into the GVA key structure and updating the scheduler's hit-check logic to account for these unique keys, the system now correctly isolates cache entries across pipeline stages. 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:
[Ops][Feature] Include PP rank in layerwise GVA keys for KV transferSuggested PR Summary:
### What this PR does / why we need it?
This PR updates the layerwise GVA key generation logic in both the pool scheduler and pool worker to include the pipeline parallel (PP) rank (`pp_rank`). This ensures that each PP worker, which stores a dense local layer range, has its own unique GVA allocation key.
Feedback from the review suggests:
- Reducing redundancy in `_make_layerwise_gva_keys_for_hit_check` and `_make_layerwise_gva_key` by dynamically constructing the group prefix.
- Refactoring the unit test to avoid using object attributes directly as loop variables, which is non-standard.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
A new unit test `test_gva_hit_check_includes_all_pp_ranks` was added to `test_pool_scheduler.py` to verify that the generated keys include all PP ranks.| if len(self.kv_cache_group_ids) > 1: | ||
| return [f"{self.model_name}@{group_id}@{block_hash_hex}@{h}" for h in range(head_or_tp_ranks)] | ||
| return [ | ||
| f"{self.model_name}@{group_id}@{block_hash_hex}@{h}@{p}" | ||
| for h in range(head_or_tp_ranks) | ||
| for p in range(self.pp_size) | ||
| ] | ||
| else: | ||
| return [f"{self.model_name}@{block_hash_hex}@{h}" for h in range(head_or_tp_ranks)] | ||
| return [ | ||
| f"{self.model_name}@{block_hash_hex}@{h}@{p}" | ||
| for h in range(head_or_tp_ranks) | ||
| for p in range(self.pp_size) | ||
| ] |
There was a problem hiding this comment.
The conditional blocks for single-group and multi-group key generation are highly redundant. We can simplify this by dynamically constructing a group prefix, which reduces duplication and improves maintainability.
group_prefix = f"@{group_id}" if len(self.kv_cache_group_ids) > 1 else ""
return [
f"{self.model_name}{group_prefix}@{block_hash_hex}@{h}@{p}"
for h in range(head_or_tp_ranks)
for p in range(self.pp_size)
]| if self.num_kv_cache_groups > 1: | ||
| return f"{self.model_name}@{group_id}@{block_hash_hex}@{self.head_or_tp_rank}" | ||
| return f"{self.model_name}@{group_id}@{block_hash_hex}@{self.head_or_tp_rank}@{self.pp_rank}" | ||
| else: | ||
| return f"{self.model_name}@{block_hash_hex}@{self.head_or_tp_rank}" | ||
| return f"{self.model_name}@{block_hash_hex}@{self.head_or_tp_rank}@{self.pp_rank}" |
There was a problem hiding this comment.
The conditional block here can be simplified by dynamically constructing a group prefix, reducing duplication and improving readability.
group_prefix = f"@{group_id}" if self.num_kv_cache_groups > 1 else ""
return f"{self.model_name}{group_prefix}@{block_hash_hex}@{self.head_or_tp_rank}@{self.pp_rank}"| for worker.head_or_tp_rank in range(scheduler.tp_size): | ||
| for worker.pp_rank in range(scheduler.pp_size): | ||
| self.assertIn(worker._make_layerwise_gva_key(group_id, "deadbeef"), keys) |
There was a problem hiding this comment.
Using object attributes directly as loop variables (for worker.head_or_tp_rank in ...) is highly non-standard and can confuse static analysis tools, linters, and other developers. It is much cleaner and more idiomatic to use standard loop variables and assign them explicitly inside the loop body.
| for worker.head_or_tp_rank in range(scheduler.tp_size): | |
| for worker.pp_rank in range(scheduler.pp_size): | |
| self.assertIn(worker._make_layerwise_gva_key(group_id, "deadbeef"), keys) | |
| for h in range(scheduler.tp_size): | |
| for p in range(scheduler.pp_size): | |
| worker.head_or_tp_rank = h | |
| worker.pp_rank = p | |
| self.assertIn(worker._make_layerwise_gva_key(group_id, "deadbeef"), keys) |
Signed-off-by: Pz1116 <zpbzpb123123@gmail.com>
* releases/v0.23.0: (104 commits) [Doc][BugFix] Update proxy script name in DeepSeek-V3.2 tutorial (vllm-project#13537) [Doc] Fix link errors and update documentation structure (vllm-project#13483) [BugFix][releases/v0.23.0] fix fiaV2 contiguous err in GQA (vllm-project#13458) [v0.23.0][BugFix] Isolate layerwise GVA keys by parallel rank (vllm-project#13513) [Doc][Feature] Add model support of Ascend 950 (vllm-project#13525) [Doc] fix DeepSeek V4 Flash&Pro model tutorial docs link error (vllm-project#13497) [Cherry-pick][releases/v0.23.0][Doc][Misc] Add limitation for reduce sample (from vllm-project#13468) (vllm-project#13469) [BugFix][v0.23.0][KV Pool] Include MTP KV in layerwise AscendStore transfer (vllm-project#13454) [Doc][Misc] Standardize TorchNPU capitalization and update Ascend 950 product terminology (vllm-project#13089) [v0.23.0][Doc] Translated Doc files 2026-08-04 (vllm-project#13437) [Misc][v0.23.0] Fix translation extraction for tables nested in tabs (vllm-project#13413) [Doc] Fix translation and formatting in documentation (vllm-project#13390) [releases/v0.23.0][Doc][Misc] Backport Kimi-K2-Thinking tuning docs to v0.23.0 (vllm-project#13361) [Doc] Deployment key parameter supplement- vllm-project#13297 (vllm-project#13299) [v0.23.0][Doc] Translated Doc files 2026-07-31 (vllm-project#13283) [Doc][Misc] Update max-num-seqs configurations in GLM5 tutorial (vllm-project#13203) [Cherry-pick][releases/v0.23.0][Doc][Misc] Add deployment reference notice for GLM-5 (from vllm-project#12958) (vllm-project#12960) [BugFix][v0.23.0][KV Pool] Guard batch_get_key_info before memcache backend init (vllm-project#13307) [DOC]Modify the scope of scenarios supported by CP (vllm-project#13303) Revert "[cherry-pick][v0.23.0][Performance] remove D2H sync in QLIMetadata builder for DSA_CP" (vllm-project#13289) ...
…roject#13513) ### What this PR does / why we need it? Isolate AscendStore layerwise GVA entries by PCP, DCP, and PP rank, and require scheduler hit checks to find every parallel-rank and TP/head key. With DSA CP enabled, each DCP rank owns a different local KV shard. The previous layerwise path omitted the DCP rank from its key and skipped non-leading ranks according to `put_step`, so DCP-local SFA caches could alias or never be saved. This change keeps the existing `put_step` deduplication when DCP is disabled, while allowing every DCP rank to allocate and save its local cache. The PP rank is also part of the key because pipeline stages address dense local layer ranges from offset zero; sharing one GVA entry could otherwise make layers from different stages overwrite each other, including an MTP layer colliding with a target-model layer at the same local offset. The change covers both single-group and multi-group KV cache layouts. ### Does this PR introduce _any_ user-facing change? No. Existing layerwise GVA cache entries use the old key format and will be treated as cache misses. ### How was this patch tested? - AscendStore pool worker and scheduler unit tests: 137 passed. - All changed-file pre-commit hooks passed. - vLLM version: v0.23.0 - vLLM main: vllm-project/vllm@ee0da84 --------- Signed-off-by: Pz1116 <zpbzpb123123@gmail.com>
…roject#13513) ### What this PR does / why we need it? Isolate AscendStore layerwise GVA entries by PCP, DCP, and PP rank, and require scheduler hit checks to find every parallel-rank and TP/head key. With DSA CP enabled, each DCP rank owns a different local KV shard. The previous layerwise path omitted the DCP rank from its key and skipped non-leading ranks according to `put_step`, so DCP-local SFA caches could alias or never be saved. This change keeps the existing `put_step` deduplication when DCP is disabled, while allowing every DCP rank to allocate and save its local cache. The PP rank is also part of the key because pipeline stages address dense local layer ranges from offset zero; sharing one GVA entry could otherwise make layers from different stages overwrite each other, including an MTP layer colliding with a target-model layer at the same local offset. The change covers both single-group and multi-group KV cache layouts. ### Does this PR introduce _any_ user-facing change? No. Existing layerwise GVA cache entries use the old key format and will be treated as cache misses. ### How was this patch tested? - AscendStore pool worker and scheduler unit tests: 137 passed. - All changed-file pre-commit hooks passed. - vLLM version: v0.23.0 - vLLM main: vllm-project/vllm@ee0da84 --------- Signed-off-by: Pz1116 <zpbzpb123123@gmail.com> Signed-off-by: lijiaqi139 <lijiaqi139@huawei.com>
…roject#13513) ### What this PR does / why we need it? Isolate AscendStore layerwise GVA entries by PCP, DCP, and PP rank, and require scheduler hit checks to find every parallel-rank and TP/head key. With DSA CP enabled, each DCP rank owns a different local KV shard. The previous layerwise path omitted the DCP rank from its key and skipped non-leading ranks according to `put_step`, so DCP-local SFA caches could alias or never be saved. This change keeps the existing `put_step` deduplication when DCP is disabled, while allowing every DCP rank to allocate and save its local cache. The PP rank is also part of the key because pipeline stages address dense local layer ranges from offset zero; sharing one GVA entry could otherwise make layers from different stages overwrite each other, including an MTP layer colliding with a target-model layer at the same local offset. The change covers both single-group and multi-group KV cache layouts. ### Does this PR introduce _any_ user-facing change? No. Existing layerwise GVA cache entries use the old key format and will be treated as cache misses. ### How was this patch tested? - AscendStore pool worker and scheduler unit tests: 137 passed. - All changed-file pre-commit hooks passed. - vLLM version: v0.23.0 - vLLM main: vllm-project/vllm@ee0da84 --------- Signed-off-by: Pz1116 <zpbzpb123123@gmail.com> Signed-off-by: lijiaqi139 <lijiaqi139@huawei.com>
…roject#13513) ### What this PR does / why we need it? Isolate AscendStore layerwise GVA entries by PCP, DCP, and PP rank, and require scheduler hit checks to find every parallel-rank and TP/head key. With DSA CP enabled, each DCP rank owns a different local KV shard. The previous layerwise path omitted the DCP rank from its key and skipped non-leading ranks according to `put_step`, so DCP-local SFA caches could alias or never be saved. This change keeps the existing `put_step` deduplication when DCP is disabled, while allowing every DCP rank to allocate and save its local cache. The PP rank is also part of the key because pipeline stages address dense local layer ranges from offset zero; sharing one GVA entry could otherwise make layers from different stages overwrite each other, including an MTP layer colliding with a target-model layer at the same local offset. The change covers both single-group and multi-group KV cache layouts. ### Does this PR introduce _any_ user-facing change? No. Existing layerwise GVA cache entries use the old key format and will be treated as cache misses. ### How was this patch tested? - AscendStore pool worker and scheduler unit tests: 137 passed. - All changed-file pre-commit hooks passed. - vLLM version: v0.23.0 - vLLM main: vllm-project/vllm@ee0da84 --------- Signed-off-by: Pz1116 <zpbzpb123123@gmail.com> Signed-off-by: lijiaqi139 <lijiaqi139@huawei.com>
…roject#13513) ### What this PR does / why we need it? Isolate AscendStore layerwise GVA entries by PCP, DCP, and PP rank, and require scheduler hit checks to find every parallel-rank and TP/head key. With DSA CP enabled, each DCP rank owns a different local KV shard. The previous layerwise path omitted the DCP rank from its key and skipped non-leading ranks according to `put_step`, so DCP-local SFA caches could alias or never be saved. This change keeps the existing `put_step` deduplication when DCP is disabled, while allowing every DCP rank to allocate and save its local cache. The PP rank is also part of the key because pipeline stages address dense local layer ranges from offset zero; sharing one GVA entry could otherwise make layers from different stages overwrite each other, including an MTP layer colliding with a target-model layer at the same local offset. The change covers both single-group and multi-group KV cache layouts. ### Does this PR introduce _any_ user-facing change? No. Existing layerwise GVA cache entries use the old key format and will be treated as cache misses. ### How was this patch tested? - AscendStore pool worker and scheduler unit tests: 137 passed. - All changed-file pre-commit hooks passed. - vLLM version: v0.23.0 - vLLM main: vllm-project/vllm@ee0da84 --------- Signed-off-by: Pz1116 <zpbzpb123123@gmail.com> Signed-off-by: lijiaqi139 <lijiaqi139@huawei.com> Signed-off-by: jiaqi-lee <15316070896@163.com>
…roject#13513) ### What this PR does / why we need it? Isolate AscendStore layerwise GVA entries by PCP, DCP, and PP rank, and require scheduler hit checks to find every parallel-rank and TP/head key. With DSA CP enabled, each DCP rank owns a different local KV shard. The previous layerwise path omitted the DCP rank from its key and skipped non-leading ranks according to `put_step`, so DCP-local SFA caches could alias or never be saved. This change keeps the existing `put_step` deduplication when DCP is disabled, while allowing every DCP rank to allocate and save its local cache. The PP rank is also part of the key because pipeline stages address dense local layer ranges from offset zero; sharing one GVA entry could otherwise make layers from different stages overwrite each other, including an MTP layer colliding with a target-model layer at the same local offset. The change covers both single-group and multi-group KV cache layouts. ### Does this PR introduce _any_ user-facing change? No. Existing layerwise GVA cache entries use the old key format and will be treated as cache misses. ### How was this patch tested? - AscendStore pool worker and scheduler unit tests: 137 passed. - All changed-file pre-commit hooks passed. - vLLM version: v0.23.0 - vLLM main: vllm-project/vllm@ee0da84 --------- Signed-off-by: Pz1116 <zpbzpb123123@gmail.com> Signed-off-by: lijiaqi139 <lijiaqi139@huawei.com> Signed-off-by: jiaqi-lee <15316070896@163.com>
…roject#13513) ### What this PR does / why we need it? Isolate AscendStore layerwise GVA entries by PCP, DCP, and PP rank, and require scheduler hit checks to find every parallel-rank and TP/head key. With DSA CP enabled, each DCP rank owns a different local KV shard. The previous layerwise path omitted the DCP rank from its key and skipped non-leading ranks according to `put_step`, so DCP-local SFA caches could alias or never be saved. This change keeps the existing `put_step` deduplication when DCP is disabled, while allowing every DCP rank to allocate and save its local cache. The PP rank is also part of the key because pipeline stages address dense local layer ranges from offset zero; sharing one GVA entry could otherwise make layers from different stages overwrite each other, including an MTP layer colliding with a target-model layer at the same local offset. The change covers both single-group and multi-group KV cache layouts. ### Does this PR introduce _any_ user-facing change? No. Existing layerwise GVA cache entries use the old key format and will be treated as cache misses. ### How was this patch tested? - AscendStore pool worker and scheduler unit tests: 137 passed. - All changed-file pre-commit hooks passed. - vLLM version: v0.23.0 - vLLM main: vllm-project/vllm@ee0da84 --------- Signed-off-by: Pz1116 <zpbzpb123123@gmail.com> Signed-off-by: lijiaqi139 <lijiaqi139@huawei.com> Signed-off-by: jiaqi-lee <15316070896@163.com>
What this PR does / why we need it?
Isolate AscendStore layerwise GVA entries by PCP, DCP, and PP rank, and require scheduler hit checks to find every parallel-rank and TP/head key.
With DSA CP enabled, each DCP rank owns a different local KV shard. The previous layerwise path omitted the DCP rank from its key and skipped non-leading ranks according to
put_step, so DCP-local SFA caches could alias or never be saved. This change keeps the existingput_stepdeduplication when DCP is disabled, while allowing every DCP rank to allocate and save its local cache.The PP rank is also part of the key because pipeline stages address dense local layer ranges from offset zero; sharing one GVA entry could otherwise make layers from different stages overwrite each other, including an MTP layer colliding with a target-model layer at the same local offset.
The change covers both single-group and multi-group KV cache layouts.
Does this PR introduce any user-facing change?
No. Existing layerwise GVA cache entries use the old key format and will be treated as cache misses.
How was this patch tested?
AscendStore pool worker and scheduler unit tests: 137 passed.
All changed-file pre-commit hooks passed.
vLLM version: v0.23.0
vLLM main: vllm-project/vllm@ee0da84