Repository: cupy/cupy
Pull request: Update `__cuda_array_interface__` to protocol version 2
Closes #2462. 

This PR is a parallel effort of the upstream PR numba/numba#4609. Although this PR is ready, it is marked as WIP and please do not review or merge until that one is approved. I will ensure any changes there to be reflected here as well. 

In the proposed v2 protocol, the `strides` field is made mandatory, and `strides=None` can be used to indicate C contiguity. 

This PR explicitly disables masked arrays (so does Numba), which was added to the v1 protocol (numba/numba#4199). When #2225 is resolved, the checks and guards implemented here should be lifted. 

To be discussed with CuPy core devs:
- ~~Do we want to enforce a strict compliance with the protocol, say, in a future release? For example,  we can check `__cuda_array_interface__['version'] == 2`. NumPy does that for `__array_interface__`.~~

cc: @pentschev @stuartarchibald @dalcinl @jakirkham 

Problem statement:
__cuda_array_interface__ doesn't include always strides
The current `__cuda_array_interface__` implementation doesn't include strides for `c_contiguous`. Even though it's obvious what the strides are in that case, it would be useful to have them there, in particular since there's no information on contiguity in that interface.

Would there be any objections to including it even if `c_contiguous`? Note that Numba includes it even in that case.

Edit: changed the description after realizing it's only not included when `c_contiguous`.

cc @jakirkham

Hints from issue discussion:
AFAIK, it's not included because `strides` is, as you said, obvious when `c_contiguous`. Plus in this situation `strides` is explicitly marked as "optional" in the Numba's doc on `__cuda_array_interface__`. IMHO CuPy is compliant while Numba isn't (numba/numba#4175 is a "counter-issue" of this one). Can't Dask assume the C strides when it's not given? I feel that if this is really needed, we should first ask the Numba folks to mark it mandatory in the doc (cc: @seibert).

FYI, this is how we handled it in mpi4py: https://github.com/mpi4py/mpi4py/blob/e747ddd091eab6b7710edcba1c445824854a70f3/src/mpi4py/MPI/asgpubuf.pxi#L100-L107
> in particular since there's no information on contiguity in that interface.

I second your observation. My experience when writing tests for `__cuda_array_interface__` is that we really need a flag to tell whether it's C-contiguous or not (evidence for my struggling 😆: https://github.com/numba/numba/issues/4175#issuecomment-501423139). Always explicitly giving `strides` makes it hard to tell the contiguity, and in mpi4py we ended up skipping testing Numba GPU arrays for this (and other) reason. 

So, in any case one of the libraries (CuPy/Numba) should change, and the interface standard must be  clearer.
Thanks for pointing me to those issues @leofang, I commented in https://github.com/numba/numba/issues/4175 and agree that as of today we shouldn't change anything in the CuPy code, but first wait for a resolution on that issue.

I would suggest keeping this open until then.
Thanks for support @pentschev! I am glad I am no longer the only one annoyed by the `strides` issue 🤣 I agree this issue should remain open, in case a change needs to be made on the CuPy side.

Commit message: update cuda array interface to v2

Patch to review:
diff --git a/cupy/core/core.pyx b/cupy/core/core.pyx
index 143b2ccb238..9b6cf5f6dc3 100644
--- a/cupy/core/core.pyx
+++ b/cupy/core/core.pyx
@@ -142,9 +142,11 @@ cdef class ndarray:
             'typestr': self.dtype.str,
             'descr': self.dtype.descr,
             'data': (self.data.ptr, False),
-            'version': 0,
+            'version': 2,
         }
-        if not self._c_contiguous:
+        if self._c_contiguous:
+            desc['strides'] = None
+        else:
             desc['strides'] = self.strides
 
         return desc
@@ -2743,14 +2745,23 @@ not_equal = create_comparison(
 cpdef ndarray _convert_object_with_cuda_array_interface(a):
     cdef Py_ssize_t sh, st
     desc = a.__cuda_array_interface__
+    # TODO(leofang): enforce compliance to the latest protocol?
     shape = desc['shape']
     dtype = numpy.dtype(desc['typestr'])
+    if 'mask' in desc:
+        mask = desc['mask']
+        if mask is not None:
+            raise ValueError("CuPy currently does not support masked arrays.")
+    # TODO(leofang): for ver.2, 'strides' is always available, so don't check.
     if 'strides' in desc:
         strides = desc['strides']
-        nbytes = 0
-        for sh, st in zip(shape, strides):
-            nbytes = max(nbytes, abs(sh * st))
-    else:
+        if strides is not None:
+            nbytes = 0
+            for sh, st in zip(shape, strides):
+                nbytes = max(nbytes, abs(sh * st))
+        else:
+            nbytes = internal.prod(shape) * dtype.itemsize
+    else:  # for backward compatibility with ver.0 & ver.1
         strides = None
         nbytes = internal.prod(shape) * dtype.itemsize
     mem = memory_module.UnownedMemory(desc['data'][0], nbytes, a)
diff --git a/tests/cupy_tests/core_tests/test_ndarray.py b/tests/cupy_tests/core_tests/test_ndarray.py
index 83f17743583..7821f2d4fe3 100644
--- a/tests/cupy_tests/core_tests/test_ndarray.py
+++ b/tests/cupy_tests/core_tests/test_ndarray.py
@@ -198,15 +198,17 @@ def test_cuda_array_interface(self):
         arr = cupy.zeros(shape=(2, 3), dtype=cupy.float64)
         iface = arr.__cuda_array_interface__
         assert (set(iface.keys()) ==
-                set(['shape', 'typestr', 'data', 'version', 'descr']))
+                set(['shape', 'typestr', 'data', 'version', 'descr',
+                     'strides']))
         assert iface['shape'] == (2, 3)
         assert iface['typestr'] == '<f8'
         assert isinstance(iface['data'], tuple)
         assert len(iface['data']) == 2
         assert iface['data'][0] == arr.data.ptr
         assert not iface['data'][1]
-        assert iface['version'] == 0
+        assert iface['version'] == 2
         assert iface['descr'] == [('', '<f8')]
+        assert iface['strides'] is None
 
     def test_cuda_array_interface_view(self):
         arr = cupy.zeros(shape=(10, 20), dtype=cupy.float64)
@@ -221,7 +223,7 @@ def test_cuda_array_interface_view(self):
         assert len(iface['data']) == 2
         assert iface['data'][0] == arr.data.ptr
         assert not iface['data'][1]
-        assert iface['version'] == 0
+        assert iface['version'] == 2
         assert iface['strides'] == (320, 40)
         assert iface['descr'] == [('', '<f8')]
 
diff --git a/tests/cupy_tests/core_tests/test_ndarray_cuda_array_interface.py b/tests/cupy_tests/core_tests/test_ndarray_cuda_array_interface.py
index e4d377d4c33..d36091e5b55 100644
--- a/tests/cupy_tests/core_tests/test_ndarray_cuda_array_interface.py
+++ b/tests/cupy_tests/core_tests/test_ndarray_cuda_array_interface.py
@@ -9,18 +9,21 @@ class DummyObjectWithCudaArrayInterface(object):
 
     def __init__(self, a):
         self.a = a
+        self._desc = None
 
     @property
     def __cuda_array_interface__(self):
-        desc = {
-            'shape': self.a.shape,
-            'strides': self.a.strides,
-            'typestr': self.a.dtype.str,
-            'descr': self.a.dtype.descr,
-            'data': (self.a.data.ptr, False),
-            'version': 0,
-        }
-        return desc
+        if self._desc is None:
+            desc = {
+                'shape': self.a.shape,
+                'strides': self.a.strides,
+                'typestr': self.a.dtype.str,
+                'descr': self.a.dtype.descr,
+                'data': (self.a.data.ptr, False),
+                'version': 2,
+            }
+            self._desc = desc
+        return self._desc
 
 
 @testing.gpu
@@ -187,12 +190,9 @@ def test_value_type(self, dtype, order):
         typestr = y.__cuda_array_interface__['typestr']
         ptr, readonly = y.__cuda_array_interface__['data']
         version = y.__cuda_array_interface__['version']
+        strides = y.__cuda_array_interface__['strides']
 
         # optional entries
-        if 'strides' in y.__cuda_array_interface__:
-            strides = y.__cuda_array_interface__['strides']
-        else:
-            strides = None
         if 'descr' in y.__cuda_array_interface__:
             descr = y.__cuda_array_interface__['descr']
         else:
@@ -203,7 +203,7 @@ def test_value_type(self, dtype, order):
         assert isinstance(typestr, str)
         assert isinstance(ptr, int)
         assert isinstance(readonly, bool)
-        assert version == 0  # update this when the standard is updated!
+        assert version == 2  # update this when the standard is updated!
         assert (strides is None) or isinstance(strides, tuple)
         assert (descr is None) or isinstance(descr, list)
         if isinstance(descr, list):
diff --git a/tests/cupy_tests/creation_tests/test_from_data.py b/tests/cupy_tests/creation_tests/test_from_data.py
index ddb33548a58..1b6110d3087 100644
--- a/tests/cupy_tests/creation_tests/test_from_data.py
+++ b/tests/cupy_tests/creation_tests/test_from_data.py
@@ -1,5 +1,7 @@
 import unittest
 
+import pytest
+
 import cupy
 from cupy import cuda
 from cupy import testing
@@ -396,12 +398,22 @@ def test_asarray_cuda_array_interface_order(self, dtype):
         testing.assert_array_equal(a, b)
 
     @testing.for_all_dtypes()
-    def test_asarray_cuda_array_interface_with_strdies(self, dtype):
+    def test_asarray_cuda_array_interface_with_strides(self, dtype):
         a = testing.shaped_arange((2, 3, 4), cupy, dtype).T
-        b = cupy.asarray(DummyObjectWithCudaArrayInterface(a, True))
+        b = cupy.asarray(DummyObjectWithCudaArrayInterface(a))
         assert a.strides == b.strides
         assert a.nbytes == b.data.mem.size
 
+    # TODO(leofang): remove this test when masked array is supported
+    def test_asarray_cuda_array_interface_with_masked_array(self):
+        a = cupy.arange(10)
+        mask = cupy.zeros(10)
+        a = DummyObjectWithCudaArrayInterface(a)
+        a.set_mask(mask)
+        with pytest.raises(ValueError) as ex:
+            b = cupy.asarray(a)  # noqa
+        assert 'does not support' in str(ex.value)
+
     def test_ascontiguousarray_on_noncontiguous_array(self):
         a = testing.shaped_arange((2, 3, 4))
         b = a.transpose(2, 0, 1)
@@ -466,22 +478,28 @@ def test_asfortranarray_cuda_array_zero_dim_dtype(
 
 class DummyObjectWithCudaArrayInterface(object):
 
-    def __init__(self, a, has_strides=False):
+    def __init__(self, a):
         self.a = a
-        self.has_strides = has_strides
+        self._desc = None
 
     @property
     def __cuda_array_interface__(self):
-        desc = {
-            'shape': self.a.shape,
-            'typestr': self.a.dtype.str,
-            'descr': self.a.dtype.descr,
-            'data': (self.a.data.mem.ptr, False),
-            'version': 0,
-        }
-        if self.has_strides:
-            desc['strides'] = self.a.strides
-        return desc
+        if self._desc is None:
+            desc = {
+                'shape': self.a.shape,
+                'strides': self.a.strides,
+                'typestr': self.a.dtype.str,
+                'descr': self.a.dtype.descr,
+                'data': (self.a.data.ptr, False),
+                'version': 2,
+            }
+            self._desc = desc
+        return self._desc
+
+    def set_mask(self, mask):
+        if self._desc is None:
+            self.__cuda_array_interface__
+        self._desc['mask'] = mask
 
 
 @testing.parameterize(


Code review comments:
diff_hunk: @@ -2743,14 +2745,23 @@ not_equal = create_comparison(
 cpdef ndarray _convert_object_with_cuda_array_interface(a):
     cdef Py_ssize_t sh, st
     desc = a.__cuda_array_interface__
+    # TODO(leofang): enforce compliance to the latest protocol?
     shape = desc['shape']
     dtype = numpy.dtype(desc['typestr'])
+    if 'mask' in desc:
+        mask = desc['mask']
+        if mask is not None:
+            raise ValueError("CuPy currently does not support masked arrays.")
+    # TODO(leofang): for ver.2, 'strides' is always available, so don't check.
     if 'strides' in desc:
         strides = desc['strides']
-        nbytes = 0
-        for sh, st in zip(shape, strides):
-            nbytes = max(nbytes, abs(sh * st))
-    else:
+        if strides is not None:
path: cupy/core/core.pyx
text: @user1:
I think you can unindent this block by 1 level to eliminate the duplicate line `nbytes = internal.prod(shape) * dtype.itemsize`.

@author:
Good point. I worked in the midnight and couldn't think of a better way! Thanks.

diff_hunk: @@ -2743,14 +2745,23 @@ not_equal = create_comparison(
 cpdef ndarray _convert_object_with_cuda_array_interface(a):
     cdef Py_ssize_t sh, st
     desc = a.__cuda_array_interface__
+    # TODO(leofang): enforce compliance to the latest protocol?
path: cupy/core/core.pyx
text: @user1:
I don't think we should enforce the protocol at the receiving side?


@author:
OK I think I was wrong. NumPy didn't check the `version` field either: In `as_ctype()` they do:
https://github.com/numpy/numpy/blob/6bdbc1e2fd20df7272802df88f3790e5dd7ed057/numpy/ctypeslib.py#L532-L533
but in `PyArray_FromInterface()` (the actual workhorse) they don't:
https://github.com/numpy/numpy/blob/4d13d4cf8e8ab029f37e36753ca991c2d74c5cdb/numpy/core/src/multiarray/ctors.c#L2446

But, if we don't check the version, we need to be aware of backward-compatibility issues, which I think are handled correctly in the current PR.

diff_hunk: @@ -2743,14 +2745,23 @@ not_equal = create_comparison(
 cpdef ndarray _convert_object_with_cuda_array_interface(a):
     cdef Py_ssize_t sh, st
     desc = a.__cuda_array_interface__
+    # TODO(leofang): enforce compliance to the latest protocol?
     shape = desc['shape']
     dtype = numpy.dtype(desc['typestr'])
+    if 'mask' in desc:
+        mask = desc['mask']
+        if mask is not None:
+            raise ValueError("CuPy currently does not support masked arrays.")
path: cupy/core/core.pyx
text: @user1:
Please use single quotes for consistency.

@author:
```suggestion
            raise ValueError('CuPy currently does not support masked arrays.')
```