-
Notifications
You must be signed in to change notification settings - Fork 459
[Feat] allow using aclgraph in ray backend #2589
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this 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.
👋 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. |
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 |
plz remove the failed ut, as it check if compilation level and cudagraph_mode is set correctly when enabling ray |
Codecov Report✅ All modified and coverable lines are covered by tests. 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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Same here like #2472 , rebase main and signoff. @WithHades |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Signed-off-by: withHades <244036962@qq.com>
### 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>
### 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>
### 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>
### 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>
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?