Remove deprecated RMM Buffer support#656
Conversation
|
|
||
| # RMM memory resources for CUDA benchmarks (requires UCXX_ENABLE_RMM and get_rmm.cmake) | ||
| # RMM memory resources for CUDA benchmarks. | ||
| if(UCXX_BENCHMARKS_ENABLE_CUDA AND UCXX_ENABLE_RMM) |
There was a problem hiding this comment.
Do we need to remove more RMM references in this block?
There was a problem hiding this comment.
We still support RMM benchmarks, they have nothing to do with the Buffer class. Maybe at some point we may want to remove that, but for now I think it's useful for us to have the option to compare between both, unless our plan is to completely remove RMM memory resources and have everyone rely on CCCL directly from 26.08.
There was a problem hiding this comment.
No, we're not quite there yet, so let's leave these benchmarks. Most likely we'll never lose all the mrs in rmm, but I am hopeful that we'll eventually lose buffers.
vyasr
left a comment
There was a problem hiding this comment.
The README needs to be updated too, it refers to building the library with rmm support.
|
|
||
| # RMM memory resources for CUDA benchmarks (requires UCXX_ENABLE_RMM and get_rmm.cmake) | ||
| # RMM memory resources for CUDA benchmarks. | ||
| if(UCXX_BENCHMARKS_ENABLE_CUDA AND UCXX_ENABLE_RMM) |
There was a problem hiding this comment.
No, we're not quite there yet, so let's leave these benchmarks. Most likely we'll never lose all the mrs in rmm, but I am hopeful that we'll eventually lose buffers.
| option(BUILD_SHARED_LIBS "Build UCXX shared libraries" ON) | ||
| option(UCXX_ENABLE_RMM "Enable support for CUDA multi-buffer transfer with RMM" OFF) | ||
| option(UCXX_ENABLE_RMM "Enable RMM-backed test and benchmark code paths" OFF) | ||
| # TODO: Flip UCXX_ENABLE_CCCL default to OFF once devcontainer builds pass -DUCXX_ENABLE_CCCL=ON |
There was a problem hiding this comment.
We need to update the devcontainers to use CCCL here, then we can change this flag here. That way CCCL support stays optional.
There was a problem hiding this comment.
Thanks for pointing that, opened rapidsai/devcontainers#705 to address that.
There was a problem hiding this comment.
@pentschev Would it be too much to rename UCXX_ENABLE_RMM to UCXX_ENABLE_RMM_TESTING?
I had the same question as @bdice here
Fixed in 5082070 |
CCCL support for UCXX was added in rapidsai/ucxx#641, and rapidsai/ucxx#656 is removing RMM support. Switching to CCCL here is necessary.
nirandaperera
left a comment
There was a problem hiding this comment.
LGTM. It would be nice if we could rename UCXX_ENABLE_RMM to something more representative now (maybe UCXX_ENABLE_RMM_TESTING).
But I dont want to block the PR for that.
| option(BUILD_SHARED_LIBS "Build UCXX shared libraries" ON) | ||
| option(UCXX_ENABLE_RMM "Enable support for CUDA multi-buffer transfer with RMM" OFF) | ||
| option(UCXX_ENABLE_RMM "Enable RMM-backed test and benchmark code paths" OFF) | ||
| # TODO: Flip UCXX_ENABLE_CCCL default to OFF once devcontainer builds pass -DUCXX_ENABLE_CCCL=ON |
There was a problem hiding this comment.
@pentschev Would it be too much to rename UCXX_ENABLE_RMM to UCXX_ENABLE_RMM_TESTING?
I had the same question as @bdice here
|
Adding this to if(UCXX_ENABLE_CCCL)
target_link_libraries(${CMAKE_TEST_NAME} PRIVATE CUDA::cudart_static)
endif() |
Thanks Paul, done in e8ff9b0.
Thanks Niranda, done in 7e8ca76. |
|
Looks like either a similar change needs to be made in |
diff --git a/python/ucxx/ucxx/_lib/CMakeLists.txt b/python/ucxx/ucxx/_lib/CMakeLists.txt
index 68922af..d3488bf 100644
--- a/python/ucxx/ucxx/_lib/CMakeLists.txt
+++ b/python/ucxx/ucxx/_lib/CMakeLists.txt
@@ -5,8 +5,10 @@
# cmake-format: on
# =================================================================================
+find_package(CUDAToolkit REQUIRED)
+
set(cython_sources arr.pyx libucxx.pyx)
-set(linked_libraries ucxx::ucxx ucxx::python)
+set(linked_libraries ucxx::ucxx ucxx::python CUDA::cudart_static)
rapids_cython_create_modules(
CXX |
|
/merge |
The
Buffersupport for RMM was deprecated in favor of CCCL. Remove that for UCXX v0.51/RAPIDS 26.08.