[BugFix] fix for new verision of cann - #11445
Conversation
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 addresses compatibility issues arising from updates to the CANN toolkit. By introducing a dedicated compatibility header and updating the build system, the changes ensure that logging and module ID definitions remain consistent across different CANN versions, preventing compilation errors. 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
|
|
👋 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. Tip 💡 Consider Linking a Related Issue or RFCYour PR title contains the [BugFix] tag, indicating a bug fix or new feature. Linking a related issue or RFC in the PR description is strongly encouraged — it gives reviewers helpful context and speeds up the review. You can use any of these keywords:
🙏 Thanks for helping us keep the project well-organized! |
There was a problem hiding this comment.
Code Review
This pull request introduces a compatibility header cann_compat.h to resolve compilation issues with newer versions of CANN (specifically B080+), where op_common/log/log.h no longer exposes the unqualified OP module ID. It also cleans up .gitignore entries. The review feedback suggests adhering to the repository style guide for the PR title and summary format, separating the -include flag and its argument in CMake to robustly handle paths containing spaces, and adding a printf format attribute to the variadic DlogRecord function declaration to prevent format string vulnerabilities.
| ) | ||
|
|
||
| # Suppress warnings from catlass/tla third-party headers for CANN kernel compilation | ||
| set(VLLM_ASCEND_CANN_COMPAT_HEADER "${OPS_TRANSFORMER_DIR}/common/include/cann_compat.h") |
There was a problem hiding this comment.
According to the Repository Style Guide, the PR Title and PR Summary should follow a specific format, especially when the PR description is empty.
Here are the suggested PR Title and PR Summary for this pull request:
Suggested PR Title:
[Ops][BugFix] Fix for new version of CANNSuggested PR Summary:
### What this PR does / why we need it?
This PR introduces a compatibility header `cann_compat.h` to fix compilation issues with newer versions of CANN (specifically B080 and above). It addresses the issue where `op_common/log/log.h` stopped exposing the unqualified `OP` module ID and provides fallback definitions for logging functions (`CheckLogLevel` and `DlogRecord`) when `LOG_CPP` is defined.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Tested by compiling the `csrc` module with the new CANN version.References
- The PR Title and PR Summary must follow the specified format when the PR description is empty or needs updating. (link)
| list(APPEND OPS_COMPILE_OPTIONS -include${VLLM_ASCEND_CANN_COMPAT_HEADER}) | ||
| add_compile_options( | ||
| $<$<COMPILE_LANGUAGE:CXX>:-include${VLLM_ASCEND_CANN_COMPAT_HEADER}> | ||
| ) |
There was a problem hiding this comment.
Passing -include${VLLM_ASCEND_CANN_COMPAT_HEADER} as a single concatenated argument can cause compilation failures if the path contains spaces. It is much more robust to pass -include and the file path as separate arguments using CMake's list structure and generator expressions.
list(APPEND OPS_COMPILE_OPTIONS "-include" "${VLLM_ASCEND_CANN_COMPAT_HEADER}")
add_compile_options(
$<$<COMPILE_LANGUAGE:CXX>:-include>
$<$<COMPILE_LANGUAGE:CXX>:${VLLM_ASCEND_CANN_COMPAT_HEADER}>
)
| extern "C" { | ||
| #endif | ||
| int32_t CheckLogLevel(int32_t moduleId, int32_t logLevel); | ||
| void DlogRecord(int32_t moduleId, int32_t level, const char *fmt, ...); |
There was a problem hiding this comment.
To prevent format string vulnerabilities and ensure compile-time type safety of arguments, it is highly recommended to add the printf format attribute to the variadic DlogRecord function declaration.
| void DlogRecord(int32_t moduleId, int32_t level, const char *fmt, ...); | |
| void DlogRecord(int32_t moduleId, int32_t level, const char *fmt, ...) __attribute__((format(printf, 3, 4))); |
|
This PR looks like an AI's work. This is a temporary bug fix, please confirm the correct way to call the CANN interface. |
### What this PR does / why we need it? The changes to log.h in the new version of cann caused compilation errors, so compatibility with the new version of cann was added. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? pip install - vLLM version: v0.23.0 - vLLM main: vllm-project/vllm@b9a7cd4 --------- Signed-off-by: ZT-AIA <1028681969@qq.com>
### What this PR does / why we need it? The changes to log.h in the new version of cann caused compilation errors, so compatibility with the new version of cann was added. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? pip install - vLLM version: v0.23.0 - vLLM main: vllm-project/vllm@b9a7cd4 --------- Signed-off-by: ZT-AIA <1028681969@qq.com>
### What this PR does / why we need it? The changes to log.h in the new version of cann caused compilation errors, so compatibility with the new version of cann was added. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? pip install - vLLM version: v0.23.0 - vLLM main: vllm-project/vllm@b9a7cd4 --------- Signed-off-by: ZT-AIA <1028681969@qq.com>
### What this PR does / why we need it? The changes to log.h in the new version of cann caused compilation errors, so compatibility with the new version of cann was added. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? pip install - vLLM version: v0.23.0 - vLLM main: vllm-project/vllm@b9a7cd4 --------- Signed-off-by: ZT-AIA <1028681969@qq.com>
### What this PR does / why we need it? The changes to log.h in the new version of cann caused compilation errors, so compatibility with the new version of cann was added. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? pip install - vLLM version: v0.23.0 - vLLM main: vllm-project/vllm@b9a7cd4 --------- Signed-off-by: ZT-AIA <1028681969@qq.com>
What this PR does / why we need it?
The changes to log.h in the new version of cann caused compilation errors, so compatibility with the new version of cann was added.
Does this PR introduce any user-facing change?
No
How was this patch tested?
pip install