-
Notifications
You must be signed in to change notification settings - Fork 74
Remove CUDA 11 references #905
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
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test |
|
Also seeing some references in the notebooks cucim/notebooks/Accessing_File_with_GDS.ipynb Lines 30 to 38 in 44c5592
Though have not addressed those |
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
Some notebooks also have outdated |
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, it looks good to me.
I pushed a 1-line commit that seems to have resolved finding the CUDA include path for GDS. Other changes look good to me, but @gigony, please confirm if you have a chance as you are the original author of most of the modified files.
I left a couple of comments for additional minor residual cleanups that could be made.
jameslamb
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.
Looks good to me, this simplification will help with CUDA 13 support (#926).
|
After handling If we can merge this soon, I'll update the CUDA 13 PR and ensure everything is still working together. If more work is needed here, we should merge the CUDA 13 PR first. |
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
|
I was able to push to John's branch, so just pushed the notebook commit here. Should be able to just merge once CI passes |
|
Great. I'll do that now, to help along CUDA 13 work. |
|
/merge |
jakirkham
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.
Just wanted to say thank you all for pushing this across the line ❤️
Had the sense this got 80-90% of the way during 25.08. However wasn't able to finish in that cycle so just split out the doc fixes and moved this to 25.10
There were some remaining things that I hoped to do here. Mainly fixing CI, cleaning up more CUDA 11 references, and some build fixes for issues I caught along the way
Think the changes that all of you made are good enough for this to be in. Will leave some comments here to discuss the remaining work that can be split out. Also to leave some context (especially for Gigon when he looks at this)
| if (TARGET CUDA::nvjpeg_static) | ||
| target_link_libraries(${CUCIM_PLUGIN_NAME} | ||
| PRIVATE | ||
| # Add nvjpeg before cudart so that nvjpeg.h in static library takes precedence. | ||
| CUDA::nvjpeg_static | ||
| # Add CUDA::culibos to link necessary methods for 'deps::nvjpeg_static' | ||
| CUDA::culibos | ||
| CUDA::cudart | ||
| ) | ||
| else() | ||
| target_link_libraries(${CUCIM_PLUGIN_NAME} | ||
| PRIVATE | ||
| CUDA::nvjpeg | ||
| CUDA::cudart | ||
| ) | ||
| endif() |
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.
Right now this checks for nvJPEG static and uses it if it finds it. Otherwise dynamically linking to nvJPEG
This works for the needs of cuCIM. Namely wheels continue to statically link to nvJPEG. Meanwhile Conda packages can dynamically link to nvJPEG
However what would be better is to add an option to build nvJPEG either dynamically or statically (like with cuFile's option). The code here could similarly be updated to check that option (just like cuFile's linking logic). Then wheel and Conda builds could pass that flag based on their needs
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.
Raised this in a new issue: #932
| if (CUCIM_STATIC_GDS) | ||
| # Enabling CUCIM_STATIC_GDS statically links cuFile | ||
| target_link_libraries(cufile_stub | ||
| PUBLIC | ||
| ${CMAKE_DL_LIBS} | ||
| PRIVATE |
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 diff (and the diff behind the fold) looks more exciting than it is
All this does is reproduce the previous logic
Lines 96 to 101 in 54cc342
| target_link_libraries(cufile_stub | |
| PUBLIC | |
| ${CMAKE_DL_LIBS} | |
| PRIVATE | |
| deps::gds_static | |
| ) |
It just replaces deps::gds_static with CUDA::cuFile_static. The former was a stand-in for what CMake now provides. So we just switch to the latter
| CUDA::cuFile_static | ||
| ) |
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.
Here is where deps::gds_static was replaced with CUDA::cuFile_static . For some reason GitHub put a large number of deleted lines between the two. Unclear why it did that. In any event, this is connected with the above content
| if (CUCIM_STATIC_GDS) | ||
| # Enabling CUCIM_STATIC_GDS statically links cuFile | ||
| target_link_libraries(cufile_stub | ||
| PUBLIC | ||
| ${CMAKE_DL_LIBS} |
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.
As another note, it appears we define a bunch of stuff unconditionally that needs dlopen, etc. and thus need to link to ${CMAKE_DL_LIBS}. However we don't actually need to use it in the static case. So it seems this linkage could be dropped (after adding some of the C++ code in guards)
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.
Raised in issue: #933
| # Set GDS include path (cufile.h) | ||
| if (DEFINED ENV{CONDA_BUILD} AND EXISTS $ENV{PREFIX}/include/cufile.h) | ||
| set(GDS_INCLUDE_PATH "$ENV{PREFIX}/include") | ||
| elseif (DEFINED ENV{CONDA_PREFIX} AND EXISTS $ENV{CONDA_PREFIX}/include/cufile.h) | ||
| set(GDS_INCLUDE_PATH "$ENV{CONDA_PREFIX}/include") | ||
| elseif (EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/../temp/cuda/include/cufile.h) | ||
| set(GDS_INCLUDE_PATH ${CMAKE_CURRENT_SOURCE_DIR}/../temp/cuda/include) | ||
| else () | ||
| set(GDS_INCLUDE_PATH /usr/local/cuda/include) | ||
| endif () | ||
|
|
||
| message("Set GDS_INCLUDE_PATH to '${GDS_INCLUDE_PATH}'.") |
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 was just to find the path to the cuFile download we did on CI
Using CMake's CUDA::cuFile* targets to pick up the cuFile in the CUDA Toolkit obviates this need
| # The CTK libraries below are missing from the conda-forge::cudatoolkit package | ||
| # for CUDA 11. The "*_host_*" version specifiers correspond to `11.8` packages | ||
| # and the "*_run_*" version specifiers correspond to `11.x` packages. | ||
|
|
||
| cuda11_libcufile_host_version: | ||
| - "1.4.0.31" | ||
|
|
||
| cuda11_libcufile_run_version: | ||
| - ">=1.0.0.82,<=1.4.0.31" | ||
|
|
||
| cuda11_libnvjpeg_host_version: | ||
| - "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.
These are CUDA 11 pinnings for dependencies outside the Conda cudatoolkit package
None of that is relevant for CUDA 12+, which has the full CTK as packages with associated pinning
Also the usage of these values was dropped already with PR: #889
Hence these can just be dropped
| - cuda-version ={{ cuda_version }} | ||
| - cuda-cudart-dev | ||
| - libcufile-dev # [linux64] | ||
| - libcufile-dev |
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.
cuFile is available on all platforms. Hence it is added as a dependency here
| - libnvjpeg-dev | ||
| - libnvjpeg-static |
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.
nvJPEG is now dynamically linked to in the Conda case. Hence no need for the static library
| - {{ pin_compatible('cuda-version', max_pin='x', min_pin='x') }} | ||
| - cuda-cudart | ||
| - libcufile # [linux64] | ||
| - libcufile |
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 adds the cuFile dependency on ARM
However the CTK only added cuFile for ARM in CUDA 12.2: conda-forge/libcufile-feedstock#9
So this won't work for ARM with CUDA 12.0 or 12.1. Meaning we need to use a similar workaround as KvikIO did: rapidsai/kvikio#754
Another option would be to just start depending on KvikIO
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.
Addressing in PR: #930
| superbuild_depend(nvjpeg) | ||
| superbuild_depend(libculibos) |
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.
These handled finding the nvJPEG and cuLIBOS dependencies via custom CMake logic
That is no longer needed now that they come from the CTK
Hence this and the associated files are dropped
Following up on this thread: #905 (comment) The indentation here was off. Think the editor may have used tabs. This fixes that spacing issue Authors: - https://github.com/jakirkham Approvers: - Kyle Edwards (https://github.com/KyleFromNVIDIA) URL: #931
Following up on this thread: #905 (comment) The CUDA Toolkit (CTK) added cuFile for ARM in CTK 12.2. As a result always requiring `libcufile` with `libcucim` can make it uninstallable on ARM with CTK 12.0 & 12.1. To fix this, build two variants of the `libcucim` package. One with `libcufile` and one without. Constrain `cuda-version` accordingly so that `libcufile` is installed whenever possible. Only when `libcufile` cannot be installed, fallback to the `libcucim` package where the `libcufile` dependency is left off. The `x86_64` CTK has had cuFile since the CTK 11.4. As RAPIDS now requires CUDA 12.0+, there is no issue always requiring `libcufile` on `x86_64`. This issue will not be present in CUDA 13+. So always require `libcufile` in that case. Authors: - https://github.com/jakirkham - James Lamb (https://github.com/jameslamb) - Gregory Lee (https://github.com/grlee77) Approvers: - James Lamb (https://github.com/jameslamb) - Bradley Dice (https://github.com/bdice) URL: #930
Fixes #904
Removes remaining CUDA 11 references.