-
-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[CI] execute all piecewise compilation tests together #24502
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: zjy0516 <[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 refactors compilation tests to use unique library names for custom PyTorch ops, preventing conflicts when tests are run together. The approach of using Path(__file__).stem
is a good way to ensure uniqueness. However, it's not robust to filenames containing hyphens, which would lead to invalid library names. I've suggested a small fix to sanitize the filenames to prevent future test failures.
@ProExpertProg comparing PR #23257 with this approach, which do you prefer? My thinking is: since only one attention type needs the internal counter, applying it everywhere seems a little strange. |
Signed-off-by: zjy0516 <[email protected]>
Signed-off-by: zjy0516 <[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.
I think this is more complicated than necessary, I would just extract silly::attention
into a common file and use it everywhere. #23257 does that roughly
@ZJY0516 just because others don't use the counter doesn't mean it has to be different. You can just think of the counter as always there and then some tests just don't use it. If there existed a |
ok, I will implement it like #23257. |
Signed-off-by: zjy0516 <[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.
Nice work, good cleanup. I also like calling the file silly_attention.py
better than testing_ops.py
in my PR!!
Signed-off-by: zjy0516 <[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.
Just comments
Signed-off-by: zjy0516 <[email protected]>
Signed-off-by: zjy0516 <[email protected]>
Signed-off-by: zjy0516 <[email protected]>
…4502) Signed-off-by: zjy0516 <[email protected]>
…4502) Signed-off-by: zjy0516 <[email protected]>
…4502) Signed-off-by: zjy0516 <[email protected]> Signed-off-by: xuebwang-amd <[email protected]>
Purpose
execute all compilation tests together
Link #24376 for visibility
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.md
andexamples
for a new model.