[ROCm] Optionally use hipblaslt #120551
[ROCm] Optionally use hipblaslt #120551
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/120551
Note: Links to docs will display an error until the docs builds have been completed. ❌ 3 New Failures, 2 Unrelated FailuresAs of commit 8edc7b9 with merge base bab4b5a ( NEW FAILURES - The following jobs have failed:
FLAKY - The following jobs failed but were likely due to flakiness present on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
janeyx99
added
the
triaged
|
@xw285cornell would appreciate your review of this. I'm assuming this PR will break your internal build? |
The hipblaslt package is not available on Fedora. Instead of requiring the package, make it optional. If it is found, define the preprocessor variable HIPBLASLT Convert the checks for ROCM_VERSION >= 507000 to HIPBLASLT checks Signed-off-by: Tom Rix <[email protected]>
trixirt
force-pushed
the
optional-hipblaslt
branch
from
2c029b1 to
8edc7b9
Compare
|
Update for a couple more hipblaslt usages that were added in main this week. |
|
This PR looks pretty straight forward and using a variable instead of "magic" version numbers seem to be much cleaner. It would be nice if this PR wouldn't linger around much longer. |
jithunnair-amd
left a comment
jithunnair-amd
left a comment
There was a problem hiding this comment.
Suggest USE_HIPBLASLT instead of HIPBLASLT as name for define
|
considering that the basis issue of this pr #119081 is now correctly recognized as a bug i think it would be good to not leave this lingering much longer. |
|
Sorry just see this. Thanks @jeffdaily for pinging, this will break our internal codebase but it should be an easy fix. I'm not objecting the idea, if you can ping me before this PR lands, I can put a fix to our internal system easily. |
jithunnair-amd
changed the title
jithunnair-amd
added
the
rocm priority
|
@trixirt Please resolve conflicts and I'll request an upstream maintainer to approve. |
@trixirt Please do consider this renaming to be aligned with current naming |
|
I am working on this. Its a bit involved and in a parallel track i trying to get hipblastlt to build on Fedora. |
|
I have submitted the hipBLASLt package for Fedora here A prelim refactoring of the above change is here refactored for 2.4 Fedora/My preference is to use the package once it is available. |
|
@trixirt , could you fix git conflict, please? I'd like to see this patch merged |
|
@AngryLoki |
Yes, please. For 2.3.x I will add current patch (with USE_HIPBLASLT cmake option), but in next versions I'd like to see it in upstream. |
|
the problem is that this patch is not just required to support distros that dont want to package hipblaslt but also because hipblaslt has a bug steming from a missunderstanding how the rocm runtime works that subtly breaks (or crashes, depends on if assertions in the runtime are enabled) all systems with gpus that dont support hipblaslt when a binary is run that links against it. |
pruthvistony
added
rocm
|
@trixirt , |
@trixirt , hi again. I tried to update this patch myself and now I understand what do you mean by "the base code changed". I think it is too difficult to do it now, so I packaged pytorch-2.4.0 with required hipblaslt (but it is still possible to build hipblaslt with 0 gpus). Feel free to close this PR if you don't need it anymore. |
Fedora has hipblaslt now with 1 gpu (WEEEE) .. so closing |
11 participants
The hipblaslt package is not available on Fedora.
Instead of requiring the package, make it optional. If it is found, define the preprocessor variable HIPBLASLT Convert the checks for ROCM_VERSION >= 50700 to HIPBLASLT checks
Fixes #119081
cc @jeffdaily @sunway513 @jithunnair-amd @pruthvistony @ROCmSupport @dllehr-amd @jataylo @hongxiayang