- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 10.9k
[Bugfix] Add triton.language.tensor placeholder #25649
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
[Bugfix] Add triton.language.tensor placeholder #25649
Conversation
Signed-off-by: Agata Dobrzyniewicz <adobrzyniewicz@habana.ai>
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 correctly addresses the AttributeError by adding the tensor attribute to the TritonLanguagePlaceholder. The fix is straightforward and aligns with the existing implementation of the placeholder class. However, to ensure this bug does not reappear in the future, it's important to add a corresponding test case. I've left a specific comment regarding updating the tests.
| self.dtype = None | ||
| self.int64 = None | ||
| self.int32 = None | ||
| self.tensor = None | 
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 correctly adds the tensor attribute. To prevent future regressions, please also update the corresponding unit test in tests/test_triton_utils.py.
The test_triton_placeholder_language function is incomplete and should be updated to check for all attributes of TritonLanguagePlaceholder, including the newly added tensor.
Here is a suggested update for the test:
def test_triton_placeholder_language():
    lang = TritonLanguagePlaceholder()
    assert isinstance(lang, types.ModuleType)
    assert lang.__name__ == "triton.language"
    assert lang.constexpr is None
    assert lang.dtype is None
    assert lang.int64 is None
    assert lang.int32 is None
    assert lang.tensor is NoneThere 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.
I added both int32 and tensor attribute
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.
Thanks for the fix!
Signed-off-by: Agata Dobrzyniewicz <adobrzyniewicz@habana.ai>
Signed-off-by: Agata Dobrzyniewicz <adobrzyniewicz@habana.ai> Signed-off-by: yewentao256 <zhyanwentao@126.com>
Signed-off-by: Agata Dobrzyniewicz <adobrzyniewicz@habana.ai> Signed-off-by: xuebwang-amd <xuebwang@amd.com>
Signed-off-by: Agata Dobrzyniewicz <adobrzyniewicz@habana.ai>
Signed-off-by: Agata Dobrzyniewicz <adobrzyniewicz@habana.ai>
Signed-off-by: Agata Dobrzyniewicz <adobrzyniewicz@habana.ai> Signed-off-by: xuebwang-amd <xuebwang@amd.com>
Fix issue:
AttributeError: module 'triton.language' has no attribute 'tensor'As LanguagePlaceholder doesn't have this field that is being used in compute_identity_kernel in vllm/model_executor/layers/fused_moe/fused_moe.py:670
Regression after: #23991