Typed memoryview doesn't work with read-only buffers

[kernc]

Typed memoryviews don't work with read-only buffers. A banal example:

cpdef getmax(double[:] x):
    cdef double max = -float('inf')
    for val in x:
        if val > max:
            max = val
    return max

Example session:

>>> import numpy as np
>>> values = np.random.random(8)
>>> values 
array([ 0.4574,  0.8468,  0.744 ,  0.3764,  0.7581,  0.8123,  0.8783,  0.9341])

>>> import getmax
>>> getmax.getmax(values)
0.9341213061235054

>>> values.setflags(write=False)
>>> assert values.flags.writeable == False
>>> getmax.getmax(values)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-8-669165de7389> in <module>()
----> 1 getmax.getmax(values)

/tmp/getmax.pyx in getmax.getmax (getmax.c:1617)()
----> 1 cpdef getmax(double[:] x):
      2     cdef:
      3         double max = -float('inf')
      4 
      5     for val in x:

/tmp/getmax.so in View.MemoryView.memoryview_cwrapper (getmax.c:7403)()
/tmp/getmax.so in View.MemoryView.memoryview.__cinit__ (getmax.c:3678)()

ValueError: buffer source array is read-only

There is no evident reason why above simple function wouldn't work with a read-only buffer. About two years ago, this issue was raised on the mailing list along with a RFC patch:
https://mail.python.org/pipermail/cython-devel/2015-February/004316.html
Some discussion about a possible use for a const keyword:
https://groups.google.com/forum/#!topic/cython-users/32CMgaLrNhc

[rainwoodman]

Hi, I also ran into this issue today.

[rainwoodman]

I really liked the const keyword approach. What are the show stoppers?

[rainwoodman]

Currently, adding const to a fused type memory view gives a compiler crash in 0.25.2. Thus it appears to need some work on the parser to get this working.

Is a pull request welcomed? @robertwb @scoder

-----------------------------------------------------------
...
        self.painter.canvas_dtype_elsize = 0

        pmesh_painter_init(self.painter)
        self.support = self.painter.support

    def paint(self, numpy.ndarray real, const postype [:, :] pos, const masstype [:] mass, order,
   ^
------------------------------------------------------------

pmesh/_window.pyx:86:4: Compiler crash in AnalyseExpressionsTransform

File 'Nodes.py', line 430, in analyse_expressions: StatListNode(_window.pyx:1:0)
File 'Nodes.py', line 430, in analyse_expressions: StatListNode(_window.pyx:50:5)
File 'Nodes.py', line 4636, in analyse_expressions: CClassDefNode(_window.pyx:50:5,
    as_name = 'ResampleWindow',
    class_name = 'ResampleWindow',
    module_name = '',
    visibility = 'private')
File 'Nodes.py', line 430, in analyse_expressions: StatListNode(_window.pyx:51:4)
File 'Nodes.py', line 3029, in analyse_expressions: DefNode(_window.pyx:86:4,
    modifiers = [...]/0,
    name = 'paint',
    num_required_args = 8,
    py_wrapper_required = True,
    reqd_kw_flags_cname = '0',
    used = True)
File 'Nodes.py', line 3164, in prepare_argument_coercion: DefNodeWrapper(_window.pyx:86:4,
    args = [...]/8,
    modifiers = [...]/0,
    name = 'paint',
    num_required_args = 8)

Compiler crash traceback from this point on:
  File "/home/yfeng1/anaconda3/install/lib/python3.5/site-packages/Cython/Compiler/Nodes.py", line 3164, in prepare_argument_coercion
    if not arg.type.create_from_py_utility_code(env):
  File "/home/yfeng1/anaconda3/install/lib/python3.5/site-packages/Cython/Compiler/PyrexTypes.py", line 856, in create_from_py_utility_code
    suffix = self.specialization_suffix()
  File "/home/yfeng1/anaconda3/install/lib/python3.5/site-packages/Cython/Compiler/PyrexTypes.py", line 828, in specialization_suffix
    return "%s_%s" % (self.axes_to_name(), self.dtype_name)
AttributeError: 'MemoryViewSliceType' object has no attribute 'dtype_name'

[rainwoodman]

It appears this bug is keeping people away from the memoryview to use the older ndarray interface. for example, see this workaround in pandas pandas-dev/pandas#12013

[jnothman]

Yes we've also avoided typed memoryviews in python for this reason, land up using pointers, resulting in hard-to-find bugs that could have easily been caught with a boundscheck were we using memoryviews.

[jnothman]

I have been surprised that this has not been addressed since I knew of the RFC patch two years ago. But perhaps the Cython team isn't aware that it should be a priority because the non-support deters users from Cython best practices, particularly where read only memory maps are a key feature of parallelism as in @joblib

[ogrisel]

This also caused to use sub-optimal hacks in pandas and scikit-learn for instance:

• BUG make take_nd support readonly arrays pandas-dev/pandas#10070
• [MRG+1] Fix safe_indexing with read-only indices scikit-learn/scikit-learn#9507

Another related open bug in pandas:

• DataFrame.iloc raises ValueError: buffer source array is read-only when applied to read-only indices pandas-dev/pandas#17192

[scoder]

From a quick glance, the patch seems to lack support for nogil sections, at least, because it cannot just raise an exception in them. Also, doing a buffer type check at every access seems costly, so I'd rather do the check once, when acquiring the buffer. Thus the preference for an explicit syntax. That would also cover cases where the user knows that a write-buffer is needed due to C-level operations that Cython cannot see.

Given that there is general support for const now, it might be easier than before to implement this nicely. Which still means that someone has to do the work.

I understand that this is a problem for users, so we should try to find a solution that fixes the problem without waiting another two years.

[scoder]

I have an initial implementation up at https://github.com/scoder/cython/tree/readonly_buffers that automatically determines the need for a writable buffer, and otherwise sticks to a read-only one. It has some bugs and fails to detect some "read-only" vs. "needs writable" cases at compile time, but it mostly works.

It also implements explicit const memory views, e.g. cdef const int[:] a, which should always request a read-only view.

Feedback welcome.

[jnothman]

Compare at master...scoder:readonly_buffers

[scoder]

Now that an initial implementation is there, I'd be happy if someone else could take over. I don't think I'll keep working on this.

[gnbl]

Came here via #1682 because of "BufferError: Object is not writable." when trying to pass a bytes object. Would the workaround be to access this with a char*?

[jnothman]

yes, it's an unfortunate workaround: it works, but it leaves more room for bugs to crawl in

[lesteve]

@scoder I guess this one can be closed now that #1869 has been merged.

[scoder]

Right, thanks for the reminder.