[Bugfix] Fix header include issue in rope #2397
Conversation
Signed-off-by: ganyi <pleaplusone.gy@gmail.com>
![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
This pull request correctly identifies and removes several direct includes of what appear to be private CANN toolkit headers in csrc/kernels/pos_encoding_kernels.cpp. This is an important fix for improving code stability and ensuring forward compatibility with toolkit updates. However, my review suggests the fix may be incomplete, as one of the problematic headers, kernel_type.h, is still included transitively via another file. Addressing this would make the change more robust.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2397 +/- ##
=======================================
Coverage ? 76.18%
=======================================
Files ? 120
Lines ? 13532
Branches ? 0
=======================================
Hits ? 10310
Misses ? 3222
Partials ? 0
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:
|
|
👋 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. |
### What this PR does / why we need it? vLLM-Ascend's rope implementaion include several header file that are not supposed to be included by outside users. Current implementation may break when canntoolkits update, this PR remove those not compatible file includes to guarantee the safety of upgrading cann toolkits. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Tested by rope unittest - vLLM version: v0.10.0 - vLLM main: vllm-project/vllm@3e6dd40 Signed-off-by: ganyi <pleaplusone.gy@gmail.com>
What this PR does / why we need it?
vLLM-Ascend's rope implementaion include several header file that are not supposed to be included by outside users. Current implementation may break when canntoolkits update, this PR remove those not compatible file includes to guarantee the safety of upgrading cann toolkits.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Tested by rope unittest