Add a versioned ABI namespace to RMM - #2462
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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds version-derived inline ABI namespaces for RMM, migrates headers and sources to the new namespace macros, documents ABI behavior, and adds CMake-based CI that builds current and previous ABI versions and verifies symbols and state isolation. ChangesABI Namespace Versioning
ABI Verification CI
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cpp/cmake/tests/CMakeLists.txt (1)
22-32: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding a
TIMEOUTto these tests.
rmm_abi_namespace_configure/rmm_abi_namespacefully configure and build RMM twice (current + previous) plus a consumer project and run readelf/state_test, with no bound on runtime. A hang anywhere in that chain (e.g. a build regression) will block the CI job indefinitely instead of failing fast.⏱️ Proposed fix
set_tests_properties(rmm_abi_namespace_configure PROPERTIES FIXTURES_SETUP rmm_abi_namespace) set_tests_properties(rmm_abi_namespace PROPERTIES FIXTURES_REQUIRED rmm_abi_namespace) +set_tests_properties(rmm_abi_namespace_configure PROPERTIES TIMEOUT 300) +set_tests_properties(rmm_abi_namespace PROPERTIES TIMEOUT 1800)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/cmake/tests/CMakeLists.txt` around lines 22 - 32, Add a TIMEOUT to both CTest entries for rmm_abi_namespace_configure and rmm_abi_namespace so the configure/build chain cannot hang indefinitely. Update the add_test calls in the CMakeLists test block to include a reasonable timeout for the configure step and the build step, keeping the existing FIXTURES_SETUP and FIXTURES_REQUIRED wiring intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/cmake/tests/rmm_abi_namespace/verify_symbol_versions.cmake`:
- Around line 14-18: Add a guard in verify_symbol_versions.cmake to ensure
CURRENT_RMM_ABI_NAMESPACE and PREVIOUS_RMM_ABI_NAMESPACE are not the same before
running the symbol checks. Update the existing validation block that iterates
over required_value so it also compares those two variables and raises a fatal
error when they match. Keep the change near the current namespace presence
checks so the failure happens early and the later symbol assertions in the same
script still rely on distinct ABI namespaces.
---
Nitpick comments:
In `@cpp/cmake/tests/CMakeLists.txt`:
- Around line 22-32: Add a TIMEOUT to both CTest entries for
rmm_abi_namespace_configure and rmm_abi_namespace so the configure/build chain
cannot hang indefinitely. Update the add_test calls in the CMakeLists test block
to include a reasonable timeout for the configure step and the build step,
keeping the existing FIXTURES_SETUP and FIXTURES_REQUIRED wiring intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d5d68399-b189-4839-8640-3d26e42df03f
📒 Files selected for processing (130)
.github/workflows/pr.yamlREADME.mdci/test_cmake.shcpp/cmake/tests/CMakeLists.txtcpp/cmake/tests/rmm_abi_namespace/CMakeLists.txtcpp/cmake/tests/rmm_abi_namespace/consumer/CMakeLists.txtcpp/cmake/tests/rmm_abi_namespace/consumer/current.cppcpp/cmake/tests/rmm_abi_namespace/consumer/current_state_a.cppcpp/cmake/tests/rmm_abi_namespace/consumer/current_state_b.cppcpp/cmake/tests/rmm_abi_namespace/consumer/previous.cppcpp/cmake/tests/rmm_abi_namespace/consumer/previous_state.cppcpp/cmake/tests/rmm_abi_namespace/consumer/state_test.cppcpp/cmake/tests/rmm_abi_namespace/verify_symbol_versions.cmakecpp/doxygen/Doxyfilecpp/include/rmm/aligned.hppcpp/include/rmm/cuda_device.hppcpp/include/rmm/cuda_stream.hppcpp/include/rmm/cuda_stream_pool.hppcpp/include/rmm/cuda_stream_view.hppcpp/include/rmm/detail/aligned.hppcpp/include/rmm/detail/export.hppcpp/include/rmm/detail/format.hppcpp/include/rmm/detail/nvtx/ranges.hppcpp/include/rmm/detail/runtime_capabilities.hppcpp/include/rmm/detail/runtime_shutdown.hppcpp/include/rmm/detail/stack_trace.hppcpp/include/rmm/detail/thrust_namespace.hcpp/include/rmm/device_buffer.hppcpp/include/rmm/device_scalar.hppcpp/include/rmm/device_uvector.hppcpp/include/rmm/device_vector.hppcpp/include/rmm/error.hppcpp/include/rmm/exec_policy.hppcpp/include/rmm/logger.hppcpp/include/rmm/mr/aligned_resource_adaptor.hppcpp/include/rmm/mr/arena_memory_resource.hppcpp/include/rmm/mr/binning_memory_resource.hppcpp/include/rmm/mr/callback_memory_resource.hppcpp/include/rmm/mr/cuda_async_managed_memory_resource.hppcpp/include/rmm/mr/cuda_async_memory_resource.hppcpp/include/rmm/mr/cuda_async_view_memory_resource.hppcpp/include/rmm/mr/cuda_memory_resource.hppcpp/include/rmm/mr/detail/aligned_resource_adaptor_impl.hppcpp/include/rmm/mr/detail/arena.hppcpp/include/rmm/mr/detail/arena_memory_resource_impl.hppcpp/include/rmm/mr/detail/binning_memory_resource_impl.hppcpp/include/rmm/mr/detail/callback_memory_resource_impl.hppcpp/include/rmm/mr/detail/coalescing_free_list.hppcpp/include/rmm/mr/detail/cuda_async_managed_memory_resource_impl.hppcpp/include/rmm/mr/detail/cuda_async_memory_resource_impl.hppcpp/include/rmm/mr/detail/failure_callback_resource_adaptor_impl.hppcpp/include/rmm/mr/detail/fixed_size_free_list.hppcpp/include/rmm/mr/detail/fixed_size_memory_resource_impl.hppcpp/include/rmm/mr/detail/free_list.hppcpp/include/rmm/mr/detail/limiting_resource_adaptor_impl.hppcpp/include/rmm/mr/detail/logging_resource_adaptor_impl.hppcpp/include/rmm/mr/detail/pool_memory_resource_impl.hppcpp/include/rmm/mr/detail/prefetch_resource_adaptor_impl.hppcpp/include/rmm/mr/detail/sam_headroom_memory_resource_impl.hppcpp/include/rmm/mr/detail/statistics_resource_adaptor_impl.hppcpp/include/rmm/mr/detail/stream_ordered_memory_resource.hppcpp/include/rmm/mr/detail/thread_safe_resource_adaptor_impl.hppcpp/include/rmm/mr/detail/tracking_resource_adaptor_impl.hppcpp/include/rmm/mr/failure_callback_resource_adaptor.hppcpp/include/rmm/mr/failure_callback_t.hppcpp/include/rmm/mr/fixed_size_memory_resource.hppcpp/include/rmm/mr/limiting_resource_adaptor.hppcpp/include/rmm/mr/logging_resource_adaptor.hppcpp/include/rmm/mr/managed_memory_resource.hppcpp/include/rmm/mr/per_device_resource.hppcpp/include/rmm/mr/pinned_host_memory_resource.hppcpp/include/rmm/mr/polymorphic_allocator.hppcpp/include/rmm/mr/pool_memory_resource.hppcpp/include/rmm/mr/prefetch_resource_adaptor.hppcpp/include/rmm/mr/sam_headroom_memory_resource.hppcpp/include/rmm/mr/statistics_resource_adaptor.hppcpp/include/rmm/mr/system_memory_resource.hppcpp/include/rmm/mr/thread_safe_resource_adaptor.hppcpp/include/rmm/mr/thrust_allocator_adaptor.hppcpp/include/rmm/mr/tracking_resource_adaptor.hppcpp/include/rmm/prefetch.hppcpp/include/rmm/process_is_exiting.hppcpp/include/rmm/resource_ref.hppcpp/src/aligned.cppcpp/src/cuda_device.cppcpp/src/cuda_stream.cppcpp/src/cuda_stream_pool.cppcpp/src/cuda_stream_view.cppcpp/src/device_buffer.cppcpp/src/error.cppcpp/src/exec_policy.cppcpp/src/logger.cppcpp/src/mr/aligned_resource_adaptor.cppcpp/src/mr/arena_memory_resource.cppcpp/src/mr/binning_memory_resource.cppcpp/src/mr/callback_memory_resource.cppcpp/src/mr/cuda_async_managed_memory_resource.cppcpp/src/mr/cuda_async_memory_resource.cppcpp/src/mr/cuda_async_view_memory_resource.cppcpp/src/mr/cuda_memory_resource.cppcpp/src/mr/detail/aligned_resource_adaptor_impl.cppcpp/src/mr/detail/arena_memory_resource_impl.cppcpp/src/mr/detail/binning_memory_resource_impl.cppcpp/src/mr/detail/callback_memory_resource_impl.cppcpp/src/mr/detail/cuda_async_managed_memory_resource_impl.cppcpp/src/mr/detail/cuda_async_memory_resource_impl.cppcpp/src/mr/detail/fixed_size_memory_resource_impl.cppcpp/src/mr/detail/limiting_resource_adaptor_impl.cppcpp/src/mr/detail/logging_resource_adaptor_impl.cppcpp/src/mr/detail/pool_memory_resource_impl.cppcpp/src/mr/detail/prefetch_resource_adaptor_impl.cppcpp/src/mr/detail/sam_headroom_memory_resource_impl.cppcpp/src/mr/detail/statistics_resource_adaptor_impl.cppcpp/src/mr/detail/thread_safe_resource_adaptor_impl.cppcpp/src/mr/detail/tracking_resource_adaptor_impl.cppcpp/src/mr/fixed_size_memory_resource.cppcpp/src/mr/limiting_resource_adaptor.cppcpp/src/mr/logging_resource_adaptor.cppcpp/src/mr/managed_memory_resource.cppcpp/src/mr/pinned_host_memory_resource.cppcpp/src/mr/pool_memory_resource.cppcpp/src/mr/prefetch_resource_adaptor.cppcpp/src/mr/sam_headroom_memory_resource.cppcpp/src/mr/statistics_resource_adaptor.cppcpp/src/mr/system_memory_resource.cppcpp/src/mr/thread_safe_resource_adaptor.cppcpp/src/mr/tracking_resource_adaptor.cppcpp/src/prefetch.cppcpp/src/runtime_shutdown.cppdependencies.yaml
baliika
left a comment
There was a problem hiding this comment.
Looks good from nvcomp's point of view, thank you!
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/mr/detail/callback_memory_resource_impl.cpp (1)
47-50: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClean up the allocation if
cudaStreamSynchronizefails.
allocate_synccan leakptrif synchronization throws afterallocatesucceeds. Deallocate the callback-owned allocation before rethrowing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/mr/detail/callback_memory_resource_impl.cpp` around lines 47 - 50, Update allocate_sync to handle synchronization failure after allocate succeeds: if cudaStreamSynchronize throws, deallocate the callback-owned ptr with the same stream, bytes, and alignment before rethrowing. Preserve the existing successful return path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cpp/src/mr/detail/callback_memory_resource_impl.cpp`:
- Around line 47-50: Update allocate_sync to handle synchronization failure
after allocate succeeds: if cudaStreamSynchronize throws, deallocate the
callback-owned ptr with the same stream, bytes, and alignment before rethrowing.
Preserve the existing successful return path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d81183f1-f237-414d-8956-0897de6774c2
📒 Files selected for processing (34)
.github/workflows/pr.yamlcpp/include/rmm/mr/cuda_async_view_memory_resource.hppcpp/include/rmm/mr/cuda_memory_resource.hppcpp/include/rmm/mr/detail/aligned_resource_adaptor_impl.hppcpp/include/rmm/mr/detail/arena.hppcpp/include/rmm/mr/detail/arena_memory_resource_impl.hppcpp/include/rmm/mr/detail/binning_memory_resource_impl.hppcpp/include/rmm/mr/detail/callback_memory_resource_impl.hppcpp/include/rmm/mr/detail/cuda_async_managed_memory_resource_impl.hppcpp/include/rmm/mr/detail/cuda_async_memory_resource_impl.hppcpp/include/rmm/mr/detail/failure_callback_resource_adaptor_impl.hppcpp/include/rmm/mr/detail/limiting_resource_adaptor_impl.hppcpp/include/rmm/mr/detail/logging_resource_adaptor_impl.hppcpp/include/rmm/mr/detail/prefetch_resource_adaptor_impl.hppcpp/include/rmm/mr/detail/sam_headroom_memory_resource_impl.hppcpp/include/rmm/mr/detail/statistics_resource_adaptor_impl.hppcpp/include/rmm/mr/detail/stream_ordered_memory_resource.hppcpp/include/rmm/mr/detail/thread_safe_resource_adaptor_impl.hppcpp/include/rmm/mr/detail/tracking_resource_adaptor_impl.hppcpp/include/rmm/mr/managed_memory_resource.hppcpp/include/rmm/mr/pinned_host_memory_resource.hppcpp/include/rmm/mr/system_memory_resource.hppcpp/src/mr/detail/aligned_resource_adaptor_impl.cppcpp/src/mr/detail/arena_memory_resource_impl.cppcpp/src/mr/detail/binning_memory_resource_impl.cppcpp/src/mr/detail/callback_memory_resource_impl.cppcpp/src/mr/detail/limiting_resource_adaptor_impl.cppcpp/src/mr/detail/logging_resource_adaptor_impl.cppcpp/src/mr/detail/prefetch_resource_adaptor_impl.cppcpp/src/mr/detail/sam_headroom_memory_resource_impl.cppcpp/src/mr/detail/statistics_resource_adaptor_impl.cppcpp/src/mr/detail/thread_safe_resource_adaptor_impl.cppcpp/src/mr/detail/tracking_resource_adaptor_impl.cppdependencies.yaml
🚧 Files skipped from review as they are similar to previous changes (30)
- cpp/include/rmm/mr/detail/cuda_async_managed_memory_resource_impl.hpp
- cpp/include/rmm/mr/detail/stream_ordered_memory_resource.hpp
- cpp/include/rmm/mr/system_memory_resource.hpp
- cpp/include/rmm/mr/detail/prefetch_resource_adaptor_impl.hpp
- cpp/src/mr/detail/logging_resource_adaptor_impl.cpp
- cpp/include/rmm/mr/cuda_async_view_memory_resource.hpp
- cpp/include/rmm/mr/detail/arena.hpp
- cpp/include/rmm/mr/detail/limiting_resource_adaptor_impl.hpp
- cpp/src/mr/detail/binning_memory_resource_impl.cpp
- cpp/src/mr/detail/tracking_resource_adaptor_impl.cpp
- cpp/src/mr/detail/aligned_resource_adaptor_impl.cpp
- cpp/src/mr/detail/thread_safe_resource_adaptor_impl.cpp
- cpp/include/rmm/mr/detail/binning_memory_resource_impl.hpp
- cpp/include/rmm/mr/detail/sam_headroom_memory_resource_impl.hpp
- cpp/src/mr/detail/statistics_resource_adaptor_impl.cpp
- cpp/src/mr/detail/arena_memory_resource_impl.cpp
- cpp/src/mr/detail/prefetch_resource_adaptor_impl.cpp
- cpp/src/mr/detail/sam_headroom_memory_resource_impl.cpp
- cpp/include/rmm/mr/managed_memory_resource.hpp
- cpp/include/rmm/mr/detail/failure_callback_resource_adaptor_impl.hpp
- cpp/include/rmm/mr/detail/arena_memory_resource_impl.hpp
- cpp/include/rmm/mr/detail/cuda_async_memory_resource_impl.hpp
- cpp/include/rmm/mr/detail/tracking_resource_adaptor_impl.hpp
- cpp/include/rmm/mr/detail/callback_memory_resource_impl.hpp
- cpp/src/mr/detail/limiting_resource_adaptor_impl.cpp
- cpp/include/rmm/mr/detail/statistics_resource_adaptor_impl.hpp
- cpp/include/rmm/mr/detail/aligned_resource_adaptor_impl.hpp
- .github/workflows/pr.yaml
- dependencies.yaml
- cpp/include/rmm/mr/cuda_memory_resource.hpp
24064df to
8a82174
Compare
|
/merge |
374bf81
into
rapidsai:release/26.08
This is an empty commit to trigger a build. This is needed after the RMM ABI break in rapidsai/rmm#2462.
This is an empty commit to trigger a build. This is needed after the RMM ABI break in rapidsai/rmm#2462.
This is an empty commit to trigger a build. This is needed after the RMM ABI break in rapidsai/rmm#2462.
This is an empty commit to trigger a build. This is needed after the RMM ABI break in rapidsai/rmm#2462.
This is an empty commit to trigger a build. This is needed after the RMM ABI break in rapidsai/rmm#2462.
This is an empty commit to trigger a build. This is needed after the RMM ABI break in rapidsai/rmm#2462.
This is an empty commit to trigger a build. This is needed after the RMM ABI break in rapidsai/rmm#2462.
This is an empty commit to trigger a build. This is needed after the RMM ABI break in rapidsai/rmm#2462.
This is an empty commit to trigger a build. This is needed after the RMM ABI break in rapidsai/rmm#2462.
This is an empty commit to trigger a build. This is needed after the RMM ABI break in rapidsai/rmm#2462.
## Description Upgrades nvcomp to version 5.3.0.16. Depends on rapidsai/rmm#2462. Replaces #23294. ## Checklist - [x] I am familiar with the [Contributing Guidelines](https://github.com/rapidsai/cudf/blob/HEAD/CONTRIBUTING.md). - [x] New or existing tests cover these changes. - [x] The documentation is up to date with these changes. --------- Co-authored-by: Bradley Dice <bdice@bradleydice.com> Co-authored-by: Vukasin Milovanovic <vmilovanovic@nvidia.com>
Description
Place RMM APIs in an RMM major/minor versioned inline ABI namespace while preserving the existing
rmm::source spelling. This allows different statically linked RMM ABI versions to coexist without duplicate strong symbols or ODR violations, while DSOs built against the same ABI version continue to share process-global resource state.RMM_NAMESPACE_BEGIN/RMM_NAMESPACE_ENDmacros using the generated RMM major/minor version.Validation:
rmm::*definitions.Checklist