Remove rmm::detail::polyfill in favor of CCCL concepts, require CCCL >=3.3#2283
Remove rmm::detail::polyfill in favor of CCCL concepts, require CCCL >=3.3#2283rapids-bot[bot] merged 2 commits intorapidsai:mainfrom
Conversation
📝 WalkthroughWalkthroughRemoves the rmm::detail::polyfill header and replaces all uses of its traits with direct CCCL Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/include/rmm/mr/system_memory_resource.hpp (1)
157-158: Add#include <cuda/memory_resource>to make this public header self-contained.This header directly uses
cuda::mr::device_accessible,cuda::mr::host_accessible, andcuda::mr::resource_within its public interface (lines 146, 149, 153, 157–158) but relies on transitive inclusion from<rmm/device_memory_resource.hpp>. Making the dependency explicit prevents brittleness if includes are reorganized.Proposed fix
`#include` <rmm/detail/format.hpp> `#include` <rmm/mr/device_memory_resource.hpp> +#include <cuda/memory_resource> + `#include` <cstddef> `#include` <string>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/include/rmm/mr/system_memory_resource.hpp` around lines 157 - 158, The header is missing an explicit include for cuda::mr types used in its public API (static_asserts referencing cuda::mr::resource_with, cuda::mr::device_accessible, cuda::mr::host_accessible and the system_memory_resource type); add `#include` <cuda/memory_resource> at the top of the header so the declarations using resource_with, device_accessible, and host_accessible are resolved without relying on transitive includes (ensuring the public header is self-contained).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cpp/include/rmm/mr/system_memory_resource.hpp`:
- Around line 157-158: The header is missing an explicit include for cuda::mr
types used in its public API (static_asserts referencing
cuda::mr::resource_with, cuda::mr::device_accessible, cuda::mr::host_accessible
and the system_memory_resource type); add `#include` <cuda/memory_resource> at the
top of the header so the declarations using resource_with, device_accessible,
and host_accessible are resolved without relying on transitive includes
(ensuring the public header is self-contained).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4f4c3a58-7cd8-4b1e-ad2e-78556db69ed4
📒 Files selected for processing (18)
cpp/include/rmm/detail/cccl_adaptors.hppcpp/include/rmm/detail/cuda_memory_resource.hppcpp/include/rmm/detail/export.hppcpp/include/rmm/mr/cuda_async_managed_memory_resource.hppcpp/include/rmm/mr/device_memory_resource.hppcpp/include/rmm/mr/is_resource_adaptor.hppcpp/include/rmm/mr/pinned_host_memory_resource.hppcpp/include/rmm/mr/system_memory_resource.hppcpp/include/rmm/mr/thrust_allocator_adaptor.hppcpp/tests/device_scalar_tests.cppcpp/tests/mock_resource.hppcpp/tests/mr/adaptor_tests.cppcpp/tests/mr/arena_mr_tests.cppcpp/tests/mr/cuda_async_mr_tests.cppcpp/tests/mr/cuda_async_view_mr_tests.cppcpp/tests/mr/host_mr_ref_tests.cppcpp/tests/mr/pool_mr_tests.cppcpp/tests/mr/system_mr_tests.cu
💤 Files with no reviewable changes (2)
- cpp/include/rmm/detail/cuda_memory_resource.hpp
- cpp/include/rmm/detail/cccl_adaptors.hpp
|
/merge |
Description
Closes #2282
Remove the
rmm::detail::polyfillnamespace and deletermm/detail/cuda_memory_resource.hpp. All usages are replaced with the CCCL concepts directly (cuda::mr::synchronous_resource,cuda::mr::resource, etc.). The CCCL version check is relocated tormm/detail/export.hppand updated to require CCCL >=3.3. (xref: rapidsai/rapids-cmake#981 -- also note that the CCCL 3.3 pre-release was the same as the final release)Checklist