[thrust] Use CCCL Runtime in Thrust set operation tests#9551
Conversation
OverviewThis PR refactors Thrust set operation tests to use CCCL Runtime APIs instead of raw CUDA stream management and Summary of ChangesNew Testing Helper HeaderA new helper header
Set Operation Test RefactoringAll six set operation test files follow a consistent refactoring pattern: Device-side tests (
CUDA streams tests:
Files updated:
Implementation NotesThe refactoring maintains the existing requirement that the test framework relies on current device settings. The author notes that reworking tests to use explicit device specifications—more aligned with CCCL runtime API best practices—is not currently feasible within existing test framework constraints. This PR establishes a pattern for future test updates as part of a planned series of PRs addressing this initiative. WalkthroughAdds ChangesCCCL Set-Op Test Refactor
Suggested reviewers
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
thrust/testing/cuda/set_difference.cu (1)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Use angle brackets for the helper include to match the project include rule.
-#include "cccl_runtime_test_helper.h" +#include <cuda/cccl_runtime_test_helper.h>As per coding guidelines, "All header inclusions must use the syntax
<header>."Source: Coding guidelines
thrust/testing/cuda/set_difference_by_key.cu (1)
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Use angle brackets for the helper include to match the project include rule.
-#include "cccl_runtime_test_helper.h" +#include <cuda/cccl_runtime_test_helper.h>As per coding guidelines, "All header inclusions must use the syntax
<header>."Source: Coding guidelines
thrust/testing/cuda/set_symmetric_difference.cu (1)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Use angle brackets for the helper include to match the project include rule.
-#include "cccl_runtime_test_helper.h" +#include <cuda/cccl_runtime_test_helper.h>As per coding guidelines, "All header inclusions must use the syntax
<header>."Source: Coding guidelines
thrust/testing/cuda/set_symmetric_difference_by_key.cu (1)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Use angle brackets for the helper include to match the project include rule.
-#include "cccl_runtime_test_helper.h" +#include <cuda/cccl_runtime_test_helper.h>As per coding guidelines, "All header inclusions must use the syntax
<header>."Source: Coding guidelines
thrust/testing/cuda/set_union.cu (1)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Use angle brackets for the helper include to match the project include rule.
-#include "cccl_runtime_test_helper.h" +#include <cuda/cccl_runtime_test_helper.h>As per coding guidelines, "All header inclusions must use the syntax
<header>."Source: Coding guidelines
thrust/testing/cuda/set_union_by_key.cu (1)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Use angle brackets for the helper include to match the project include rule.
-#include "cccl_runtime_test_helper.h" +#include <cuda/cccl_runtime_test_helper.h>As per coding guidelines, "All header inclusions must use the syntax
<header>."Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8d474be3-8b07-4e01-9de2-e9a08aaeaf38
📒 Files selected for processing (7)
thrust/testing/cuda/cccl_runtime_test_helper.hthrust/testing/cuda/set_difference.cuthrust/testing/cuda/set_difference_by_key.cuthrust/testing/cuda/set_symmetric_difference.cuthrust/testing/cuda/set_symmetric_difference_by_key.cuthrust/testing/cuda/set_union.cuthrust/testing/cuda/set_union_by_key.cu
| inline cuda::device_ref current_test_device() | ||
| { | ||
| int device = 0; | ||
| ASSERT_EQUAL(cudaSuccess, cudaGetDevice(&device)); | ||
| return cuda::device_ref{device}; | ||
| } | ||
|
|
||
| inline auto single_thread_config() | ||
| { | ||
| return cuda::make_config(cuda::make_hierarchy(cuda::grid_dims(1), cuda::block_dims<1>())); | ||
| } | ||
|
|
||
| __device__ inline void assert_device(bool condition, const char* expression, const char* file, int line) | ||
| { | ||
| if (!condition) | ||
| { | ||
| printf("Device assertion failed: %s (%s:%d)\n", expression, file, line); | ||
| __trap(); | ||
| } | ||
| } | ||
|
|
||
| template <typename Buffer> | ||
| void assert_equal(cuda::stream_ref stream, Buffer& buffer, cuda::std::initializer_list<int> expected) | ||
| { | ||
| std::vector<int> actual(buffer.size()); | ||
| cuda::copy_bytes(stream, buffer, actual); | ||
| stream.sync(); | ||
|
|
||
| ASSERT_EQUAL(actual.size(), expected.size()); | ||
|
|
||
| for (cuda::std::size_t i = 0; i < expected.size(); ++i) | ||
| { | ||
| ASSERT_EQUAL(expected.begin()[i], actual[i]); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
important: these functions are missing required CCCL API/noexcept annotations for header code. mark each function with the appropriate _CCCL_*_API macro (_CCCL_HOST_API for host helpers, _CCCL_DEVICE_API for assert_device) and add noexcept where applicable.
As per coding guidelines, **/*.{h,hpp,cu,cpp,cuh}: “Functions must be marked with one of: _CCCL_HOST_API, _CCCL_DEVICE_API, _CCCL_HOST_DEVICE_API, _CCCL_TILE_API, or _CCCL_API” and “Functions that do not throw exceptions must use noexcept.”
Source: Coding guidelines
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Question: Why isnt this a .cuh file?
| #include <cstdio> | ||
| #include <vector> | ||
|
|
||
| namespace test |
There was a problem hiding this comment.
Question: Could we name this like test_runtime? test is just so overloaded
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)
thrust/testing/cuda/cccl_runtime_test_helper.cuh (1)
24-43: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winimportant: add
inlineto the header-defined non-template functions at Line 24, Line 31, and Line 36. In this header form, missinginlinecan produce ODR/multiple-definition failures across test translation units.As per coding guidelines,
**/*.{h,hpp,cu,cpp,cuh}: “Non-template, non-constexprfunctions must useinline.”Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a147b50e-f88d-41c9-a4b5-ba8a3de15e07
📒 Files selected for processing (7)
thrust/testing/cuda/cccl_runtime_test_helper.cuhthrust/testing/cuda/set_difference.cuthrust/testing/cuda/set_difference_by_key.cuthrust/testing/cuda/set_symmetric_difference.cuthrust/testing/cuda/set_symmetric_difference_by_key.cuthrust/testing/cuda/set_union.cuthrust/testing/cuda/set_union_by_key.cu
🚧 Files skipped from review as they are similar to previous changes (6)
- thrust/testing/cuda/set_difference.cu
- thrust/testing/cuda/set_union.cu
- thrust/testing/cuda/set_union_by_key.cu
- thrust/testing/cuda/set_symmetric_difference.cu
- thrust/testing/cuda/set_symmetric_difference_by_key.cu
- thrust/testing/cuda/set_difference_by_key.cu
🥳 CI Workflow Results🟩 Finished in 54m 53s: Pass: 100%/70 | Total: 22h 42m | Max: 54m 36s | Hits: 93%/147545See results here. |
2 participants
Part of #2800
This PR is first on a series to use cccl runtime APIs in other CCCL tests as a form of dog fooding. The first PR is an arbitrary set of thrust tests to mostly gather feedback before I do more changes.
The test framework uses current device setting, which I believe is also needed for thrust. I don't think we can rework the tests to use an explicit device, even if that would make more sense with cccl runtime APIs.