Repository navigation
Handle empty RMM buffers without an upstream stream - #1261
Merged
rapids-bot[bot] merged 3 commits intoOct 15, 2026
Merged
Conversation
Signed-off-by: Vyas Ramasubramani <vyasr@nvidia.com>
vyasr
added
bug
|
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. |
Contributor
Author
|
/ok to test f061499 |
This was referenced Oct 15, 2026
Follow-up to the fix for empty `rmm::device_buffer`s reporting an invalid stream after rapidsai/rmm#2546: - Document in `BufferResource::move()` that an empty buffer without a stream (default-constructed or moved-from `rmm::device_buffer`) is not synchronized and simply has its stream set to the target stream. - Simplify the invalid-stream check to `upstream == cuda::invalid_stream`, using the comparison operator provided by CCCL. - Add a regression test for moving a moved-from `rmm::device_buffer`, which rapidsai/rmm#2546 documents as a separate case that now reports an invalid stream. Signed-off-by: Peter Andreas Entschev <peter@entschev.com>
pentschev
marked this pull request as ready for review
October 15, 2026 08:25
pentschev
approved these changes
Oct 15, 2026
Member
There was a problem hiding this comment.
Thanks @vyasr for getting started with this. I hope you don't mind, I pushed a few changes myself to get this PR merged right away and unblock CI:
- Document in
BufferResource::move()that an empty buffer without a stream (default-constructed or moved-fromrmm::device_buffer) is not synchronized and simply has its stream set to the target stream. - Simplify the invalid-stream check to
upstream == cuda::invalid_stream, using the comparison operator provided by CCCL. Add a regression test for moving a moved-fromrmm::device_buffer, which rapidsai/rmm#2546 documents as a separate case that now reports an invalid stream.
EDIT: Moving from an rmm::device_buffer also moves out its memory resource, leaving an empty any_resource, so the subsequent is_host_accessible(data->memory_resource()) call dereferences an empty resource. The test was wrong and is removed now.
Member
|
/merge |
`BufferResource.MoveMovedFromDeviceBuffer` segfaults in `BufferResource::move()`. Moving from an `rmm::device_buffer` also moves out its memory resource (`_mr`), leaving an empty `any_resource`, so the subsequent `is_host_accessible(data->memory_resource())` call dereferences an empty resource. This is unrelated to the invalid-stream change in rapidsai/rmm#2546: moved-from buffers were never valid input to `BufferResource::move()`. Remove the test and drop the "moved-from" mentions from the `move()` docstring and the code comment, which only apply to default-constructed buffers. Signed-off-by: Peter Andreas Entschev <peter@entschev.com>
rapids-bot Bot
pushed a commit
that referenced
this pull request
Oct 19, 2026
Passing `cuda::invalid_stream` to a buffer factory used to succeed and only fail later, as `cudaErrorInvalidResourceHandle` from a CUDA call far from the cause. The public `HostBuffer::from_rmm_device_buffer` also skipped the stream ordering that `BufferResource::move` does. The buffer factories and `Buffer::rebind_stream` now throw `std::invalid_argument` on an invalid stream. `from_rmm_device_buffer` now orders its stream after the device buffer's pending work. Follow-up to #1261. - `BufferResource::make_buffer`, `BufferResource::move`, the `HostBuffer` constructors (used by `from_uint8_vector` and `from_owned_vector`), `HostBuffer::from_rmm_device_buffer` and `Buffer::rebind_stream` throw `std::invalid_argument` for `cuda::invalid_stream`. - `HostBuffer::from_rmm_device_buffer` makes `stream` wait on the device buffer's stream, and frees the memory on the `HostBuffer`'s current stream. Before, memory was freed on the original stream, which could be stale after `rebind_stream`. - The stream join in `BufferResource::move` and `from_rmm_device_buffer` now just checks `capacity() > 0`. An empty buffer owns no memory, so there's nothing to join. This includes default-constructed buffers, which have no stream. - Added tests that each entry point throws on an invalid stream, and updated the docstrings. Authors: - Niranda Perera (https://github.com/nirandaperera) Approvers: - Lawrence Mitchell (https://github.com/wence-) - Peter Andreas Entschev (https://github.com/pentschev) URL: #1263
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adopting a default-constructed or moved-from empty
rmm::device_buffercan attempt to record an event on an invalid CUDA stream after rapidsai/rmm#2546. Skip the upstream join only when the buffer has no allocation and carries the invalid-stream sentinel, then assign the requested stream before constructing the rapidsmpf buffer. Valid upstream streams retain their synchronization behavior.Add a regression test that adopts a default-constructed buffer onto a nondefault stream and waits for its recorded event. This addresses the
MetadataMessage.round_tripruntime failure separately from the rattler-build migration in #1258.