Remove CUDA 11 references #905
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,11 @@ | ||
| c_compiler_version: | ||
| - 13 | ||
|
|
||
| cxx_compiler_version: | ||
| - 13 | ||
|
|
||
| cuda_compiler: | ||
| - cuda-nvcc | ||
|
|
||
| c_stdlib: | ||
| - sysroot | ||
|
|
@@ -18,16 +15,3 @@ c_stdlib_version: | |
|
|
||
| cmake_version: | ||
| - ">=3.30.4" | ||
|
Comment on lines
-22
to
-33
Member
Author
There was a problem hiding this comment. These are CUDA 11 pinnings for dependencies outside the Conda 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 |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,15 +55,14 @@ requirements: | |
| host: | ||
| - cuda-version ={{ cuda_version }} | ||
| - cuda-cudart-dev | ||
| - libcufile-dev | ||
|
Member
Author
There was a problem hiding this comment. cuFile is available on all platforms. Hence it is added as a dependency here |
||
| - libnvjpeg-dev | ||
|
Comment on lines
59
to
-60
Member
Author
There was a problem hiding this comment. nvJPEG is now dynamically linked to in the Conda case. Hence no need for the static library |
||
| - nvtx-c >=3.1.0 | ||
| - openslide | ||
| run: | ||
| - {{ pin_compatible('cuda-version', max_pin='x', min_pin='x') }} | ||
| - cuda-cudart | ||
| - libcufile | ||
|
Member
Author
There was a problem hiding this comment. 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 |
||
| - libnvjpeg | ||
| run_constrained: | ||
| - {{ pin_compatible('openslide') }} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -125,8 +125,6 @@ superbuild_depend(cli11) | |
| superbuild_depend(pugixml) | ||
| superbuild_depend(json) | ||
| superbuild_depend(libdeflate) | ||
|
Comment on lines
-128
to
-129
Member
Author
There was a problem hiding this comment. 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 |
||
|
|
||
| ################################################################################ | ||
| # Find cucim package | ||
|
|
@@ -216,11 +214,6 @@ target_compile_options(${CUCIM_PLUGIN_NAME} PRIVATE $<$<COMPILE_LANGUAGE:CXX>:-W | |
| # Link libraries | ||
| target_link_libraries(${CUCIM_PLUGIN_NAME} | ||
| PRIVATE | ||
| deps::fmt | ||
| cucim::cucim | ||
| deps::libtiff | ||
|
|
@@ -231,6 +224,22 @@ target_link_libraries(${CUCIM_PLUGIN_NAME} | |
| deps::json | ||
| deps::libdeflate | ||
| ) | ||
| 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 | ||
|
Comment on lines
+230
to
+233
Member
Author
There was a problem hiding this comment. Looks like I goofed on the indenting here. Will follow up on this |
||
| CUDA::cudart | ||
| ) | ||
| else() | ||
| target_link_libraries(${CUCIM_PLUGIN_NAME} | ||
| PRIVATE | ||
| CUDA::nvjpeg | ||
| CUDA::cudart | ||
| ) | ||
| endif() | ||
|
Comment on lines
+227
to
+242
Member
Author
There was a problem hiding this comment. 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 |
||
|
|
||
| target_include_directories(${CUCIM_PLUGIN_NAME} | ||
| PUBLIC | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,9 @@ if (NOT APPLE) | |
| set(CMAKE_INSTALL_RPATH $ORIGIN) | ||
| endif () | ||
|
|
||
| # Find CUDA Toolkit for cudart and cufile. | ||
| find_package(CUDAToolkit REQUIRED) | ||
|
Comment on lines
+27
to
+28
Member
Author
There was a problem hiding this comment. To ensure cuFile is picked up from the CTK, we add this Note that in the case where nvJPEG is used, we already had an equivalent line. Hence no change there |
||
|
|
||
| ################################################################################ | ||
| # Add library: cufile_stub | ||
| ################################################################################ | ||
|
|
@@ -52,52 +55,19 @@ target_compile_options(cufile_stub | |
| ) | ||
|
|
||
| ## Link libraries | ||
| if (CUCIM_STATIC_GDS) | ||
| # Enabling CUCIM_STATIC_GDS statically links cuFile | ||
| target_link_libraries(cufile_stub | ||
| PUBLIC | ||
| ${CMAKE_DL_LIBS} | ||
|
Comment on lines
+58
to
62
Member
Author
There was a problem hiding this comment. As another note, it appears we define a bunch of stuff unconditionally that needs |
||
| PRIVATE | ||
|
Comment on lines
+58
to
63
Member
Author
There was a problem hiding this comment. 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 It just replaces |
||
|
Comment on lines
-62
to
-73
Member
Author
There was a problem hiding this comment. This was just to find the path to the cuFile download we did on CI Using CMake's |
||
|
Comment on lines
-80
to
-90
Member
Author
There was a problem hiding this comment. Again obviated by CMake's |
||
| CUDA::cuFile_static | ||
| ) | ||
|
Comment on lines
+64
to
65
Member
Author
There was a problem hiding this comment. Here is where |
||
| else() | ||
| # Use `dlopen` to load cuFile at runtime | ||
| target_link_libraries(cufile_stub | ||
| PUBLIC | ||
| ${CMAKE_DL_LIBS} | ||
| ) | ||
|
Comment on lines
+66
to
71
Member
Author
There was a problem hiding this comment. This is just the same dynamic cuFile library case as before Lines 54 to 60 in 54cc342 For some reason GitHub included Also as we use We did include the That said, @gigony please let me know if I'm missing something here |
||
| endif() | ||
|
|
||
|
|
@@ -110,9 +80,8 @@ PUBLIC | |
| target_include_directories(cufile_stub | ||
| PUBLIC | ||
| $<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/include> | ||
| ${CUDAToolkit_INCLUDE_DIRS} | ||
|
Member
Author
There was a problem hiding this comment. Since cuFile is picked up from the CTK now, this can just be the CTK header directories |
||
| PRIVATE | ||
|
Member
Author
There was a problem hiding this comment. As Greg already noted, the comment above is irrelevant. So it is dropped |
||
| ${CMAKE_CURRENT_SOURCE_DIR}/../cpp/include # for including helper.h in cucim/dynlib | ||
| ) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,12 @@ | |
| #ifndef CUCIM_CUFILE_STUB_H | ||
| #define CUCIM_CUFILE_STUB_H | ||
|
|
||
| // Try to include the real cufile.h, fall back to minimal types if not available | ||
| #if __has_include(<cufile.h>) | ||
| #include <cufile.h> | ||
| #else | ||
| #include "cufile_stub_types.h" | ||
| #endif | ||
|
Comment on lines
+19
to
+24
Member
Author
There was a problem hiding this comment. @grlee77 it looks like you added this change. Could you please share a bit more context? Given the CTK has Is there some other issue that is coming up? |
||
|
|
||
| #include "cucim/dynlib/helper.h" | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,7 +34,7 @@ | |
| "\n", | ||
| "or\n", | ||
| "```\n", | ||
| "!pip install cupy-cuda12x\n", | ||
| "!pip install torch==1.7.1+cu110 torchvision==0.8.2+cu110 torchaudio==0.7.2 -f https://download.pytorch.org/whl/torch_stable.html\n", | ||
|
Comment on lines
-37
to
38
Member
Author
There was a problem hiding this comment. @grlee77 thanks for updating the CuPy packages! 🙏 Should we do the same with PyTorch and friends? There are a bunch more lines like these throughout the notebooks. Perhaps we should update them all and rerun to make sure they are working ok |
||
| "```" | ||
| ] | ||
|
|
@@ -49,7 +49,7 @@ | |
| "\n", | ||
| "# or\n", | ||
| "\n", | ||
| "#!pip install cupy-cuda12x\n", | ||
| "#!pip install torch==1.7.1+cu110 torchvision==0.8.2+cu110 torchaudio==0.7.2 -f https://download.pytorch.org/whl/torch_stable.html" | ||
| ] | ||
| }, | ||
|
|
||
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.
No more GCC 11 now that we are on CUDA 12+
This sticks with GCC 13 on CUDA 12 for simplicity
Though we have been bumping to GCC 14 in RAPIDS projects. So that could be done as a follow up