WIP: Make shape a pointer and add ndim + strides by jakirkham · Pull Request #121 · rapidsai/ucx-py

jakirkham · 2019-06-07T20:31:06Z

Adds _shape and _strides as private pointers that contain the shape and strides of the data. Also adds ndim to aid tracking the dimensionality as a publicly visible value. Includes properties to view the shape and strides.

Note: Still need to handle some code that is assigning to self.shape as that is no longer valid with this approach.

mrocklin · 2019-06-07T20:40:03Z

Right, I get this error

Traceback (most recent call last):
  File "/home/nfs/mrocklin/miniconda/envs/ucx/lib/python3.7/site-packages/tornado/gen.py", line 736, in run
    yielded = self.gen.throw(*exc_info)  # type: ignore
  File "/home/nfs/mrocklin/distributed/distributed/nanny.py", line 354, in _on_exit
    yield self.scheduler.unregister(address=self.worker_address)
  File "/home/nfs/mrocklin/miniconda/envs/ucx/lib/python3.7/site-packages/tornado/gen.py", line 729, in run
    value = future.result()
  File "/home/nfs/mrocklin/miniconda/envs/ucx/lib/python3.7/site-packages/tornado/gen.py", line 736, in run
    yielded = self.gen.throw(*exc_info)  # type: ignore
  File "/home/nfs/mrocklin/distributed/distributed/core.py", line 741, in send_recv_from_rpc
    result = yield send_recv(comm=comm, op=key, **kwargs)
  File "/home/nfs/mrocklin/miniconda/envs/ucx/lib/python3.7/site-packages/tornado/gen.py", line 729, in run
    value = future.result()
  File "/home/nfs/mrocklin/miniconda/envs/ucx/lib/python3.7/site-packages/tornado/gen.py", line 736, in run
    yielded = self.gen.throw(*exc_info)  # type: ignore
  File "/home/nfs/mrocklin/distributed/distributed/core.py", line 533, in send_recv
    yield comm.write(msg, serializers=serializers, on_error="raise")
  File "/home/nfs/mrocklin/miniconda/envs/ucx/lib/python3.7/site-packages/tornado/gen.py", line 729, in run
    value = future.result()
  File "/home/nfs/mrocklin/distributed/distributed/comm/ucx.py", line 113, in write
    await self.ep.send_obj(meta)
  File "ucp/_libs/ucp_py.pyx", line 370, in ucp_py.Endpoint.send_obj
  File "ucp/_libs/ucp_py.pyx", line 348, in ucp_py.Endpoint._send_obj_host
  File "ucp/_libs/ucp_py_buffer_helper.pyx", line 204, in ucp_py.BufferRegion.populate_ptr
  File "ucp/_libs/ucp_py_buffer_helper.pyx", line 207, in ucp_py.BufferRegion._populate_ptr
AttributeError: attribute 'shape' of 'ucp_py.BufferRegion' objects is not writable
ERROR    asyncio:base_events.py:1608 Future exception was never retrieved
future: <Future finished exception=AttributeError("attribute 'shape' of 'ucp_py.BufferRegion' objects is not writable")>

when I run this test (on latest distributed master)

py.test distributed/comm/tests/test_ucx.py::test_stress -s --runslow

jakirkham · 2019-06-07T20:46:48Z

Yep, that's expected currently.

One of the things I'm trying to wrap my head around ATM is what kinds of buffers are we trying to handle here?

For instance in master ndim = 1. Is the general idea to only support 1-D buffers in this class? If so, the code can probably be simplified a lot and this assignment issue dealt with much more simply.

Also are we expecting buffers to be contiguous or may they be non-contiguous? Currently the code to create the buffer suggests it should be contiguous, but that is not enforced here.

jakirkham · 2019-06-07T20:53:48Z

Perhaps these are questions for @TomAugspurger or @Akshay-Venkatesh?

TomAugspurger · 2019-06-07T21:10:31Z

I thought that everything was 1D by this point, but that may have been mistaken?

…

On Fri, Jun 7, 2019 at 3:53 PM jakirkham ***@***.***> wrote: Perhaps these are questions for @TomAugspurger <https://github.com/TomAugspurger> or @Akshay-Venkatesh <https://github.com/Akshay-Venkatesh>? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#121?email_source=notifications&email_token=AAKAOIXP6BKUJXSJPG3NAYDPZLDFZA5CNFSM4HV2DAKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXG65OQ#issuecomment-500035258>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKAOIVYX4I7NFYUPD6SJKTPZLDFZANCNFSM4HV2DAKA> .

mrocklin · 2019-06-07T21:15:41Z

The fact that my PR fixes a failure implies that that might not be true.

jakirkham · 2019-06-07T21:26:47Z

Makes sense. I'm happy to keep pushing on this then. Just want to make sure I'm not overcomplicating things.

If anyone has thoughts on whether data is required to be contiguous or not, that would be good to know.

mrocklin · 2019-06-07T21:35:52Z

I don't know what the implementation does. I recall conversations where we said "lets not bother with non-contiguous data. If it's non-contiguous, let's just make a copy". I can't authoritatively say that that's what happened though.

TomAugspurger · 2019-06-07T21:42:24Z

That's my recollection as well.

…

On Fri, Jun 7, 2019 at 4:35 PM Matthew Rocklin ***@***.***> wrote: I don't know what the implementation does. I recall conversations where we said "lets not bother with non-contiguous data. If it's non-contiguous, let's just make a copy". I can't authoritatively say that that's what happened though. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#121?email_source=notifications&email_token=AAKAOISNYQ5JV5CYXSPY3M3PZLIDRA5CNFSM4HV2DAKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXHBY5A#issuecomment-500046964>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKAOIQUZLLXOUULVUPZ7RLPZLIDRANCNFSM4HV2DAKA> .

jakirkham · 2019-06-07T21:49:02Z

Is there a way to try sending a NumPy or CuPy array through UCX and seeing what you get out? I think this would be really informative as we could quickly see how these different cases are handled and get several test cases to include as a bonus.

Akshay-Venkatesh · 2019-06-10T14:51:01Z

@jakirkham This test written by @TomAugspurger is probably the easiest way to check what is being received.

mrocklin · 2019-06-18T14:28:11Z

When I try this with the ucx stress test I get the following error:

  File "/home/nfs/mrocklin/distributed/distributed/worker.py", line 742, in _register_with_scheduler
    serializers=["msgpack"],
  File "/home/nfs/mrocklin/miniconda/envs/ucx/lib/python3.7/site-packages/tornado/gen.py", line 729, in run
    value = future.result()
  File "/home/nfs/mrocklin/distributed/distributed/comm/ucx.py", line 113, in write
    await self.ep.send_obj(meta)
  File "ucp/_libs/ucp_py.pyx", line 370, in ucp_py.Endpoint.send_obj
  File "ucp/_libs/ucp_py.pyx", line 348, in ucp_py.Endpoint._send_obj_host
  File "ucp/_libs/ucp_py_buffer_helper.pyx", line 211, in ucp_py.BufferRegion.populate_ptr
  File "ucp/_libs/ucp_py_buffer_helper.pyx", line 214, in ucp_py.BufferRegion._populate_ptr
AttributeError: attribute 'shape' of 'ucp_py.BufferRegion' objects is not writable

I think that the proper thing to do here is to try this with the following test:

py.test distributed/comm/tests/test_ucx.py::test_stress --runslow

jakirkham · 2019-06-18T14:57:54Z

@mrocklin, I haven't been doing anything on this since we last discussed it. The commit pushed above included some changes locally that needed to be moved out of my local copy before doing other work. I doubt they (or this PR more generally) are complete as the commit message notes. Happy to take a look at this again if you would like. 🙂

jakirkham · 2019-06-18T17:15:52Z

Independently we don't seem to be handling buffer protocol requests correctly. Have raised issue ( #127 ) to discuss that.

mrocklin · 2019-08-13T14:23:29Z

@madsbk could I ask you to take a look at this PR and #120 ?

madsbk

I think it looks good. I have added some suggestions regarding support of multi-dimension memoryviews

madsbk · 2019-08-15T12:27:55Z

ucp/_libs/ucp_py_buffer_helper.pyx

+            self._shape[i] = 0
+        self._strides = <Py_ssize_t*>malloc(sizeof(Py_ssize_t) * self.ndim)
+        for i in range(self.ndim):
+            self._strides[i] = 1


To support multi-dimension memoryviews, you could do something like:

s = 1 for i in reversed(range(self.ndim)): self._strides[i] = s s *= self._strides[i]

Are BufferRegion.strides() supposed to be in elements or bytes?

madsbk · 2019-08-15T12:34:16Z

ucp/_libs/ucp_py_buffer_helper.pyx

            return 0
        else:
-            return self.shape[0]
+            return self._shape[0]


Ditto

size = 1 for i in range(self.ndim): size *= self._shape[i]

madsbk · 2019-08-15T12:35:17Z

ucp/_libs/ucp_py_buffer_helper.pyx

-        strides[0] = <Py_ssize_t>self.itemsize
-        assert len(self.shape)
-        if self.shape[0] == 0:
+        if self._shape[0] == 0:


Ditto

if len(self) == 0:

madsbk · 2019-08-15T12:36:01Z

ucp/_libs/ucp_py_buffer_helper.pyx

        buffer.itemsize = self.itemsize
-        buffer.len = self.shape[0] * self.itemsize
-        buffer.ndim = 1
+        buffer.len = self._shape[0] * self.itemsize


Ditto

buffer.len = self.nbytes()

jakirkham · 2019-08-15T13:24:24Z

Feel free to push changes directly to this PR @madsbk if you are comfortable. 🙂

madsbk · 2019-08-15T14:24:48Z

OK but I just realized that populate_ptr() and populate_cuda_ptr() also needs to support ndim changes.
@jakirkham, is it alright with you if I create a new PR that rewrites BufferRegion into a more clean implementation that incorporate your ideas?

jakirkham · 2019-08-15T17:22:04Z

Please go ahead, @madsbk. 🙂

jakirkham · 2019-08-19T16:56:38Z

I think this was basically addressed by @madsbk's PR ( #144 ). So closing it out. Though feel free to pull things from this if it winds up still being useful for some reason.

Make shape a pointer and add ndim + strides

09d1d41

jakirkham mentioned this pull request Jun 7, 2019

Use full shape in buffer protocol #120

Merged

Set ndim

f983409

wip

2964e40

madsbk suggested changes Aug 15, 2019

View reviewed changes

jakirkham closed this Aug 19, 2019

jakirkham deleted the hdl_shape_buf_protocol branch September 10, 2019 18:49

Conversation

jakirkham commented Jun 7, 2019

Uh oh!

mrocklin commented Jun 7, 2019

Uh oh!

jakirkham commented Jun 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakirkham commented Jun 7, 2019

Uh oh!

TomAugspurger commented Jun 7, 2019 via email

Uh oh!

mrocklin commented Jun 7, 2019

Uh oh!

jakirkham commented Jun 7, 2019

Uh oh!

mrocklin commented Jun 7, 2019

Uh oh!

TomAugspurger commented Jun 7, 2019 via email

Uh oh!

jakirkham commented Jun 7, 2019

Uh oh!

Akshay-Venkatesh commented Jun 10, 2019

Uh oh!

mrocklin commented Jun 18, 2019

Uh oh!

jakirkham commented Jun 18, 2019

Uh oh!

jakirkham commented Jun 18, 2019

Uh oh!

mrocklin commented Aug 13, 2019

Uh oh!

madsbk left a comment

Choose a reason for hiding this comment

Uh oh!

madsbk Aug 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

madsbk Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

madsbk Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

madsbk Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

madsbk Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

jakirkham commented Aug 15, 2019

Uh oh!

madsbk commented Aug 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakirkham commented Aug 15, 2019

Uh oh!

jakirkham commented Aug 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jakirkham commented Jun 7, 2019 •

edited

Loading

madsbk Aug 15, 2019 •

edited

Loading

madsbk commented Aug 15, 2019 •

edited

Loading