Use full shape in buffer protocol#120
Conversation
Previously we would only use the first dimension in a Message shape in the __getbuffer__ method. This would result in poorly formed memoryview objects. Fixes rapidsai#119
This test generated rapidsai/ucx-py#120
|
FYI @Akshay-Venkatesh this was the cause of the zero-byte messages that I was seeing. |
This test generated rapidsai/ucx-py#120
| buffer.ndim = len(self.shape) | ||
| buffer.obj = self | ||
| buffer.readonly = 0 # TODO | ||
| buffer.shape = shape2 |
There was a problem hiding this comment.
Not part of the change here, but this line is a little concerning to me. It seems to be taking a pointer to memory allocated on the stack. Meaning this pointer won't be valid when we leave this function. I wouldn't be surprised if this causes segfaults somewhere else.
One alternative would be to dynamically allocate memory for buffer.shape to point to and fill in the content from shape2 and then clean up this memory in __releasebuffer__. Another alternative would be to make shape2 part of self.
Thoughts?
|
@mrocklin Your changes seem to be flattening the shape. Doesn't that result in shape knowledge being lost? ucx-py/ucp/_libs/ucp_py_buffer_helper.pyx Line 167 in 4030eff I'm likely missing the full picture here. @TomAugspurger |
|
FWIW, I wouldn't place high value on the code I wrote here being correct :)
…On Fri, Jun 7, 2019 at 2:26 PM Akshay-Venkatesh ***@***.***> wrote:
@mrocklin <https://github.com/mrocklin> Your changes seem to be
flattening the shape. Doesn't that result in shape knowledge being lost?
https://github.com/rapidsai/ucx-py/blob/4030eff63438ac87d58fd009656198a184914041/ucp/_libs/ucp_py_buffer_helper.pyx#L167
I likely missing the full picture here. @TomAugspurger
<https://github.com/TomAugspurger>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#120?email_source=notifications&email_token=AAKAOIUP3FHS7EBGLYK2ZPDPZKY5TA5CNFSM4HVZG56KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXGYWFI#issuecomment-500009749>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKAOIV5XZ7RPXHPX676CZ3PZKY5TANCNFSM4HVZG56A>
.
|
|
@Akshay-Venkatesh, is the |
|
If |
|
No thoughts from me. I honestly don't know Cython that well and don't have a sense for what is handled automatically or not. @jakirkham if you see a good obvious solution here would you be interested in making a quick PR?
Yes. This is less than ideal, but not terrible. We only really care about this thing as a byte buffer, not as a full array. We send shape and dtype information as part of a separate header. |
|
Yes, happily. One sec... |
|
Have started this with PR ( #121 ). There is a bunch of code that is assigning to |
|
If no one objects I'm going to merge this in for now. I think that this is a strict improvement. We can still continue with #121 afterwards. OK with you @jakirkham ? |
|
I am +1 for merging |
|
Wait, running tests with this PR and things are failing |
|
Nevermind -- tests are passing merging |
|
Sorry I let this one slip off my radar. In general anything that gets us closer to complying with the buffer protocol is a good thing. This is certainly a step in that direction. |
Previously we would only use the first dimension in a Message shape
in the getbuffer method. This would result in poorly formed memoryview
objects.
Fixes #119