-
-
Notifications
You must be signed in to change notification settings - Fork 17.2k
rocmPackages.aotriton: 0.9.2b -> 0.10b #439056
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
This comment was marked as outdated.
This comment was marked as outdated.
Flakebi
left a comment
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, LGTM!
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-already-reviewed/2617/2530 |
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 like the new expression as it reads a lot better. My build is still running, but some comments for now
3a36d43 to
248906f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
248906f to
1bb7513
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
Oh that's fun. The combination of this diff and 2a2a51e doesn't build, but either works alone. Marking draft since that just got merged, glad we didn't merge both in quick succession. |
1bb7513 to
fc5983d
Compare
fc5983d to
400c14c
Compare
|
GaetanLepage
left a comment
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 overall. Made minor comments.
Diff: ROCm/aotriton@0.9.2b...0.10b Significantly improves the build by reusing python3Packages.triton
400c14c to
4f33cb4
Compare
|
Applied both suggestions |
GaetanLepage
left a comment
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. Would like a second opinion though.
Diff: ROCm/aotriton@0.9.2b...0.10b
This is a rewrite of
aotriton/default.nix. We're properly reusing the existingpython3Packages.tritoninstance as the build-time triton instead of letting aotriton's CMakeLists.txt build its own triton, so lots of hackery can be dropped! and some packages that weren't actually used and were in buildInputs 😅If in future we needed to pin a specific triton source rev here with an override it should be relatively safe as it's a build time only dependency and won't run into collisions like in many cases in python packaging.
For now we have no need to pin triton; aotriton builds happily against triton 3.4.
This is a followup to the recent torch bump #431973. torch 2.8 introduced faster sliding window attention via aotriton but requires a minimum version of 0.10b to enable it.
https://github.com/pytorch/pytorch/blob/379ebdaf5e2fbf387de297f673254d005e583b31/aten/src/ATen/native/transformers/hip/flash_attn/aot/mha_all_aot.hip#L96-L119
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.