[CI]test: restore ACL graph path for Qwen3-235B accuracy validation A - #16863
U1stRsouland wants to merge 1 commit into
Conversation
Reapply the ACL graph replay implementation reverted by vllm-project#16726 to validate the Qwen3-235B A3 three-node PD accuracy regression. Signed-off-by: U1stRsouland <19800362117@163.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. |
U1stRsouland
changed the title
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 the UpdatableGraph framework to standardize how attention graphs are updated and replayed on Ascend NPUs. By replacing manual parameter tracking with a task-based registration system, the changes improve the robustness of graph-based inference and resolve previous regression issues encountered during Qwen3-235B accuracy validation. 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
|
|
/nightly QWEN3_235B_PD
|
![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] Introduce UpdatableGraph for dynamic host-side attention metadata updatesSuggested PR Summary:
### What this PR does / why we need it?
This PR refactors the attention graph replay mechanism by introducing `UpdatableGraph` (inheriting from `NPUGraph`). It replaces the legacy static graph parameter updates for Fused Infer Attention (FIA) and Paged Attention (PA) with a dynamic, host-side update mechanism. This is achieved by registering tasks with parameter providers (`FIAParamProvider`, `FIAV2ParamProvider`, `PAParamProvider`) that resolve metadata at runtime.
Additionally, the PR:
- Updates `weak_ref_tensors` to recursively process containers and only weak-reference NPU tensors.
- Integrates `UpdatableGraph` across model runners, speculators, and speculative decoding proposers (Eagle, DFlash, DSpark).
- Cleans up legacy update paths and refactors unit tests to mock and verify the new `UpdatableGraph` behavior.
Feedback:
An issue was identified in `vllm_ascend/compilation/updatable_graph.py` where `factory()` is called redundantly when initializing a new resource with `use_max_workspace=True`, leading to unnecessary memory allocations. This can be optimized using an `elif` block.
### Does this PR introduce _any_ user-facing change?
No user-facing API changes are introduced. This is an internal refactoring of the graph capture and replay mechanism for Ascend NPU.
### How was this patch tested?
The changes were tested using updated unit tests in `tests/ut/attention/test_attention_v1.py`, `tests/ut/compilation/test_acl_graph.py`, and speculative decoding tests.| if key not in self.capture_resources: | ||
| self.capture_resources[key] = factory() | ||
| if use_max_workspace: | ||
| # Some models mix attention layer shapes under the same graph size. | ||
| # During capture, keep the largest required workspace for that size. | ||
| candidate_workspace = factory() | ||
| if ( | ||
| candidate_workspace.numel() * candidate_workspace.element_size() | ||
| > self.capture_resources[key].numel() * self.capture_resources[key].element_size() | ||
| ): | ||
| self.capture_resources[key] = candidate_workspace |
There was a problem hiding this comment.
When 'key' is not present in 'self.capture_resources', 'factory()' is called to initialize it. However, if 'use_max_workspace' is True, 'factory()' is immediately called a second time to obtain 'candidate_workspace', which is then compared against the newly created resource. Since both calls use the same inputs, they will return identical workspaces, making the second call redundant and leading to unnecessary memory allocation of a potentially large workspace tensor during graph capture.\n\nUsing an 'elif' block for the 'use_max_workspace' check avoids this redundant call when the key is first initialized.
if key not in self.capture_resources:\n self.capture_resources[key] = factory()\n elif use_max_workspace:\n # Some models mix attention layer shapes under the same graph size.\n # During capture, keep the largest required workspace for that size.\n candidate_workspace = factory()\n if (\n candidate_workspace.numel() * candidate_workspace.element_size()\n > self.capture_resources[key].numel() * self.capture_resources[key].element_size()\n ):\n self.capture_resources[key] = candidate_workspace
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
2 participants
Reapply the ACL graph replay implementation reverted by #16726 to validate the Qwen3-235B A3 three-node PD accuracy regression.
What this PR does / why we need it?
Does this PR introduce any user-facing change?
How was this patch tested?