-
-
Notifications
You must be signed in to change notification settings - Fork 10.7k
[MISC] fix import violations for re and triton modules #26654
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
Signed-off-by: Sungjae Lee <[email protected]>
Signed-off-by: Sungjae Lee <[email protected]>
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 aims to fix import violations for re
and triton
modules. The change to use regex
instead of re
is consistent with the project's conventions. However, the change to use a centralized triton
import introduces a critical issue. It will cause a runtime AttributeError
when Triton is not installed because the placeholder object is missing the cdiv
method used in qutlass_utils.py
. I've provided a comment with a suggested fix.
import triton.language as tl | ||
from torch.library import wrap_triton | ||
|
||
from vllm.triton_utils import triton |
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.
This change to use the centralized triton
import utility is a good step for consistency. However, it introduces a runtime error when Triton is not installed. The TritonPlaceholder
object, which is used when Triton is unavailable, does not implement the cdiv
method. This file calls triton.cdiv
on lines 109, 110, 127, and 128, which will lead to an AttributeError
.
To resolve this, please replace all calls to triton.cdiv
with the locally defined ceil_div
function (line 145), which provides the same ceiling division functionality.
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.
maybe this part should be reverted?
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.
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.
maybe this part should be reverted?
I think we should keep the current changes, as this could use TritonPlaceholder
to avoid importing error. However, if the triton kernel is invoked at the runtime, we should keep the error to be raised, as that indicates we're using triton kernel in a scenario triton is not installed
Co-authored-by: Mengqing Cao <[email protected]> Signed-off-by: Sungjae Lee <[email protected]>
Signed-off-by: Sungjae Lee <[email protected]>
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.
LGTM, thanks for the work!
…26654) Signed-off-by: Sungjae Lee <[email protected]> Co-authored-by: Mengqing Cao <[email protected]> Signed-off-by: Alberto Perdomo <[email protected]>
…26654) Signed-off-by: Sungjae Lee <[email protected]> Co-authored-by: Mengqing Cao <[email protected]>
…26654) Signed-off-by: Sungjae Lee <[email protected]> Co-authored-by: Mengqing Cao <[email protected]>
…26654) Signed-off-by: Sungjae Lee <[email protected]> Co-authored-by: Mengqing Cao <[email protected]>
Purpose
I found pre-commit hook errors during the merge process. To fix these violations, I updated the import statements.
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.md
andexamples
for a new model.