-
Notifications
You must be signed in to change notification settings - Fork 60
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
cuCIM: Build CUDA 12 packages #572
Conversation
Thanks Bradley! 🙏 Have pushed some changes to address those comments above. Please take another look when you have a moment 🙂 |
This is getting close! If someone can fix the Python tests, that'd be helpful. |
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
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.
One question, otherwise LGTM!
@jakirkham I think this draft can be opened for broader review. Please remove the |
Just noting that even though CI passes, there is still an issue in the CUDA 12.0 case as noted in comment ( #572 (review) ). So this isn't really working yet |
As `branch-23.08` now contains CUDA 12 support, switch back to using that GHA in the cuCIM PR.
This is unset & unused as best we can tell. So drop this `echo` line.
@@ -4,7 +4,6 @@ CUCIM_BUILD_TYPE=${CUCIM_BUILD_TYPE:-release} | |||
|
|||
echo "CC : ${CC}" | |||
echo "CXX : ${CXX}" | |||
echo "CUDA : ${CUDA}" |
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.
Dropping this line as it is unset & unused based on our prior discussion. This resolves issue ( #582 )
Looks like we are now picking up the CUDA 12 CTK packages on CI 🥳
|
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, it looks good to me.
I guess the section we discussed under
if [ ! -f ${cufile_include}/cufile.h ]; then
c_echo_err Y "GDS client library is not available! Downloading the redistributable package to get cufile.h and libraries."
where it downloads x86_64 GDS can be updated in a follow up PR to not download if on aarch64?
Yeah it sounded like from our discussion earlier that @gigony was picking that piece up in a new PR |
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.
A few small questions and suggestions, then we should be good to go.
- cuda-version=11.8 | ||
- nvcc_linux-64=11.8 | ||
- libcufile=1.4.0.31 | ||
- libcufile-dev=1.4.0.31 | ||
- libnvjpeg=11.6.0.55 | ||
- libnvjpeg-dev=11.6.0.55 |
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.
Not an absolute must, but it would be nice to split this into a separate matrix entry for all cuda 11.8 (x86 and arm) since half of these are common.
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.
Ah actually this doesn't work. dfg doesn't (yet) support multiple matches with different subsets of the matrix. Never mind!
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.
Oh interesting! Ok should we file an issue here to follow up?
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.
Filed as issue ( #585 ). Please add more details there
Co-authored-by: Vyas Ramasubramani <vyas.ramasubramani@gmail.com>
cuCIM only uses `cuda-nvrtc` through Python indirectly via CuPy. So this usage of `cuda-nvrtc` shouldn't be needed. So drop it.
This should already be added via `libcufile-dev` in `host`.
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!
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 @jakirkham for handling CUDA 12 build issues!
Looks good to me!
Thanks all! 🙏 |
/merge |
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.
Final changes LGTM!
Filed for follow up in issue ( #586 ) |
Fixes #513
Fixes #582
Start building cuCIM for CUDA 12.