You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
NumPy 1.24 seems to use multi-dimensional memoryviews when getting array data with array.data. msgpack does not play nicely with such multi-dimensional views.
Example
Here's a crashing example without NumPy, but creating a multi-dimensional memoryview by hand:
The packer actually requests a buffer object with the PyBUF_SIMPLE flag, so there should be no shape, strides, or suboffsets. See here. The API also says that "Without stride information, the buffer must be C-contiguous."
As long as I'm not misunderstanding something here Numpy should either return a 1D C-contiguous buffer (possibly creating a copy of non-contiguous arrays first), or return an error if it can't do that. So maybe Numpy is at fault here?
When packing a NumPy array (in my code not shown here), I explicilty ask for the memoryview using ndarray.data, so I don't blame NumPy for returning a multi-dimensional memoryview.
But even without NumPy: msgpack packs multi-dimensional memoryviews incorrectly as shown in the example, wouldn't you agree?
When packing a NumPy array (in my code not shown here), I explicilty ask for the memoryview using ndarray.data, so I don't blame NumPy for returning a multi-dimensional memoryview.
You're right, I misunderstood what you were trying to do. Numpy is not to blame, as it's job is done after you get the memoryview object out of it. My point still stands though, if the memoryview cannot produce a nice and simple contiguous buffer object it should return an error and packing should in turn fail.
@jfolz The memoryview is contiguous, but it's multi-dimensional. The question is what msgpack should do with multi-dimensional memoryviews:
Treat it as a flat memoryview and pack and unpack it like that? This would mean that the unpacked memoryview is slightly different: same data, but different shape (flattened). I think that's the current behavior (at least for @methane who currently cannot reproduce the issue).
Refuse packing a multi-dimensional memoryview? Ok, but impractical, IMHO.
Activity
jfolz commentedon Jan 17, 2023
The packer actually requests a buffer object with the
PyBUF_SIMPLEflag, so there should be no shape, strides, or suboffsets. See here. The API also says that "Without stride information, the buffer must be C-contiguous."As long as I'm not misunderstanding something here Numpy should either return a 1D C-contiguous buffer (possibly creating a copy of non-contiguous arrays first), or return an error if it can't do that. So maybe Numpy is at fault here?
pohlt commentedon Jan 17, 2023
@jfolz, I don't fully understand your comment.
When packing a NumPy array (in my code not shown here), I explicilty ask for the memoryview using
ndarray.data, so I don't blame NumPy for returning a multi-dimensional memoryview.But even without NumPy: msgpack packs multi-dimensional memoryviews incorrectly as shown in the example, wouldn't you agree?
methane commentedon Jan 18, 2023
I can not reproduce.
methane commentedon Jan 18, 2023
I tested Python 3.10 and 3.11 but output is same.
jfolz commentedon Jan 18, 2023
You're right, I misunderstood what you were trying to do. Numpy is not to blame, as it's job is done after you get the
memoryviewobject out of it. My point still stands though, if thememoryviewcannot produce a nice and simple contiguous buffer object it should return an error and packing should in turn fail.pohlt commentedon Jan 18, 2023
@methane , that's weird. I reproduced your steps above in a fresh venv with only msgpack installed:
As you can see, the bin array has a correct size of 6 in the first case and an incorrect size 3 (the size of the first dimension) in the second case.
pohlt commentedon Jan 18, 2023
@jfolz The memoryview is contiguous, but it's multi-dimensional. The question is what msgpack should do with multi-dimensional memoryviews:
Could be another configuration parameter.
methane commentedon Jan 18, 2023
Now I got it. Fallback module packs it wrong.
Add test for #526
Add test for #526
fallback: Fix #526
methane commentedon Jan 18, 2023
I released 1.0.5rc1. Please try it.
pohlt commentedon Jan 18, 2023
Issue is fixed with 1.0.5rc1. Thanks a lot!
pohlt commentedon Jan 19, 2023
But since you now provide wheels for 3.11, I'm no longer using the fallback code, I guess.
fallback: Fix packing multidim memoryview (msgpack#527)