Create new libcudf_streaming C++ library - #22747
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. |
|
/ok to test |
|
/ok to test |
1 similar comment
|
/ok to test |
f47f5fa to
f11245f
Compare
|
/ok to test |
@vyasr, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/ |
f11245f to
5b4e206
Compare
|
/ok to test |
| [[nodiscard]] void* data() noexcept; | ||
|
|
||
| /** | ||
| * @brief @return Cnst Pointer to the underlying storage. |
There was a problem hiding this comment.
| * @brief @return Cnst Pointer to the underlying storage. | |
| * @brief @return Const Pointer to the underlying storage. |
| if (( SUITEERROR == 0 )); then | ||
| rapids-logger "Run libcudf examples" | ||
| timeout 30m ./ci/run_cudf_examples.sh | ||
| timeout 5m ./ci/run_cudf_examples.sh |
There was a problem hiding this comment.
Are the timeout reductions really intentional here? From my experience with CI, there's high variability and I'd suggest being wary.
There was a problem hiding this comment.
Fair point. I think it's safe based on what I've seen, but happy to revisit after this PR. Reverted to 30m for now.
| NAME cudf_streaming_single_tests | ||
| COMMAND cudf_streaming_single_tests | ||
| GPUS 1 | ||
| PERCENT 25 |
There was a problem hiding this comment.
It's to allow for test parallelism (rapids_test_add uses PERCENT to determine how many tests can run concurrently based on GPU resource allocation). But for now I've set it to 100 to disable that, since we don't have enough tests to benefit from parallelism yet.
nirandaperera
left a comment
There was a problem hiding this comment.
I think we can (and should) remove single communicator from the tests.
| #include <cstdint> | ||
| #include <memory> | ||
|
|
||
| namespace cudf_streaming::integrations { |
There was a problem hiding this comment.
I'm wondering if this integrations ns is required or not. Previously, cudf was one integration to rapidsmpf. But in cudf_streaming, I feel like this is not really necessary. 😇
There was a problem hiding this comment.
I agree, but I want the initial set of PRs to be as close to a copy of the rapidsmpf code as possible because 1) we should move quickly since rapidsmpf development is fully paused until these PRs merged and 2) these PRs are huge and hard to review and I'd rather have meaningful changes done in follow-up PRs that are smaller in scope and reasonable to review.
| #include <rapidsmpf/communicator/single.hpp> | ||
| #include <rapidsmpf/progress_thread.hpp> |
There was a problem hiding this comment.
So, cudf_streaming tests depend on rapidsmpf?
| @@ -0,0 +1,45 @@ | |||
| /** | |||
There was a problem hiding this comment.
I doubt we would need a custom test Environment for these tests. We might not even need rapidsmpf or the Single Communicator. Because these tests might not be passing messages around. (Do they?)
There was a problem hiding this comment.
I'm not sure, you would know better than me. But as above, I'd prefer for this PR to be a faithful upstreaming of rapidsmpf code and to defer this type of improvement to a follow-up PR.
- Fix typo in bloom_filter.hpp doxygen (Cnst -> Const) - Revert timeout reductions in test_cpp.sh back to 30m - Set PERCENT 100 for cudf_streaming tests (disable parallelism for now) - Add note to libcudf_streaming README that most users need not install directly
|
/ok to test |
bdice
left a comment
There was a problem hiding this comment.
Reviewed packaging / CMake changes again. Minor comments/questions, please address as you see fit. Thanks!
|
|
||
| if (( SUITEERROR == 0 )); then | ||
| rapids-logger "Run libcudf_streaming gtests" | ||
| timeout 30m ./ci/run_cudf_streaming_ctests.sh -j20 |
There was a problem hiding this comment.
Should we reduce the timeout, then?
Rename conda package from libcudf_streaming to libcudf-streaming to follow the convention used by cudf-polars and dask-cudf. Also remove the now-unnecessary entry from the version update script.
I addressed these reviews and Niranda gave the OK offline.
|
/merge |
This PR ports over the Python bindings for the parts of librapidsmpf as the new `libcudf_streaming` library in #22747 Note that, just like in the preexisting rapidsmpf testing setup, only the conda environment will run the MPI/UCXX tests. Contributes to #22528 Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Bradley Dice (https://github.com/bdice) - Matthew Murray (https://github.com/Matt711) - Matthew Roeschke (https://github.com/mroeschke) URL: #22760
The cudf_streaming tests are lightweight and complete quickly. Reduce the timeout from 30m to 5m which still provides ample headroom while avoiding masking long hangs. References: - NVIDIA#22747 (comment) - NVIDIA#22747 (comment) Signed-off-by: Vyas Ramasubramani <vyasr@nvidia.com>
Reduce the `timeout` for `cudf_streaming` C++ tests from 30m to 5m in `ci/test_cpp.sh`. The cudf_streaming tests are lightweight and complete quickly. The 30m timeout was kept during the initial PR (#22747) out of caution after reviewer feedback (#22747 (comment), #22747 (comment)), but 5m provides ample headroom while catching hangs much sooner. Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Bradley Dice (https://github.com/bdice) URL: #22880
Add `cudf_streaming` to the 'other libcudf includes' regex in `.clang-format` so that `cudf_streaming/` headers are sorted into the correct include category (priority 5) instead of falling through to the generic system includes bucket (priority 9). Follow-up from #22747 (comment)
Description
This PR ports over the libcudf-specific components of librapidsmpf into a new C++ library inside this repository that depends on librapidsmpf.
Contributes to #22528
Checklist