[nirandaperera]

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.

[pentschev]

Since the from_rmm_device_buffer deallocator now frees memory on the HostBuffer's current stream, HostBuffer::set_stream(cuda::invalid_stream) leads to freeing on an invalid stream inside the noexcept destructor. I think HostBuffer::set_stream should get the same std::invalid_argument.

[nirandaperera]

Fair point

[nirandaperera]

done

[pentschev]

Could you add an invalid-stream EXPECT_THROW for HostBuffer::from_rmm_device_buffer and HostBuffer::from_external? It would also help to test the new from_rmm_device_buffer behavior, build a pinned rmm::device_buffer on stream A, call from_rmm_device_buffer(..., B), check the contents are read correctly on B, and destroy it after set_stream(C). Right now nothing covers the new join or the free on the current stream.

[nirandaperera]

done

[pentschev]

Thanks for the checks. Two things from the earlier comment are still open:

1. an EXPECT_THROW for HostBuffer::from_external(span, invalid, ...)
2. a test of the new ordering and freeing. For example: fill a pinned rmm::device_buffer on stream A, call from_rmm_device_buffer(..., B), check the contents on B, then set_stream(C) and destroy it. If this is out of scope for this PR, that's fine, but please say so in the PR description, because it currently says each entry point is tested.

[nirandaperera]

I think 1 is already there. I will add a test to 2. That is a good point. It was a bug uncovered during this invalid stream changes

[pentschev]

Some more test asks but otherwise LGTM. I'm preemptively approving.

[pentschev]

br here is created without PinnedPoolProperties, so br->pinned_mr() throws std::invalid_argument while the argument is being built. EXPECT_THROW then passes without ever calling from_rmm_device_buffer. Could you create a separate resource with pinned memory turned on, e.g. BufferResource::create(rmm::mr::get_current_device_resource_ref(), rapidsmpf::PinnedPoolProperties{})? Then allocate the rmm::device_buffer outside the EXPECT_THROW, so only the from_rmm_device_buffer call is inside it.

[nirandaperera]

Yes, thanks. This was a good catch. this check was masked by the invalid arg exception from pinned mr, rather than the invalid stream

[pentschev]

Thanks for the checks. Two things from the earlier comment are still open:

1. an EXPECT_THROW for HostBuffer::from_external(span, invalid, ...)
2. a test of the new ordering and freeing. For example: fill a pinned rmm::device_buffer on stream A, call from_rmm_device_buffer(..., B), check the contents on B, then set_stream(C) and destroy it. If this is out of scope for this PR, that's fine, but please say so in the PR description, because it currently says each entry point is tested.

[nirandaperera]

/merge