[Bugfix] fix the wrong use of extract_slice and insert_slice#6956
[Bugfix] fix the wrong use of extract_slice and insert_slice#6956wangx700 wants to merge 3 commits intovllm-project:mainfrom
Conversation
…end main branch, extract_slice and insert_slice are located in the triton.language.extra.cann.extension library, rather than in the triton.language library Signed-off-by: wangx700 <wangxin700@huawei.com>
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 resolves an issue where Triton's Highlights
🧠 New Feature in Public Preview: You can now enable Memory 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. Changelog
Activity
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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. 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. |
There was a problem hiding this comment.
Code Review
The pull request correctly identifies and addresses the issue of extract_slice and insert_slice being located in a specific Triton extension library (triton.language.extra.cann.extension) rather than the standard triton.language module in the TritonAscend main branch. The changes introduce conditional imports to use the extension's functions if available, falling back to the standard tl module otherwise. This improves compatibility across different Triton environments.
However, the fallback mechanism in the except ImportError block assumes that tl.extract_slice and tl.insert_slice will always exist in the triton.language module. Given the PR description stating these functions are "rather than in the triton.language library" for the specific branch, this assumption might be incorrect and could lead to AttributeError if the standard tl module does not define these functions. A more robust fallback is suggested to explicitly check for the existence of these functions in tl or raise a more informative error.
| try: | ||
| import triton.language.extra.cann.extension as extension | ||
| extract_slice = extension.extract_slice | ||
| except ImportError: | ||
| extract_slice = tl.extract_slice |
There was a problem hiding this comment.
The current try-except ImportError block assumes that tl.extract_slice will always be available if triton.language.extra.cann.extension cannot be imported. However, the PR description indicates that these functions might not be in the standard triton.language library for the TritonAscend main branch. If tl.extract_slice is indeed missing, this fallback will result in an AttributeError at runtime.
To make this more robust, consider explicitly checking for the existence of tl.extract_slice or handling AttributeError in the fallback, or raising a more specific error if neither source provides the function.
try:
import triton.language.extra.cann.extension as extension
extract_slice = extension.extract_slice
except ImportError:
if hasattr(tl, 'extract_slice'):
extract_slice = tl.extract_slice
else:
raise RuntimeError("Neither triton.language.extra.cann.extension.extract_slice nor tl.extract_slice is available.")| try: | ||
| import triton.language.extra.cann.extension as extension | ||
| insert_slice = extension.insert_slice | ||
| extract_slice = extension.extract_slice | ||
| except ImportError: | ||
| insert_slice = tl.insert_slice | ||
| extract_slice = tl.extract_slice | ||
|
|
There was a problem hiding this comment.
Similar to the issue in swiglu_quant.py, the fallback in the except ImportError block assumes tl.insert_slice and tl.extract_slice are always present. If these functions are not part of the standard triton.language module in some environments, this could lead to an AttributeError.
It's recommended to add a check for the existence of these functions in tl or to catch AttributeError to provide a more graceful failure or a clearer error message.
| try: | |
| import triton.language.extra.cann.extension as extension | |
| insert_slice = extension.insert_slice | |
| extract_slice = extension.extract_slice | |
| except ImportError: | |
| insert_slice = tl.insert_slice | |
| extract_slice = tl.extract_slice | |
| try: | |
| import triton.language.extra.cann.extension as extension | |
| insert_slice = extension.insert_slice | |
| extract_slice = extension.extract_slice | |
| except ImportError: | |
| if hasattr(tl, 'insert_slice') and hasattr(tl, 'extract_slice'): | |
| insert_slice = tl.insert_slice | |
| extract_slice = tl.extract_slice | |
| else: | |
| raise RuntimeError("Neither triton.language.extra.cann.extension nor tl provides insert_slice/extract_slice.") |
| try: | ||
| import triton.language.extra.cann.extension as extension | ||
| insert_slice = extension.insert_slice | ||
| extract_slice = extension.extract_slice | ||
| except ImportError: | ||
| insert_slice = tl.insert_slice | ||
| extract_slice = tl.extract_slice | ||
|
|
There was a problem hiding this comment.
The conditional import for insert_slice and extract_slice has the same potential issue as identified in the other files. If triton.language.extra.cann.extension fails to import, the fallback to tl.insert_slice and tl.extract_slice might fail with an AttributeError if these functions are not defined in the standard triton.language module for the current environment.
Please implement a more robust fallback mechanism to ensure these functions are available or to provide a clear error if they are not.
try:
import triton.language.extra.cann.extension as extension
insert_slice = extension.insert_slice
extract_slice = extension.extract_slice
except ImportError:
if hasattr(tl, 'insert_slice') and hasattr(tl, 'extract_slice'):
insert_slice = tl.insert_slice
extract_slice = tl.extract_slice
else:
raise RuntimeError("Neither triton.language.extra.cann.extension nor tl provides insert_slice/extract_slice.")Signed-off-by: wangx700 <wangxin700@huawei.com>
Signed-off-by: wangx700 <wangxin700@huawei.com>
There was a problem hiding this comment.
#6937 has already been merged to avoid these cases and keep compatibility between triton-ascend 3.2 and newest one. Please check that whether it can help with your case~
|
|
||
| insert_slice = extension.insert_slice | ||
| extract_slice = extension.extract_slice | ||
| except ImportError: |
There was a problem hiding this comment.
you could move the same logic to triton_utils.py
What this PR does / why we need it?
fix the wrong use of extract_slice and insert_slice. In the TritonAscend main branch, extract_slice and insert_slice are located in the triton.language.extra.cann.extension library, rather than in the triton.language library
Does this PR introduce any user-facing change?
How was this patch tested?