Add `join` method to `DeviceBuffer` #1035

jakirkham · 2022-05-11T19:53:58Z

Adds join method to DeviceBuffer analogous to bytes.join(...). Provides a simple way to concatenate buffers together into a single one.

harrism · 2022-05-12T01:15:29Z

python/rmm/_lib/device_buffer.pyx

+        cdef uintptr_t sp = <uintptr_t>self.c_data()
+        cdef size_t sdbs = self.c_size()


lots of very short and undescriptive variable names in this function.

Definitely could use clearer and more canonical names like sp->sep, sdbs->sep_bytes, etc.

harrism · 2022-05-12T01:53:37Z

python/rmm/_lib/device_buffer.pyx

+        for i in range(N - 1):
+            db = L[i]
+            dp = <uintptr_t>db.c_data()
+            dbs = db.c_size()
+            copy_device_to_ptr(dp, rp + offset, dbs, stream)
+            offset += dbs
+            if sdbs > 0:
+                copy_device_to_ptr(sp, rp + offset, sdbs, stream)
+                offset += sdbs


Perhaps a comment explaining this loop. The variable names don't help clarify it.

shwina · 2022-05-12T15:36:48Z

@jakirkham could you please provide a bit more context here? Is this to enable a specific feature or algorithm?

leofang · 2022-05-12T16:16:45Z

python/rmm/_lib/device_buffer.pyx

+        db = L[N - 1]
+        dp = <uintptr_t>db.c_data()
+        dbs = db.c_size()
+        copy_device_to_ptr(dp, rp + offset, dbs, stream)


nit: can't we merge these lines into the loop?

Yes, we would just need to change 330 from if sdbs > 0 to if sdbs > 0 and i != N-1 and update the for loop on 324 to use range(N).

jakirkham · 2022-05-13T00:46:54Z

could you please provide a bit more context here? Is this to enable a specific feature or algorithm?

In Distributed serialization (part of spilling and communication), we often split and/or join frames as part of this process. ATM this just happens with host side serialization ("pickle" or "dask"). However we may want to extend this to CUDA-based serialization to allow for compression/decompression, consolidating many small frames into one larger one (like before sending), etc.

On the host side this is done with syntax like b"".join(...) or byte().join(...) or bytearray().join(...). Having support for similar syntax with DeviceBuffer would be handy as it would make easier to drop-in more places and leverage existing code paths in Distributed. Plus we can also be assured that the final result is a DeviceBuffer, which we are already use to working with in Dask serialization and RAPIDS will be familiar with as well.

This has also come up before primarily in issue ( rapidsai/cudf#9726 ). Though there is also overlap from ancillary issues ( rapidsai/ucx-py#478 ) ( rapidsai/dask-cuda#760 ).

vyasr

When I first read the description of this PR I was really expecting something more like a classmethod factory that just concatenated buffers together, I did not expect this to be a method using self as the separator for the purpose of concatenation. I see where the API is coming from though. It feels a little out of scope for RMM to me, but I also don't really see a better place for this feature to exist, so I'm OK with pushing forward if nobody else objects. The code needs some better variable naming/comments, but otherwise mostly looks fine.

vyasr · 2022-05-13T17:43:12Z

python/rmm/_lib/device_buffer.pyx

+        L : ``list`` of ``DeviceBuffer``s
+        stream : CUDA stream to use for copying, default the default stream


Suggested change

L : ``list`` of ``DeviceBuffer``s

stream : CUDA stream to use for copying, default the default stream

L : list

The ``DeviceBuffer``s to concatenate.

stream : Stream

The stream to use for copying. Defaults to the default stream

Can we also name the parameter buffers instead?

vyasr · 2022-05-13T17:44:09Z

python/rmm/_lib/device_buffer.pyx

@@ -285,6 +285,58 @@ cdef class DeviceBuffer:

        return b

+    cpdef DeviceBuffer join(self, list L, Stream stream=DEFAULT_STREAM):
+        """Joins a sequence of ``DeviceBuffer``s with ``self`` inbetween.


Suggested change

"""Joins a sequence of ``DeviceBuffer``s with ``self`` inbetween.

"""Joins a sequence of ``DeviceBuffer``s with ``self`` in between.

I think this would be clearer if you actually copied more of the docstring from help(b''.join). Specifically, it's not obvious that "in between" means that self is inserted between every consecutive element. Use of the word "separator" to describe self might also help.

vyasr · 2022-05-13T18:00:43Z

python/rmm/_lib/device_buffer.pyx

+        cdef uintptr_t sp = <uintptr_t>self.c_data()
+        cdef size_t sdbs = self.c_size()


Definitely could use clearer and more canonical names like sp->sep, sdbs->sep_bytes, etc.

vyasr · 2022-05-13T18:04:17Z

python/rmm/_lib/device_buffer.pyx

+        db = L[N - 1]
+        dp = <uintptr_t>db.c_data()
+        dbs = db.c_size()
+        copy_device_to_ptr(dp, rp + offset, dbs, stream)


Yes, we would just need to change 330 from if sdbs > 0 to if sdbs > 0 and i != N-1 and update the for loop on 324 to use range(N).

vyasr · 2022-05-13T18:07:58Z

python/rmm/_lib/device_buffer.pyx

+        cdef DeviceBuffer rdb = DeviceBuffer(size=s, stream=stream)
+        cdef uintptr_t rp = <uintptr_t>rdb.c_data()
+
+        cdef uintptr_t dp, offset = 0


Why maintain an offset when you can just increment rp directly?

jakirkham · 2022-05-20T07:19:44Z

Moving to 22.08

github-actions · 2022-06-19T08:01:04Z

This PR has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates. This PR will be labeled inactive-90d if there is no activity in the next 60 days.

github-actions · 2022-09-17T09:01:14Z

This PR has been labeled inactive-90d due to no recent activity in the past 90 days. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates.

harrism · 2022-09-28T04:36:48Z

@jakirkham @shwina @vyasr so what do you think, is this out of scope for RMM? Is this PR dead?

jakirkham · 2022-09-28T07:35:22Z

Going to close for now. Can reopen later as needed.

github-actions bot added the Python Related to RMM Python API label May 11, 2022

jakirkham added feature request New feature or request non-breaking Non-breaking change Python Related to RMM Python API and removed Python Related to RMM Python API labels May 11, 2022

jakirkham force-pushed the add_join branch 7 times, most recently from 2714e50 to 063f2da Compare May 11, 2022 23:40

Add join method to DeviceBuffer

bef339a

jakirkham force-pushed the add_join branch from 063f2da to bef339a Compare May 12, 2022 00:50

jakirkham marked this pull request as ready for review May 12, 2022 01:51

jakirkham requested a review from a team as a code owner May 12, 2022 01:51

harrism reviewed May 12, 2022

View reviewed changes

leofang reviewed May 12, 2022

View reviewed changes

vyasr requested changes May 13, 2022

View reviewed changes

jakirkham changed the base branch from branch-22.06 to branch-22.08 May 20, 2022 07:19

github-actions bot added the inactive-30d label Jun 19, 2022

github-actions bot added the inactive-90d label Sep 17, 2022

github-actions bot removed inactive-90d inactive-30d labels Sep 28, 2022

jakirkham closed this Sep 28, 2022

jakirkham deleted the add_join branch September 28, 2022 07:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `join` method to `DeviceBuffer` #1035

Add `join` method to `DeviceBuffer` #1035

jakirkham commented May 11, 2022

harrism May 12, 2022

vyasr May 13, 2022

harrism May 12, 2022

shwina commented May 12, 2022

leofang May 12, 2022

vyasr May 13, 2022

jakirkham commented May 13, 2022

vyasr left a comment

vyasr May 13, 2022

vyasr May 13, 2022

vyasr May 13, 2022

vyasr May 13, 2022

vyasr May 13, 2022

vyasr May 13, 2022

vyasr May 13, 2022

jakirkham commented May 20, 2022

github-actions bot commented Jun 19, 2022

github-actions bot commented Sep 17, 2022

harrism commented Sep 28, 2022

jakirkham commented Sep 28, 2022

		cdef uintptr_t sp = <uintptr_t>self.c_data()
		cdef size_t sdbs = self.c_size()

		L : ``list`` of ``DeviceBuffer``s
		stream : CUDA stream to use for copying, default the default stream

-        L : ``list`` of ``DeviceBuffer``s
-        stream : CUDA stream to use for copying, default the default stream
+        L : list
+            The ``DeviceBuffer``s to concatenate.
+        stream : Stream
+            The stream to use for copying. Defaults to the default stream

	"""Joins a sequence of ``DeviceBuffer``s with ``self`` inbetween.
	"""Joins a sequence of ``DeviceBuffer``s with ``self`` in between.

Add join method to DeviceBuffer #1035

Add join method to DeviceBuffer #1035

Conversation

jakirkham commented May 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shwina commented May 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakirkham commented May 13, 2022

vyasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakirkham commented May 20, 2022

github-actions bot commented Jun 19, 2022

github-actions bot commented Sep 17, 2022

harrism commented Sep 28, 2022

jakirkham commented Sep 28, 2022

Add `join` method to `DeviceBuffer` #1035

Add `join` method to `DeviceBuffer` #1035