False positives for warning about writing to broadcast array in cython code

In astropy, we're getting loads of DeprecationWarning about possibly writing to a broadcast array from cythonized code. They all seem false positives, and may rather reflect something in Cython which I do not understand. But it can be reproduced with this trivial code below. For our particular case, I think this could be solved if one ensured the warning flag was only set if there was actual memory overlap inside the broadcast array.

Sample file broadcast.pyx

import warnings
warnings.filterwarnings('error')

import numpy as np
cimport numpy as np


t, _ = np.broadcast_arrays(np.ones(10), np.array(1.))

# Real code also has the following to ensure that broadcast arrays are copied.
# But that does not make a difference for the error, since the strides of `t` are fine.
# t = np.asarray(t, order='C')

ctypedef np.float64_t DTYPE_t


cdef check(DTYPE_t[::1] t):
    pass


print(t.strides)
check(t)

Corresponding setup.py:

from distutils.core import setup
from Cython.Build import cythonize

setup(
    ext_modules = cythonize("broadcast.pyx")
)

Compile and run with:

python3 setup.py build_ext --inplace
python3 -c 'import broadcast'

What cython version?

For the astropy tests, 0.29.10, for my tests, 0.29.2.

I checked, and the code above still gives the deprecation warning for 0.29.12 (newest with pip)

Hmm, hard to track, but it looks like cython is setting the buffer to writable.

Or maybe just checking for writable.

__pyx_t_6 = __Pyx_PyObject_to_MemoryviewSlice_dc_nn___pyx_t_9broadcast_DTYPE_t(__pyx_t_2, PyBUF_WRITABLE); if (unlikely(!__pyx_t_6.memview)) __PYX_ERR(2, 22, __pyx_L1_error)

So maybe a Cython bug, but probably not one we can track down for 1.17.0.

Cython is requesting a writable buffer, and in this case I think NumPy should provide same. However, I wonder if cython handles read only arrays correctly?

This fixes the example program

t, _ = np.broadcast_arrays(np.ones(10), np.array(1.))
t.flags.writeable = 1

Indeed, when I explicitly set t.flags.writeable = False, I get

ValueError: buffer source array is read-only

so, you're right, cython expects its memoryview to be writeable.

Apparently, one should be able to avoid that in more recent cython (0.28) by explicitly declaring them const (https://github.com/cython/cython/pull/1869/files).

And this I can make to work, but only if I explicitly set my array as non-writeable (see below). Apparently, unless the array is explicitly set as non-writeable, cython will check it is writeable even if one is taking a readonly view. Which is, of course, a bit odd.

From the numpy view, I'm not sure what would be best to work-around this...

import warnings
warnings.filterwarnings('error')

import numpy as np
cimport numpy as np


t, _ = np.broadcast_arrays(np.ones(10), np.array(1.))

# Real code also has the following to ensure that broadcast arrays are copied.
# But that does not make a difference for the error, since the strides of `t` are fine.
# t = np.asarray(t, order='C')

ctypedef np.float64_t DTYPE_t

cdef const DTYPE_t[::1] tc
t.setflags(write=False)


cdef check(const DTYPE_t[::1] t):
    pass


print(t.strides)
check(t)

More

In [2]: t, _ = np.broadcast_arrays(np.ones(10), np.array(1.))                   

In [3]: t.flags                                                                 
Out[3]: 
  C_CONTIGUOUS : True
  F_CONTIGUOUS : True
  OWNDATA : False
  WRITEABLE : True  (with WARN_ON_WRITE=True)
  ALIGNED : True
  WRITEBACKIFCOPY : False
  UPDATEIFCOPY : False

In [4]: t.flags.writeable = 1                                                   

In [5]: t.flags                                                                 
Out[5]: 
  C_CONTIGUOUS : True
  F_CONTIGUOUS : True
  OWNDATA : False
  WRITEABLE : True
  ALIGNED : True
  WRITEBACKIFCOPY : False
  UPDATEIFCOPY : False

Making a copy also fixes the problem as claimed.

In [5]: t, _ = np.broadcast_arrays(np.ones(10), np.array(1.))                   

In [6]: t.copy().flags                                                          
Out[6]: 
  C_CONTIGUOUS : True
  F_CONTIGUOUS : True
  OWNDATA : True
  WRITEABLE : True
  ALIGNED : True
  WRITEBACKIFCOPY : False
  UPDATEIFCOPY : False

Ah, the buffer protocol cannot indicate readonly specifically (just request writeable). So I suppose we could simply return a readonly buffer right away when the user does not ask for writable specifically. That jumps the deprecation a little, but is likely fine. The only thing I find a bit annoying about it, is that without the deprecation, it might be a bit hard to see why an array is readonly, but I suppose that cannot be helped.

So yeah, jumping the deprecation might be the best way and we can do that specific to the buffer protocol:

Patch to jump deprecation for the buffer protocol:

diff --git a/numpy/core/src/multiarray/buffer.c b/numpy/core/src/multiarray/buffer.c
index d8ad80266..4041b055f 100644
--- a/numpy/core/src/multiarray/buffer.c
+++ b/numpy/core/src/multiarray/buffer.c
@@ -771,17 +771,6 @@ array_getbuffer(PyObject *obj, Py_buffer *view, int flags)
             goto fail;
         }
     }
-    /*
-     * If a read-only buffer is requested on a read-write array, we return a
-     * read-write buffer, which is dubious behavior. But that's why this call
-     * is guarded by PyArray_ISWRITEABLE rather than (flags &
-     * PyBUF_WRITEABLE).
-     */
-    if (PyArray_ISWRITEABLE(self)) {
-        if (array_might_be_written(self) < 0) {
-            goto fail;
-        }
-    }
 
     if (view == NULL) {
         PyErr_SetString(PyExc_ValueError, "NULL view in getbuffer");
@@ -797,7 +786,14 @@ array_getbuffer(PyObject *obj, Py_buffer *view, int flags)
     view->buf = PyArray_DATA(self);
     view->suboffsets = NULL;
     view->itemsize = PyArray_ITEMSIZE(self);
-    view->readonly = !PyArray_ISWRITEABLE(self);
+    /*
+     * Set a requested buffer to readonly also if the array will be readonly
+     * after a deprecation. This jumps the deprecation, but avoiding the
+     * warning is not convenient here and a warning is given if a writeable
+     * buffer is requested.
+     */
+    view->readonly = (!PyArray_ISWRITEABLE(self) ||
+                      PyArray_CHKFLAGS(self, NPY_ARRAY_WARN_ON_WRITE));
     view->internal = NULL;
     view->len = PyArray_NBYTES(self);
     if ((flags & PyBUF_FORMAT) == PyBUF_FORMAT) {

I am not sure if that diff will force you to use the const specifiers, or if cython is doesn't request writeable unless it knows it will be written to or so.

@seberg

It's possible we should just rollback the warning on accessing the writeable attribute, it doesn't provide a huge amount of value, and sounds like with cython it comes with a high cost.

@seberg: IMO forcing cython to use const for read-only arrays would be a really good idea, if there's a way to make that happen

My patch works for me anyway (in the sense that either const or setting arr.flags.writeable = False is sufficient). (although it segfaulted with const in cython cell magic, but I blame a cython/inline issue)

Cython does indeed force the const in this case (which also means it asks for writeable in all other cases). So: From the point of view of cython typed memoryviews (with new enough cython). My patch seems to actually achieve the exact deprecation we want. For other buffer users it could be a bit rougher in principle.

Thanks for looking into this! I like the possibility of just using const in cython.

I wonder if the we should add that const information into the warning. (Or maybe add it to the warning when the warning is issued from inside the buffer protocol). The only thing my code changes is that if you do not ask for a writeable buffer, we switch to a readonly buffer directly. Which is a change in behaviour, but maybe not bad enough to worry about it.

I can create a PR soon (although a few other things I would like to tick off the not-done list first). I suppose the next RC would tell us how things go.

EDIT: Of course if someone beats me to it, I am happy as well :)

DEP: Speed up WarnOnWrite deprecation in buffer interface

@seberg

@seberg - thanks for the fix - it works well, and getting us to declare arrays we don't write to as const is good!

DEP: Speed up WarnOnWrite deprecation in buffer interface

Navigation Menu

Repository navigation

False positives for warning about writing to broadcast array in cython code #13929

6 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

False positives for warning about writing to broadcast array in cython code #13929

Description

Activity

charris commented on Jul 7, 2019

mhvk commented on Jul 7, 2019

charris commented on Jul 7, 2019

charris commented on Jul 7, 2019

charris commented on Jul 7, 2019

charris commented on Jul 7, 2019

charris commented on Jul 7, 2019

mhvk commented on Jul 7, 2019

charris commented on Jul 7, 2019

charris commented on Jul 7, 2019

6 remaining items

seberg commented on Jul 7, 2019

seberg commented on Jul 7, 2019

eric-wieser commented on Jul 7, 2019

seberg commented on Jul 8, 2019

mhvk commented on Jul 8, 2019

seberg commented on Jul 8, 2019

mhvk commented on Jul 16, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions