Skip to content

Conversation

WithHades
Copy link
Contributor

@WithHades WithHades commented Aug 28, 2025

What this PR does / why we need it?

Allow using aclgraph in ray backend, for tp + pp + aclgraph in multi machine

Does this PR introduce any user-facing change?

How was this patch tested?

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request enables the use of ACL Graph with the Ray distributed executor backend by removing the check that previously disabled it. While this is a valuable feature enablement, the corresponding unit test that verifies the old behavior has not been updated, which will cause the test suite to fail. Please address the failing test to ensure the change is properly validated.

WithHades

This comment was marked as duplicate.

Copy link

👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:‌‌

  • A PR should do only one thing, smaller PRs enable faster reviews.
  • Every PR should include unit tests and end-to-end tests ‌to ensure it works and is not broken by other future PRs.
  • Write the commit message by fulfilling the PR description to help reviewer and future developers understand.

If CI fails, you can run linting and testing checks locally according Contributing and Testing.

@Yikun
Copy link
Collaborator

Yikun commented Aug 28, 2025

is it possible to add a e2e test for ray+aclgraph.

@yiz-liu @MengqingCao Please take a look on this, thanks.

@MengqingCao
Copy link
Collaborator

is it possible to add a e2e test for ray+aclgraph.

@yiz-liu @MengqingCao Please take a look on this, thanks.

the test of ray+aclgraph on single node will be added naturally when the refactor of CI is done, plz refer to https://github.com/vllm-project/vllm-ascend/pull/2276/files#diff-19912dc7bc00e91104289260c0d7127e6e320a91cbf36dba23966beb800d4f9eL45

And to cover the previous failed scenario, we'll add a e2e test for ray + aclgraph + multi-node when the multi-node CI is ready

@MengqingCao
Copy link
Collaborator

plz remove the failed ut, as it check if compilation level and cudagraph_mode is set correctly when enabling ray
https://github.com/vllm-project/vllm-ascend/actions/runs/17286060253/job/49063686288?pr=2589

@wangxiyuan wangxiyuan added the ready read for review label Aug 28, 2025
Copy link

codecov bot commented Aug 28, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.73%. Comparing base (24d4dad) to head (a79245e).
⚠️ Report is 21 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2589      +/-   ##
==========================================
- Coverage   73.71%   72.73%   -0.99%     
==========================================
  Files         152      150       -2     
  Lines       21967    21483     -484     
==========================================
- Hits        16194    15625     -569     
- Misses       5773     5858      +85     
Flag Coverage Δ
unittests 72.73% <ø> (-0.99%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@yiz-liu
Copy link
Collaborator

yiz-liu commented Sep 1, 2025

Same here like #2472 , rebase main and signoff. @WithHades

Copy link

github-actions bot commented Sep 2, 2025

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Signed-off-by: withHades <244036962@qq.com>
@wangxiyuan wangxiyuan merged commit 0c0789b into vllm-project:main Sep 4, 2025
25 checks passed
Angazenn pushed a commit to Angazenn/vllm-ascend that referenced this pull request Sep 10, 2025
### What this PR does / why we need it?

Allow using aclgraph in ray backend, for tp + pp + aclgraph in multi
machine

### Does this PR introduce _any_ user-facing change?

### How was this patch tested?

- vLLM version: v0.10.1.1
- vLLM main:
vllm-project/vllm@4ba0c58

Signed-off-by: withHades <244036962@qq.com>
offline893 pushed a commit to offline893/vllm-ascend that referenced this pull request Sep 16, 2025
### What this PR does / why we need it?

Allow using aclgraph in ray backend, for tp + pp + aclgraph in multi
machine

### Does this PR introduce _any_ user-facing change?

### How was this patch tested?

- vLLM version: v0.10.1.1
- vLLM main:
vllm-project/vllm@4ba0c58

Signed-off-by: withHades <244036962@qq.com>
Signed-off-by: offline0806 <z00858301@china.huawei.com>
wangxiaoteng888 pushed a commit to LCAIZJ/vllm-ascend that referenced this pull request Sep 25, 2025
### What this PR does / why we need it?

Allow using aclgraph in ray backend, for tp + pp + aclgraph in multi
machine

### Does this PR introduce _any_ user-facing change?

### How was this patch tested?

- vLLM version: v0.10.1.1
- vLLM main:
vllm-project/vllm@4ba0c58

Signed-off-by: withHades <244036962@qq.com>
chopper0126 pushed a commit to chopper0126/vllm-ascend that referenced this pull request Sep 26, 2025
### What this PR does / why we need it?

Allow using aclgraph in ray backend, for tp + pp + aclgraph in multi
machine

### Does this PR introduce _any_ user-facing change?

### How was this patch tested?

- vLLM version: v0.10.1.1
- vLLM main:
vllm-project/vllm@4ba0c58

Signed-off-by: withHades <244036962@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants