[CI] main2main-0717 - #12218
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. |
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 synchronizes the vllm-ascend repository with the latest upstream vLLM main branch (commit fe784ff2). It implements necessary compatibility adaptations to ensure the Ascend-specific extensions function correctly across different vLLM versions. Key changes include refactoring KV cache management, updating speculative decoding graph capture logic, and adding version-specific conditional checks to maintain stability. 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. Ignored Files
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:
[Misc][Misc] Support compatibility with vLLM v0.24.0Suggested PR Summary:
### What this PR does / why we need it?
This PR introduces compatibility updates across various modules (schedulers, coordinators, speculators, and operators) to support both vLLM v0.24.0 and other verified main refs. It dynamically adjusts block size calculations, cache hit lookups, and attention forward passes based on the detected vLLM version.
Feedback: In `vllm_ascend/patch/worker/patch_qwen3_5.py`, the direct call to `_all_gather_hidden_and_residual` in `qwen3_5_mtp_forward` bypasses the patched `_ascend_all_gather_hidden_and_residual` wrapper, which would prevent sequence-parallel communication optimization when `flash_comm_v1_enabled` is active.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Existing and updated unit tests in `tests/ut/` and `tests/e2e/`.| if not _IS_VLLM_RELEASE and mtp_layer.use_attn_reduce_scatter_for_moe: | ||
| hidden_states, residual = _all_gather_hidden_and_residual( | ||
| hidden_states, | ||
| residual, | ||
| positions.shape[-1], | ||
| self.config.hidden_size, | ||
| ) |
There was a problem hiding this comment.
In qwen3_5_mtp_forward, the code calls _all_gather_hidden_and_residual directly. However, because _all_gather_hidden_and_residual was imported directly from vllm.model_executor.models.qwen3_next at the top of the file, calling it directly bypasses the patched version _ascend_all_gather_hidden_and_residual. This means that when _EXTRA_CTX.flash_comm_v1_enabled is True, the all-gather will still be executed, bypassing the sequence-parallel communication optimization. To fix this, you should call the patched wrapper _ascend_all_gather_hidden_and_residual instead.
| if not _IS_VLLM_RELEASE and mtp_layer.use_attn_reduce_scatter_for_moe: | |
| hidden_states, residual = _all_gather_hidden_and_residual( | |
| hidden_states, | |
| residual, | |
| positions.shape[-1], | |
| self.config.hidden_size, | |
| ) | |
| if not _IS_VLLM_RELEASE and mtp_layer.use_attn_reduce_scatter_for_moe: | |
| hidden_states, residual = _ascend_all_gather_hidden_and_residual( | |
| hidden_states, | |
| residual, | |
| positions.shape[-1], | |
| self.config.hidden_size, | |
| ) |
zhao-stack
added
ready
fc8f839 to
6e301b6
Compare
45ea195 to
e599634
Compare
| from vllm.v1.worker.gpu.spec_decode.autoregressive import speculator as vllm_speculator_module | ||
|
|
||
| from vllm_ascend.worker.v2.spec_decode.eagle.aclgraph import DecodeEagleAclGraphManager, PrefillEagleAclGraphManager | ||
| from vllm_ascend.worker.v2.spec_decode.eagle.aclgraph import EagleAclGraphManager |
There was a problem hiding this comment.
Did vllm deletes PrefillEagleAclGraphManager?
Now, there is only DecodeEagleAclGraphManager?
There was a problem hiding this comment.
DecodeEagleAclGraphManager and PrefillEagleAclGraphManager are merged into EagleAclGraphManager.
There was a problem hiding this comment.
Yes, vllm delete it, please see vllm-project/vllm#48261 @drslark
There was a problem hiding this comment.
Okay, but I think the name SpeculatorCudaGraphManager should be changed to EagleAclGraphManager.
There was a problem hiding this comment.
My mistake, it is EagleAclGraphManager to be change to SpeculatorCudaAclManager.
Also, this task should not be done in this pr.
I will add it to my todo list.
There was a problem hiding this comment.
LGTM. I think it is good. I will approve it.
Thanks for your adaptation.
| num_tokens_across_dp, | ||
| ) | ||
|
|
||
| CudaGraphManager.capture(self, create_forward_fn, progress_bar_desc=progress_bar_desc) |
There was a problem hiding this comment.
Did vllm change capture to staticmethod?
There was a problem hiding this comment.
No, CudaGraphManager.capture is still an instance method. The explicit call
CudaGraphManager.capture(self, create_forward_fn, ...)
is used to bypass SpeculatorCudaGraphManager.capture for the Ascend decode path. This path needs its own create_forward_fn to preserve the Ascend-specific PIECEWISE skip_attn handling and num_tokens_across_dp preparation.
self is therefore passed explicitly as with a normal unbound instance-method call. For the prefill path, we use super().capture(...) directly to reuse the new unified upstream implementation.
There was a problem hiding this comment.
Okay, i see.
You are right.
I think it is okay.
### What this PR does / why we need it? #### Root cause and provenance The CPU unit-test lane in [run 29653393420](https://github.com/vllm-project/vllm-ascend/actions/runs/29653393420) fails while collecting `tests/ut/_tools/test_generate_zh_docs.py`: ```text tests/ut/_tools/test_generate_zh_docs.py:1: in <module> from tools.generate_zh_docs import apply_translations tools/generate_zh_docs.py:16: in <module> from polib import pofile E ModuleNotFoundError: No module named 'polib' ``` - [PR #11252](#11252), merged as `fbbf5167ada3f3b59909bcf947e69036fc4af425`, introduced the Chinese-doc generator, its top-level `polib` import, and `polib` in `docs/requirements-docs.txt`. - [PR #12331](#12331), merged as `ff83573c37c0bc84e33111e0558086f2c4d73aea`, added `test_generate_zh_docs.py`. That test imports only the pure `apply_translations` helper. - The selected-test CPU environment installs `requirements-dev.txt`, not `docs/requirements-docs.txt`, so it correctly does not contain the docs-only `polib` package. Both the [main CPU job](https://github.com/vllm-project/vllm-ascend/actions/runs/29653393420/job/88103795623) and [v0.24.0 CPU job](https://github.com/vllm-project/vllm-ascend/actions/runs/29653393420/job/88103795682) fail at the same collection point. #### Minimal fix Move `from polib import pofile` into `parse_po_file()`, the only path that actually reads PO files. This keeps `polib` in the existing documentation dependency group, avoids adding a docs-only package to core/runtime or general test dependencies, and allows pure Markdown translation helpers to be imported and tested without the optional docs stack. Calling the real PO parsing path still imports and requires `polib` as intended. This is an existing `main` issue and is independent of PR #12218 and its main2main changes. ### Does this PR introduce _any_ user-facing change? No. It only narrows when an existing documentation dependency is imported. ### How was this patch tested? From current `upstream/main` `e1bd15041af9d9e1a274ae381da0bc0dd8b50e4d`: - Reproduced the collection failure in an isolated environment with `regex` and `pytest`, but no `polib`. - Verified ordinary `tools.generate_zh_docs` import succeeds when `polib` is absent. - `pytest -q --confcutdir=tests/ut/_tools tests/ut/_tools/test_generate_zh_docs.py` without `polib`: `4 passed`. - `pytest --collect-only -q --confcutdir=tests/ut/_tools tests/ut/_tools/test_generate_zh_docs.py` without `polib`: `4 tests collected`. - Parsed a temporary PO file through `parse_po_file()` with `polib` installed and verified the expected `{"Hello": "你好"}` mapping. - `ruff check tools/generate_zh_docs.py tests/ut/_tools/test_generate_zh_docs.py`. - `ruff format --check tools/generate_zh_docs.py tests/ut/_tools/test_generate_zh_docs.py`. - `git diff --check`. - vLLM version: v0.24.0 - vLLM main: vllm-project/vllm@85c09e9 Signed-off-by: zhao-stack <2020265299@qq.com>
cbf132c to
7bc8fcd
Compare
|
|
||
|
|
||
| class DecodeEagleAclGraphManager(DecodeSpeculatorCudaGraphManager): | ||
| class EagleAclGraphManager(SpeculatorCudaGraphManager): |
There was a problem hiding this comment.
I reviewed the logic, merging the two Managers here is right. However, I think naming the class SpeculatorAclGraphManager would be more appropriate. This class will be refactored and promoted to the autoregressive directory later.
wxsIcey
left a comment
wxsIcey
left a comment
There was a problem hiding this comment.
I think the modification to the mrv2 spec decode is correct. For other modules, please confirm with the relevant person in charge.
| from vllm.v1.worker.gpu.spec_decode.autoregressive import speculator as vllm_speculator_module | ||
|
|
||
| from vllm_ascend.worker.v2.spec_decode.eagle.aclgraph import DecodeEagleAclGraphManager, PrefillEagleAclGraphManager | ||
| from vllm_ascend.worker.v2.spec_decode.eagle.aclgraph import EagleAclGraphManager |
There was a problem hiding this comment.
Okay, but I think the name SpeculatorCudaGraphManager should be changed to EagleAclGraphManager.
Signed-off-by: shenzhao <shenzhao9@huawei.com>
Signed-off-by: shenzhao <shenzhao9@huawei.com>
Signed-off-by: shenzhao <shenzhao9@huawei.com>
Align the main lane with vLLM #47006 and #48429 while preserving the v0.24.0 decoder wrapper. Signed-off-by: shenzhao <shenzhao9@huawei.com>
Signed-off-by: zhao-stack <2020265299@qq.com>
Signed-off-by: zhao-stack <2020265299@qq.com>
Signed-off-by: zhao-stack <2020265299@qq.com>
Signed-off-by: shenzhao <shenzhao9@huawei.com>
vLLM #48549 removed LLM's compatibility shim for the deprecated and ignored swap_space keyword. Stop forwarding it from VllmRunner and DPVllmRunner. Signed-off-by: shenzhao <shenzhao9@huawei.com>
Signed-off-by: shenzhao <shenzhao9@huawei.com> (cherry picked from commit 68f7c48)
Signed-off-by: shenzhao <shenzhao9@huawei.com>
Signed-off-by: shenzhao <shenzhao9@huawei.com>
Signed-off-by: shenzhao <shenzhao9@huawei.com>
Signed-off-by: shenzhao <shenzhao9@huawei.com>
Signed-off-by: shenzhao <shenzhao9@huawei.com>
Use the per-KV-group causal state retained after vLLM #48167 removed dflash_causal. Signed-off-by: shenzhao <shenzhao9@huawei.com>
Signed-off-by: zhao-stack <2020265299@qq.com>
Signed-off-by: zhao-stack <2020265299@qq.com>
Signed-off-by: shenzhao <shenzhao9@huawei.com>
8a74268 to
1e32921
Compare
Remove v0.25 tag compatibility from model runner v2 speculative decoding and bind the implementations directly to the verified vLLM main contracts. Signed-off-by: shenzhao <shenzhao9@huawei.com>
Signed-off-by: shenzhao <shenzhao9@huawei.com>
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
| from vllm.v1.worker.gpu.spec_decode.autoregressive import speculator as vllm_speculator_module | ||
|
|
||
| from vllm_ascend.worker.v2.spec_decode.eagle.aclgraph import DecodeEagleAclGraphManager, PrefillEagleAclGraphManager | ||
| from vllm_ascend.worker.v2.spec_decode.eagle.aclgraph import EagleAclGraphManager |
There was a problem hiding this comment.
LGTM. I think it is good. I will approve it.
Thanks for your adaptation.
|
This PR will not be merged. We will use the main2main workflow to upgrade. |
…roject#12352) ### What this PR does / why we need it? #### Root cause and provenance The CPU unit-test lane in [run 29653393420](https://github.com/vllm-project/vllm-ascend/actions/runs/29653393420) fails while collecting `tests/ut/_tools/test_generate_zh_docs.py`: ```text tests/ut/_tools/test_generate_zh_docs.py:1: in <module> from tools.generate_zh_docs import apply_translations tools/generate_zh_docs.py:16: in <module> from polib import pofile E ModuleNotFoundError: No module named 'polib' ``` - [PR vllm-project#11252](vllm-project#11252), merged as `fbbf5167ada3f3b59909bcf947e69036fc4af425`, introduced the Chinese-doc generator, its top-level `polib` import, and `polib` in `docs/requirements-docs.txt`. - [PR vllm-project#12331](vllm-project#12331), merged as `ff83573c37c0bc84e33111e0558086f2c4d73aea`, added `test_generate_zh_docs.py`. That test imports only the pure `apply_translations` helper. - The selected-test CPU environment installs `requirements-dev.txt`, not `docs/requirements-docs.txt`, so it correctly does not contain the docs-only `polib` package. Both the [main CPU job](https://github.com/vllm-project/vllm-ascend/actions/runs/29653393420/job/88103795623) and [v0.24.0 CPU job](https://github.com/vllm-project/vllm-ascend/actions/runs/29653393420/job/88103795682) fail at the same collection point. #### Minimal fix Move `from polib import pofile` into `parse_po_file()`, the only path that actually reads PO files. This keeps `polib` in the existing documentation dependency group, avoids adding a docs-only package to core/runtime or general test dependencies, and allows pure Markdown translation helpers to be imported and tested without the optional docs stack. Calling the real PO parsing path still imports and requires `polib` as intended. This is an existing `main` issue and is independent of PR vllm-project#12218 and its main2main changes. ### Does this PR introduce _any_ user-facing change? No. It only narrows when an existing documentation dependency is imported. ### How was this patch tested? From current `upstream/main` `e1bd15041af9d9e1a274ae381da0bc0dd8b50e4d`: - Reproduced the collection failure in an isolated environment with `regex` and `pytest`, but no `polib`. - Verified ordinary `tools.generate_zh_docs` import succeeds when `polib` is absent. - `pytest -q --confcutdir=tests/ut/_tools tests/ut/_tools/test_generate_zh_docs.py` without `polib`: `4 passed`. - `pytest --collect-only -q --confcutdir=tests/ut/_tools tests/ut/_tools/test_generate_zh_docs.py` without `polib`: `4 tests collected`. - Parsed a temporary PO file through `parse_po_file()` with `polib` installed and verified the expected `{"Hello": "你好"}` mapping. - `ruff check tools/generate_zh_docs.py tests/ut/_tools/test_generate_zh_docs.py`. - `ruff format --check tools/generate_zh_docs.py tests/ut/_tools/test_generate_zh_docs.py`. - `git diff --check`. - vLLM version: v0.24.0 - vLLM main: vllm-project/vllm@85c09e9 Signed-off-by: zhao-stack <2020265299@qq.com>
10 participants
What this PR does / why we need it?
Baselines
maincommit69d1d6fe.fe784ff2, covering85c09e98...fe784ff2.v0.24.0tov0.25.0(702f4814).468fba5a.v0.25 compatibility decisions
vllm.model_executor.layers.fla, while the verified main target importsvllm.third_party.flash_linear_attention.max(). Main-only partial Mamba-align hits use hash-block alignment._group_causal. Model Runner V2 speculative decoding does not claim v0.25 tag compatibility until formal bring-up and debugging.swap_spacecleanup (vLLM #48549)Rebase notes
69d1d6fewas conflict-free, andgit range-diffreported all 32 commits patch-equivalent to the already reviewed intermediate head.Does this PR introduce any user-facing change?
Yes. The supported release lane is now vLLM
v0.25.0; the verified main lane remains pinned tofe784ff2. Model Runner V2 speculative decoding in this PR is intentionally main-only and does not claim release-tag compatibility.How was this patch tested?
Ruff lint and format checks passed for the six files changed by the final MRV2 main-only cleanup.
Focused mypy with the CI Python 3.10 settings passed for those six files; the previous
no-redef,valid-type, andmiscfailures are eliminated.GitHub Actions
lint-and-select-testspassed on head5b2bccf7, including both pre-commit and mypy.Follow-up head
468fba5akeeps MRV2 speculative decoding main-only at the registration boundary: v0.25 skips only the Eagle, DFlash, and DSpark MRV2 speculative cases, while ordinary MRV2 coverage remains enabled. Focused Ruff, codespell, typos, repository checks, and mypy for Python 3.10/3.11/3.12 passed locally.git diff --checkpassed.The main2main mapping
validateandpredictpasses were rerun against the exact verified target, followed by manual review of method, field, call, registration, inheritance, and broad candidates touched by this PR.Source contracts were compared directly at vLLM
v0.25.0andfe784ff2. Non-MRV2 version gates above follow those exact refs;worker/v2/spec_decodeis bound only tofe784ff2.Added/updated regression coverage for grouped hashes, KV-cache per-group hit tuples, GDN lane calls, FLA imports, and speculative graph registration.
Local pytest collection is unavailable in this workspace because
torchandvllmare not installed; head468fba5ahas triggered GitHub CI for executable unit/device coverage.vLLM version: v0.25.0
vLLM main: vllm-project/vllm@85c09e9