Remove cudf components - #1083
Conversation
Restore benchmarks, examples, non-cudf tests, sanitizer suppressions, and shuffler header that were deleted outside the intended cudf-specific removal scope. Only streaming/cudf and integrations/cudf directories were supposed to be removed.
After moving cudf-specific streaming code to the cudf_streaming package, update the rapidsmpf benchmarks, tests, and examples that use this functionality to include the new <cudf_streaming/...> headers instead of the removed <rapidsmpf/streaming/cudf/...> headers.
b9e7f2b to
8d136d0
Compare
Remove get_cudf.cmake and cudf::cudf link from core CMakeLists.txt. Replace the single cudf::detail::memcpy_async call in cuda_memcpy_async.hpp with a direct cudaMemcpyAsync call — the cudf wrapper is identical behavior.
Replace all cudf:: stream/memory-resource getters with RMM equivalents:
- cudf::get_default_stream() -> rmm::cuda_stream_view{}
- cudf::get_current_device_resource_ref() -> rmm::mr::get_current_device_resource_ref()
- cudf::test::BaseFixtureWithParam -> ::testing::TestWithParam directly
Remove dead cudf_test includes and cudf::cudftestutil link from CMakeLists.
Delete bench_pack.cpp and random_data.{hpp,cu} which were already moved
to the cudf repo. Replace cudf::get_default_stream() and
cudf::get_current_device_resource_ref() in bench_comm.cpp with RMM
equivalents. Clean CMakeLists.txt to remove the deleted targets.
Remove from_cudf_packed_columns classmethod from PackedData (moved to cudf_streaming in cudf repo). Delete testing.py (unused) and streaming_benchmark.py (moved to cudf repo). Remove libcudf.load_library() call from librapidsmpf/load.py.
8d136d0 to
21a841d
Compare
pentschev
left a comment
There was a problem hiding this comment.
Thanks @vyasr . I did an initial pass and left a few comments. In particular I'm a bit concerned about removing certain benchmarks/examples and more importantly tests that we had for "core" components both in C++ and Python. I think some of them were not necessarily intended to be moved to cudf_streaming, but maybe I'm missing some context. For tests in particular, I think we should still keep core components being tests against cuDF, as they provide important coverage that we would lose if they're not handled here. Would be also good to hear @madsbk 's opinion (and Lawrence too, but I'm not tagging him since he's on PTO).
| --exclude libkvikio.so \ | ||
| --exclude libnvcomp.so.5 \ |
There was a problem hiding this comment.
I think kvikio and nvcomp were here for cuDF, so they should probably be removed too.
| CPM_ARGS | ||
| GIT_REPOSITORY https://github.com/NVIDIA/cuCascade.git | ||
| GIT_TAG main | ||
| GIT_TAG 6a8cf0f7c545c601cec4b41a0da22ad6a17eeb7c |
There was a problem hiding this comment.
Why is this pinning to a specific commit and not main?
| COMPONENT benchmarking | ||
| DESTINATION bin/benchmarks/librapidsmpf | ||
| EXCLUDE_FROM_ALL | ||
| ) |
There was a problem hiding this comment.
Why are we removing benchmarks? I think they're not necessarily part of the move to cudf_streaming, in particular those that are not part of the streaming submodule, are they?
There was a problem hiding this comment.
I was thinking the same. Cant benchmarks depend on cudf_streaming?
There was a problem hiding this comment.
If I'm reading this right, it's still there, but moved down below into a conditional block if(BUILD_CUDF_TESTS).
There was a problem hiding this comment.
I'm not sure we want to remove Shuffler tests entirely. We probably want to find a middle-ground here, since removing this file completely will leave Shuffler pretty much untested. Maybe for benchmarks tests we should still link to cuDF since they're not packaged and keep them mostly "as is"?
Remove all depends_on_libcudf and depends_on_pylibcudf entries from dependencies.yaml, conda recipes, pyproject.toml, and conda environments. Remove libcudf.so auditwheel excludes from wheel build scripts. Remove cudf/libcudf/pylibcudf from version-bump script. Remove cudf Doxygen tagfile, intersphinx mapping, and stale API docs for removed integrations module. Remove cudf ASAN/compute-sanitizer suppressions. Remove cudf from clang-format include priority and ruff isort sections. Update docs to reflect cudf is no longer a direct dependency.
…id shell splitting
…atchAsync support
cca7c83 to
7e5d658
Compare
…udf_streaming CPM
There was a problem hiding this comment.
Possibly this and bench_partition.cpp could move to cudf_streaming, they're primarily cuDF tests with no collectives, I don't think it has to be in this PR though. WDYT @nirandaperera ?
There was a problem hiding this comment.
Seems like the change in permissions is wanted since execution is removed from this file that contains a shebang.
… deps bench_read and other NDSH benchmarks link cudf_streaming::cudf_streaming which transitively pulls in libcudf.so. Since benchmarks are installed into the librapidsmpf package (--component=benchmarking), the package needs these runtime dependencies declared to pass the overlinking check.
…orts Tests and examples import cudf partition/streaming APIs that moved from rapidsmpf.integrations.cudf and rapidsmpf.streaming.cudf to cudf_streaming.integrations and cudf_streaming.streaming respectively. - Add depends_on_pylibcudf and depends_on_cudf_streaming to test_python and py_test_rapidsmpf file keys - Remap all Python imports to cudf_streaming namespace - Regenerate pyproject.toml via rapids-dependency-file-generator
… cudf-streaming conda-only dep
| # This warning needs to be suppressed because some parts of rapidsmpf instantiate templated CCCL | ||
| # functions in contexts where the resulting instantiations would have internal linkage (e.g. in | ||
| # anonymous namespaces). In such contexts, the visibility attribute on the template is ignored, and | ||
| # the compiler issues a warning. This is not a problem and will be fixed in future versions of CCCL. |
There was a problem hiding this comment.
Follow up: This suppression has been here since @madsbk's "initial commit" 2 years ago. We should re-evaluate if it is still needed and remove it if not.
| - ${{ stdlib("c") }} | ||
| by_name: | ||
| - cuda-cupti | ||
| - libcudf |
There was a problem hiding this comment.
We still need to ignore libcudf (and possibly other dependencies? Please double-check the list below). This is now being included from cache-host, because libcudf has a run-export on itself. https://github.com/rapidsai/rapidsmpf/actions/runs/27112901660/job/80014151119?pr=1083#step:11:1401
│ │ Finalized run dependencies (librapidsmpf-26.08.00a74-cuda13_260608_c925c61f):
│ │ ╭──────────────────┬──────────────────────────────────────────────────────╮
│ │ │ Name ┆ Spec │
│ │ ╞══════════════════╪══════════════════════════════════════════════════════╡
│ │ │ Run dependencies ┆ │
│ │ │ cuda-cupti ┆ * │
│ │ │ cuda-version ┆ >=13,<14.0a0 (PC) │
│ │ │ libcudf ┆ >=26.8.0a615,<26.9.0a0 (RE of [cache-host: libcudf]) │
│ │ │ libnuma ┆ >=2.0.18,<3.0a0 (RE of [host: libnuma]) │
│ │ │ ┆ >=2.0.18,<3.0a0 (RE of [cache-host: libnuma]) │
│ │ │ libpmix ┆ >=5.0,<6.0 │
│ │ │ ┆ >=5.0.8,<6.0a0 (RE of [host: libpmix-devel]) │
│ │ │ ┆ >=5.0.8,<6.0a0 (RE of [cache-host: libpmix-devel]) │
│ │ │ librmm ┆ 26.8.* │
│ │ │ libucxx ┆ 0.51.* │
│ │ │ ┆ >=0.51.0a25,<0.52.0a0 (RE of [host: libucxx]) │
│ │ │ ┆ >=0.51.0a25,<0.52.0a0 (RE of [cache-host: libucxx]) │
│ │ │ openmpi ┆ >=5.0 │
│ │ ╰──────────────────┴──────────────────────────────────────────────────────╯
Only the librapidsmpf-tests package should have libcudf / libcudf-streaming dependencies. https://github.com/rapidsai/rapidsmpf/actions/runs/27112901660/job/80014151119?pr=1083#step:11:1980
│ │ Finalized run dependencies (librapidsmpf-tests-26.08.00a74-cuda13_260608_c925c61f):
│ │ ╭───────────────────┬──────────────────────────────────────────────────────╮
│ │ │ Name ┆ Spec │
│ │ ╞═══════════════════╪══════════════════════════════════════════════════════╡
│ │ │ Run dependencies ┆ │
│ │ │ cuda-cupti ┆ * │
│ │ │ cuda-version ┆ >=13,<14.0a0 (PC) │
│ │ │ libcudf ┆ 26.8.* │
│ │ │ ┆ >=26.8.0a615,<26.9.0a0 (RE of [host: libcudf]) │
│ │ │ ┆ >=26.8.0a615,<26.9.0a0 (RE of [cache-host: libcudf]) │
│ │ │ libcudf-streaming ┆ 26.8.* │
│ │ │ libnuma ┆ >=2.0.18,<3.0a0 (RE of [host: libnuma]) │
│ │ │ ┆ >=2.0.18,<3.0a0 (RE of [cache-host: libnuma]) │
│ │ │ libpmix ┆ >=5.0.8,<6.0a0 (RE of [cache-host: libpmix-devel]) │
│ │ │ librmm ┆ 26.8.* │
│ │ │ libucxx ┆ 0.51.* │
│ │ │ ┆ >=0.51.0a25,<0.52.0a0 (RE of [cache-host: libucxx]) │
│ │ │ openmpi ┆ >=5.0 │
│ │ ╰───────────────────┴──────────────────────────────────────────────────────╯
| script: | ||
| content: | | ||
| cmake --install cpp/build --component testing | ||
| cmake --install cpp/build --component=benchmarking |
There was a problem hiding this comment.
This has an = but the line above doesn't. Let's be consistent.
There was a problem hiding this comment.
Fixed — using --component=testing now for consistency.
| - libcudf-streaming =${{ minor_version }} | ||
| - libcudf =${{ minor_version }} |
There was a problem hiding this comment.
We should alphabetize these (libcudf before libcudf-streaming). Please apply throughout.
There was a problem hiding this comment.
Fixed — libcudf before libcudf-streaming throughout the recipe.
| } | ||
| if (msg.holds<rapidsmpf::streaming::TableChunk>()) { | ||
| auto chunk = co_await msg.release<rapidsmpf::streaming::TableChunk>() | ||
| if (msg.holds<cudf_streaming::streaming::TableChunk>()) { |
There was a problem hiding this comment.
It's a little odd to have "streaming" twice in the name, maybe this can be refactored later.
There was a problem hiding this comment.
Renaming the namespaces is tracked in NVIDIA/cudf#22771.
| COMPONENT benchmarking | ||
| DESTINATION bin/benchmarks/librapidsmpf | ||
| EXCLUDE_FROM_ALL | ||
| ) |
There was a problem hiding this comment.
If I'm reading this right, it's still there, but moved down below into a conditional block if(BUILD_CUDF_TESTS).
| - Regex: '^<(thrust|cub|cuda)/' | ||
| Priority: 40 # CCCL includes | ||
| - Regex: '^<(cudf.*|rmm|cugraph|cuml|raft|kvikio|cucascade)' | ||
| - Regex: '^<(rmm|cugraph|cuml|raft|kvikio|cucascade)' |
There was a problem hiding this comment.
This change seems unexpected to me. Why remove cudf here? We still have a few optional includes, in the tests/benchmarks. We try to keep this file mostly-consistent across RAPIDS, which is why it mentions other libraries too.
There was a problem hiding this comment.
You're right, tests and benchmarks still #include <cudf/...>. Added cudf back to the RAPIDS includes regex.
| message(STATUS " RAPIDSMPF_VERBOSE_INFO : ${RAPIDSMPF_VERBOSE_INFO}") | ||
| message(STATUS " BUILD_CUDF_TESTS : ${BUILD_CUDF_TESTS}") |
There was a problem hiding this comment.
Spacing.
| message(STATUS " RAPIDSMPF_VERBOSE_INFO : ${RAPIDSMPF_VERBOSE_INFO}") | |
| message(STATUS " BUILD_CUDF_TESTS : ${BUILD_CUDF_TESTS}") | |
| message(STATUS " RAPIDSMPF_VERBOSE_INFO : ${RAPIDSMPF_VERBOSE_INFO}") | |
| message(STATUS " BUILD_CUDF_TESTS : ${BUILD_CUDF_TESTS}") |
| - *pylibcudf_unsuffixed | ||
| depends_on_cudf_streaming: | ||
| common: | ||
| - output_types: conda |
There was a problem hiding this comment.
Are there wheels for cudf-streaming yet? I thought I approved some code for that but maybe it wasn't merged at the time this was written. Please check.
There was a problem hiding this comment.
Not yet. We should have had them after NVIDIA/cudf#22760, but there was an oversight in that PR's changes to build.yaml — it didn't properly specify that Python artifacts of cudf_streaming use the stable ABI, which causes failures like this one. The circularity of the rapidsmpf↔cudf dependency tree (partly alleviated by this branch making cudf a test-only dep of rapidsmpf) meant we couldn't merge a fix there until this rapidsmpf branch lands. Once this merges we'll get that fixed and enable the wheels.
bdice
left a comment
There was a problem hiding this comment.
Thank you. Approving packaging and CI and CMake.
|
It seems Niranda's concerns (like mine) have been addressed, so I'm going ahead and merging it to speedup unblocking. |
|
/merge |
Concerns should have been addressed. Please open an issue if something is still not addressed.
This PR removes the `-diag-suppress=1407` NVCC flag from `cmake/Modules/ConfigureCUDA.cmake`. ## Background The suppression was added in the initial commit ~2 years ago to silence NVCC warning 1407, which fires when CCCL templates are instantiated in contexts with internal linkage (e.g. anonymous namespaces). The comment noted: *"This is not a problem and will be fixed in future versions of CCCL."* The item was tracked as a follow-up in [NVIDIA/cudf#22771](NVIDIA/cudf#22771), referenced from [PR #1083 discussion](#1083 (comment)). ## Why it's safe to remove 1. **The library has zero `.cu` files** — all source under `cpp/src/` is `.cpp`, so the NVCC-specific flag never applied to library compilation. 2. **The only `.cu` file is `cpp/tests/test_allreduce.cu`**, which uses CCCL templates (`cuda::std::plus`, `cuda::minimum`, etc.) inside an anonymous namespace — exactly the pattern that used to trigger the warning. 3. **Verified empirically**: compiling `test_allreduce.cu` with `-Werror=all-warnings` and **without** `-diag-suppress=1407` using CUDA 12.9 (nvcc V12.9.86) produces **zero** warning 1407 instances. CCCL has fixed the underlying visibility attribute issue. ## Addresses - Partial fix for [NVIDIA/cudf#22771](NVIDIA/cudf#22771) (the rapidsmpf follow-up item) Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Mads R. B. Kristensen (https://github.com/madsbk) URL: #1100
This PR removes all the parts of rapidsmpf that rely on libcudf from rapidsmpf. Those parts have all been moved to the cudf_streaming library in the cudf repository.
Tests and benchmarks remain (at least for now), with benchmarks having been moved to
librapidsmpf-testspackage to avoid introducing dependencies tolibcudf_streamingin the library package.