Repository: cupy/cupy
Pull request: Avoid looking up null pointers' attributes
Follow-up of #2611. Close #2789.

This PR fixes the consumer side (while #2611 fixed the producer side) for handling zero-size arrays. 

cc: @jakirkham 

Problem statement:
`__cuda_array_interface__ ` with `NULL` pointer fails to convert to CuPy array
Recently ran into an issue with CuPy's `__cuda_array_interface__` conversion support where it fails if it holds a `NULL` pointer for the data. Here's an example using an existing CuPy array. Though this happens with any `__cuda_array_interface__` supporting object that holds a `NULL` pointer. 

* Conditions (you can just paste the output of `python -c 'import cupy; cupy.show_config()'`)
  - CuPy version: 6.6.0
  - OS/Platform: Ubuntu 18.04, x86_64
  - CUDA version: 10.0
  - cuDNN/NCCL version (if applicable): 7.6.4/2.4.6
* Code to reproduce

```python
import cupy                                                             

a = cupy.empty((0,), dtype="|u1")                                       
a2 = cupy.core.core._convert_object_with_cuda_array_interface(a)
```

* Error messages, stack traces, or logs

```python
CUDARuntimeError                          Traceback (most recent call last)
<ipython-input-3-cf9277f8c1fb> in <module>
----> 1 a2 = cupy.core.core._convert_object_with_cuda_array_interface(a)

cupy/core/core.pyx in cupy.core.core._convert_object_with_cuda_array_interface()

cupy/core/core.pyx in cupy.core.core._convert_object_with_cuda_array_interface()

cupy/cuda/memory.pyx in cupy.cuda.memory.UnownedMemory.__init__()

cupy/cuda/runtime.pyx in cupy.cuda.runtime.pointerGetAttributes()

cupy/cuda/runtime.pyx in cupy.cuda.runtime.check_status()

CUDARuntimeError: cudaErrorInvalidValue: invalid argument
```

Hints from issue discussion:
Thanks, @jakirkham. This was my oversight when fixing it in #2611. As we discussed elsewhere, in this case `ptr = 0` is used as a flag to prevent from looking up the pointer attribute. 

Are you able to create a PR to fix this? It should just be a simple one liner `if ptr > 0: ...` added to `cupy.cuda.memory.UnownedMemory`. I can also create one later today (no access to my computer now). Thanks.
No worries. There were a lot of good fixes there too. Thanks for all of those. 🙂

Yeah I think it should be straightforward. It might be a little bit until I get to it (so if someone beats me to it no worries), but more than happy to submit a fix. 😄

Mostly wanted to make sure we were aware of the issue and were tracking it somewhere. 😉

Commit message: apply https://github.com/cupy/cupy/pull/2802#issuecomment-565037306

Patch to review:
diff --git a/cupy/core/core.pyx b/cupy/core/core.pyx
index 7eb451ec4f5..cdfa5338898 100644
--- a/cupy/core/core.pyx
+++ b/cupy/core/core.pyx
@@ -2776,13 +2776,16 @@ not_equal = create_comparison(
 
 cpdef ndarray _convert_object_with_cuda_array_interface(a):
     cdef Py_ssize_t sh, st
-    desc = a.__cuda_array_interface__
-    shape = desc['shape']
+    cdef dict desc = a.__cuda_array_interface__
+    cdef tuple shape = desc['shape']
+    cdef int dev_id = -1
+    cdef int nbytes
+
+    ptr = desc['data'][0]
     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.')
+    mask = desc.get('mask')
+    if mask is not None:
+        raise ValueError('CuPy currently does not support masked arrays.')
     strides = desc.get('strides')
     if strides is not None:
         nbytes = 0
@@ -2790,7 +2793,11 @@ cpdef ndarray _convert_object_with_cuda_array_interface(a):
             nbytes = max(nbytes, abs(sh * st))
     else:
         nbytes = internal.prod_sequence(shape) * dtype.itemsize
-    mem = memory_module.UnownedMemory(desc['data'][0], nbytes, a)
+    # the v2 protocol sets ptr=0 for 0-size arrays, so we can't look up
+    # the pointer attributes and must use the current device
+    if nbytes == 0:
+        dev_id = device.get_device_id()
+    mem = memory_module.UnownedMemory(ptr, nbytes, a, dev_id)
     memptr = memory.MemoryPointer(mem, 0)
     return ndarray(shape, dtype, memptr, strides)
 
diff --git a/cupy/cuda/memory.pyx b/cupy/cuda/memory.pyx
index 88f9f615a19..fd97d2dfb14 100644
--- a/cupy/cuda/memory.pyx
+++ b/cupy/cuda/memory.pyx
@@ -122,9 +122,14 @@ cdef class UnownedMemory(BaseMemory):
     def __init__(self, intptr_t ptr, size_t size, object owner,
                  int device_id=-1):
         cdef runtime.PointerAttributes ptr_attrs
+        # ptr=0 for 0-size arrays from __cuda_array_interface__ v2:
+        # we need a valid device id as null ptr can't be looked up
         if device_id < 0:
-            ptr_attrs = runtime.pointerGetAttributes(ptr)
-            device_id = ptr_attrs.device
+            if ptr > 0:
+                ptr_attrs = runtime.pointerGetAttributes(ptr)
+                device_id = ptr_attrs.device
+            else:
+                raise RuntimeError
         self.size = size
         self.device_id = device_id
         self.ptr = ptr
diff --git a/tests/cupy_tests/creation_tests/test_from_data.py b/tests/cupy_tests/creation_tests/test_from_data.py
index 3109317234d..ce0c5d215d9 100644
--- a/tests/cupy_tests/creation_tests/test_from_data.py
+++ b/tests/cupy_tests/creation_tests/test_from_data.py
@@ -489,6 +489,16 @@ def test_with_strides(self, dtype):
         assert a.strides == b.strides
         assert a.nbytes == b.data.mem.size
 
+    @testing.for_all_dtypes()
+    def test_with_zero_size_array(self, dtype):
+        a = testing.shaped_arange((0,), cupy, dtype)
+        b = cupy.asarray(
+            DummyObjectWithCudaArrayInterface(a, self.ver, self.strides))
+        assert a.strides == b.strides
+        assert a.nbytes == b.data.mem.size
+        assert a.data.ptr == 0
+        assert a.size == 0
+
 
 @testing.gpu
 @testing.parameterize(*testing.product({


Code review comments:
diff_hunk: @@ -122,9 +122,14 @@ cdef class UnownedMemory(BaseMemory):
     def __init__(self, intptr_t ptr, size_t size, object owner,
                  int device_id=-1):
         cdef runtime.PointerAttributes ptr_attrs
+        # ptr=0 for 0-size arrays from __cuda_array_interface__ v2:
+        # we need a valid device id as null ptr can't be looked up
         if device_id < 0:
-            ptr_attrs = runtime.pointerGetAttributes(ptr)
-            device_id = ptr_attrs.device
+            if ptr > 0:
+                ptr_attrs = runtime.pointerGetAttributes(ptr)
+                device_id = ptr_attrs.device
+            else:
+                raise RuntimeError
path: cupy/cuda/memory.pyx
text: @user1:
Let's add a message.

```suggestion
                raise RuntimeError('UnownedMemory requires explicit device ID for a null pointer.')
```

diff_hunk: @@ -122,9 +122,14 @@ cdef class UnownedMemory(BaseMemory):
     def __init__(self, intptr_t ptr, size_t size, object owner,
                  int device_id=-1):
         cdef runtime.PointerAttributes ptr_attrs
+        # ptr=0 for 0-size arrays from __cuda_array_interface__ v2:
+        # we need a valid device id as null ptr can't be looked up
         if device_id < 0:
-            ptr_attrs = runtime.pointerGetAttributes(ptr)
-            device_id = ptr_attrs.device
+            if ptr > 0:
path: cupy/cuda/memory.pyx
text: @user1:
Please use `==`. `ptr` can be negative.
Also, it's better to indent the code this way:

```py
            if ptr == 0:
                raise RuntimeError('UnownedMemory requires explicit device ID for a null pointer.')
            ptr_attrs = runtime.pointerGetAttributes(ptr)
            device_id = ptr_attrs.device
```

@author:
Good point, thanks.

diff_hunk: @@ -2776,21 +2776,28 @@ not_equal = create_comparison(
 
 cpdef ndarray _convert_object_with_cuda_array_interface(a):
     cdef Py_ssize_t sh, st
-    desc = a.__cuda_array_interface__
-    shape = desc['shape']
+    cdef dict desc = a.__cuda_array_interface__
path: cupy/core/core.pyx
text: @user1:
I think it's not safe to assume it's `dict`.
In Python you can define arbitrary type that behaves as a dictionary.

@author:
Before I proposed the v2 protocol, I investigated and ensured all current supporting libraries (see numba/numba#4448) implemented it as a simple Python dict, so that we could safely require it [to be a dict in v2](http://numba.pydata.org/numba-doc/latest/cuda/cuda_array_interface.html)
> The `__cuda_array_interface__` attribute returns a dictionary that ...

without any backward incompatibility. I believe this is a very safe and compliant change.

@user1:
Do you mean v2 already requires it to be a `dict`?
In the specification (https://numba.pydata.org/numba-doc/dev/cuda/cuda_array_interface.html), I can only find a statement that it returns a "dictionary" which is broader than `dict`.

In any case, this is the receiving side. It may be safe not to unnecessarily assume it (even if the protocol specifies it).

@author:
> I can only find a statement that it returns a "dictionary" which is broader than dict.

That is not true. Dictionaries in Python almost always refer to the built-in `dict`. There is no ambiguity for alternative interpretation. Also, the wording and the implementation follow that of `__array_interface__` in NumPy.

I revised the wording in v2 in the hope that it can be used as a small optimization in Cython (both CuPy and mpi4py have the receiver code written in Cython), so I would very much like to keep the change as is.

@user1:
>Dictionaries in Python almost always refer to the built-in `dict`.

I can't find any (quasi-)official statement about that.
[The official glossary](https://docs.python.org/3/glossary.html#term-dictionary) describes "dictionary" as "An associative array, where arbitrary keys are mapped to values.". It's still ambiguous, I think.

Python is a duck-typing language. Without explicit wording, I wouldn't assume "dictionary" refers to `dict`.

As for performance, I did some measurement using the following code. There was no noticeable difference whether or not `d` was typed (actually it seems typed code was a bit slower, perhaps because of extra type checking).

```py
# distutils: language = c++
cpdef int func(dict d):
    cdef int s = 0
    cdef int t = 0
    for i in range(100):
        t = d['key']
        s += t
    return s
```

Also, for the purpose of optimization, an implementation of `__cuda_array_interface__` could return a specialized and faster variant of dictionary with limited set of keys, for example. Explicit typing could potentially cause problems in such a case.


@author:
> I can't find any (quasi-)official statement about that.
> [The official glossary](https://docs.python.org/3/glossary.html#term-dictionary) describes "dictionary" as "An associative array, where arbitrary keys are mapped to values.". It's still ambiguous, I think.
> 
> Python is a duck-typing language. Without explicit wording, I wouldn't assume "dictionary" refers to `dict`.

> Also, for the purpose of optimization, an implementation of `__cuda_array_interface__` could return a specialized and faster variant of dictionary with limited set of keys, for example. Explicit typing could potentially cause problems in such a case.

I disagree. The terms "dictionary" and `dict` are always used interchangeably, see, e.g., any place in the official Python documentation that refers to dictionaries:
https://docs.python.org/3.7/library/stdtypes.html?highlight=dictionary#mapping-types-dict

In practice, having any alternative dictionary implementation for `__cuda_array_interface__` can only increase the complexity of code dependency and the risk of not being interoperable. It makes little sense to be overcautious on this.


 
> As for performance, I did some measurement using the following code. There was no noticeable difference whether or not `d` was typed (actually it seems typed code was a bit slower, perhaps because of extra type checking).
> 
> ```python
> # distutils: language = c++
> cpdef int func(dict d):
>     cdef int s = 0
>     cdef int t = 0
>     for i in range(100):
>         t = d['key']
>         s += t
>     return s
> ```
> 

This is what I got:
```python
In [1]: %load_ext Cython

In [2]: class Array:
   ...:     def __init__(self):
   ...:         self.__interface__ = {'shape': (1, 2, 3), 'ptr':18}
   ...:         

In [3]: arr = Array()

In [4]: %%cython
   ...: cpdef get_face(a):
   ...:     x = a.__interface__
   ...:     shape = x['shape']
   ...:     ptr = x['ptr']
   ...:     return ptr
   ...: 

In [5]: %%cython
    ...: cpdef get_face_typed(a):
    ...:     cdef dict x = a.__interface__
    ...:     cdef tuple shape = x['shape']
    ...:     ptr = x['ptr']
    ...:     return ptr
    ...: 

In [6]: %timeit get_face(arr)
107 ns ± 0.312 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

In [7]: %timeit get_face_typed(arr)
101 ns ± 0.212 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
```

@author:
FYI, see the original intent of the v2 protocol for `__cuda_array_interface__` being a Python `dict`: https://github.com/numba/numba/pull/4609#issue-321035175.

@author:
@user1, I thought you replied something but I can't find it now. Anyway, I am surprised about the pushback here, to end this and land the fix asap I changed the type in 230e83a. 

@user1:
Thank you for understanding.
This is what I posted (modified a bit):

>>I disagree. The terms "dictionary" and `dict` are always used interchangeably, see, e.g., any place in the official Python documentation that refers to dictionaries:
>https://docs.python.org/3.7/library/stdtypes.html?highlight=dictionary#mapping-types-dict
>
>Thank you for the pointer.
>I see, maybe you're correct in that the official documentation refers to `dict` by the word "dictionary".
>Maybe officially there's no ambiguity, but in reality, I think there is.
>Even I, who have been using Python for 8~9 years, mistook the word, because the primary mode of the language is duck-typing and this philosophy is everywhere.
>
>Anyway, this is about the expression in the protocol specification, and I think not essential in this discussion. As I said above, even if the protocol explicitly specifies the type to be `dict`, I don't think the receiver should assume that.
>
>>This is what I got:
>
>I see. it might be dependent on environment. But anyway I think a few nanosecond is not so important.
> 
>
>>In practice, having any alternative dictionary implementation for `__cuda_array_interface__` can only increase the complexity of code dependency and the risk of not being interoperable. It makes little sense to be overcautious on this.
>
>I agree it could increase the risk of not being interoperable (not sure about code dependency). 
>But it's about the producer side. Receiver side would increase the risk by making an assumption, instead.


@author:
Thanks, @user1. I have only one comment:

> > In practice, having any alternative dictionary implementation for `__cuda_array_interface__` can only increase the complexity of code dependency and the risk of not being interoperable. It makes little sense to be overcautious on this.
> 
> I agree it could increase the risk of not being interoperable (not sure about code dependency).
> But it's about the producer side. Receiver side would increase the risk by making an assumption, instead.

I still think it is a right thing to do on the receiver side to reject any non-standard `dict` alternatives. First, this is specified in the protocol as you also agreed. Second, the receiver does not (and should not) recognize the type of any non-standard dictionary, otherwise the receiving library would have to depend on the library that provides the customary dictionary implementation. 

@user2:
Have submitted PR ( https://github.com/numba/numba/pull/5049 ) in an attempt to clarify this in the spec. Feedback welcome 🙂