bpo-33176 Add readonly mode to memoryviews by pablogsal · Pull Request #6314 · python/cpython

pablogsal · 2018-03-30T17:26:14Z

https://bugs.python.org/issue33176

pablogsal · 2018-03-30T17:27:17Z

This is an initial working implementation of the feature to discuss the design and other aspects.

pitrou

Thanks for doing this. I've posted a couple of comments below. You'll also need to update the memoryview.cast documentation in Doc/library/stdtypes.rst.

pitrou · 2018-03-31T16:54:11Z

The Py_buffer structure already a readonly field, so you don't need to add this. See https://docs.python.org/3/c-api/buffer.html#buffer-structure

pitrou · 2018-03-31T16:54:36Z

Even if it's not writable, you should still be able to cast to readonly=True.

Should I test both (writable and not writable) separately?

I did that adding a explicit test for readonly=False and readonly buffer. (Raises when casting a readonly buffer to a writable one).

pitrou · 2018-03-31T16:56:09Z

I wonder if it makes sense for the readonly argument to be keyword-only. @skrah may have an opinion on this.

Yes, intuitively I think it should be keyword-only.

pitrou · 2018-03-31T16:57:31Z

Instead of this, just use the p type format in PyArgs_ParseTuple above (see https://docs.python.org/3/c-api/arg.html#other-objects).

bedevere-bot · 2018-03-31T16:59:36Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

skrah · 2018-03-31T18:05:49Z

I agree with all of Antoine's comments. The flag should just set view.readonly, the only thing that should be disallowed is casting a readonly view to a writable one.

pablogsal · 2018-03-31T19:22:28Z

I have made the requested changes; please review again

bedevere-bot · 2018-03-31T19:22:31Z

Thanks for making the requested changes!

@pitrou: please review the changes made to this pull request.

pitrou

Thanks for the updates. I have some more minor comments (see below). However, I noticed a more fundamental limitation that I think would deserve fixing for this new feature to be really useful. I'll post separately about it.

pitrou · 2018-04-01T15:01:49Z

      .. versionadded:: 3.2

-   .. method:: cast(format[, shape])
+   .. method:: cast(format[, shape, readonly])


The signature here should be cast(format[, shape[, readonly]]) (note the additional square brackets around the "readonly" parameter, since it's optional).

What about something like

cast(format[, shape], *, readonly=<unchanged>)

That implies readonly is a keyword-only parameter, which matches your doc string and tests. Using the keyword notation also reduces the bracket nesting.

pitrou · 2018-04-01T15:02:04Z

-      and C-contiguous -> 1D.
+      and C-contiguous -> 1D. If *readonly* is ``True`` the result view will
+      not be writable. Triying to cast a view with a readonly underlying buffer to
+      a writable view will raise a :class:`ValueError`.


Judging by the code, it's TypeError not ValueError.

Also you'll need to add a versionchanged entry below, for the added parameter.

pitrou · 2018-04-01T15:02:57Z

        m = None
        self.assertEqual(sys.getrefcount(b), oldrefcount)

+    def test_setitem_readonly_and_writable_buffer(self):


Nit: I would call this test test_cast_writable_to_readonly, or something.

pitrou · 2018-04-01T15:03:37Z

+        b = self.rw_type(self._source)
+        oldrefcount = sys.getrefcount(b)
+        m = self._view(b)
+        readonly_m = m.cast("B", readonly=True)


I would test m.cast(m.format, readonly=True) instead, to make sure that only changing writability is supported.

Same for the other test below.

pitrou · 2018-04-01T15:04:03Z

+        readonly_m = m.cast("B", readonly=True)
+        def setitem(value):
+            readonly_m[0] = value
+        self.assertRaises(TypeError, setitem, 12)


Also check that readonly_m.readonly is True?

Same for the other test below.

pitrou · 2018-04-01T15:04:22Z

+        readonly_m = None
+        self.assertEqual(sys.getrefcount(b), oldrefcount)
+
+    def test_setitem_readonly_and_readonly_buffer(self):


Same as above: rename this test_cast_readonly_to_readonly?

pitrou · 2018-04-01T15:04:44Z

+
+    def test_setitem_readonly_and_readonly_buffer(self):
+        if not self.ro_type:
+            self.skipTest("no writable type to test")


You mean "no readonly type to test".

pitrou · 2018-04-01T15:06:50Z

+
+    if (self->view.readonly && !readonly){
+        PyErr_SetString(PyExc_TypeError,
+        "memoryview: cannot cast readonly buffer to a writable buffer.");


Nit: this line needs indenting.

bedevere-bot · 2018-04-01T15:10:39Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

pitrou · 2018-04-01T15:19:24Z

As I mentioned in my review above, there is a fundamental limitation that this PR is hitting, and it's that currently memoryview.cast() doesn't support same-shape casting for non-1D buffers. You would typically notice that if you have a view over a Numpy array.

To witness it without Numpy installed, CPython has a private type _testbuffer.ndarray that emulates a N-dimensional array. Below you can see the issue I'm talking about:

>>> a = _testbuffer.ndarray([1,2,3,4], format='B', shape=(2, 2))
>>> memoryview(a).tolist()
[[1, 2], [3, 4]]
>>> memoryview(a).cast('B').tolist()
[1, 2, 3, 4]
>>> memoryview(a).cast('B', shape=a.shape).tolist()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: memoryview: cast must be 1D -> ND or ND -> 1D

And the issue is similar if using non-byte formats and trying to cast to an identical format:

>>> a = _testbuffer.ndarray([1,2,3,4], format='i', shape=(2, 2))
>>> memoryview(a).cast('i').tolist()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: memoryview: cannot cast between two non-byte formats

For this PR to be truly useful, m.cast(m.format, m.shape, readonly=True) should be supported for any buffer kind. @skrah may have ideas on how to make it work.

pablogsal · 2018-04-01T15:21:29Z

@pitrou Should we close this PR then?

pitrou · 2018-04-01T15:24:10Z

I don't think so. I think that supporting the case where format and shape are unchanged should be reasonably doable (simply by comparing the values). If you feel you won't be motivated doing so, I (or Stefan) could take over.

pablogsal · 2018-04-01T15:27:22Z

@pitrou On the contrary, I'm more than happy to continue! Just wanted to understand what is the status of this PR. Thanks!

skrah · 2018-04-01T15:54:03Z

Perhaps we can make format not required. Then, if readonly is the only parameter, allow an early return with just the readonly field changed.

I'm not sure about our policy of making a required parameter not-required.

pitrou · 2018-04-01T15:56:37Z

I think it's ok to make format optional. There is still the issue that omitting shape converts the memoryview to 1-d, and that passing an unchanged shape for a n-d memoryview raises.

skrah · 2018-04-01T20:02:45Z

I think we can't change the behavior that to 1D is assumed when shape is missing.

Initially the point of cast() was to cast to bytes, then the reverse operation (casting back to the previous shape) was demanded. :-)

Allowing the same shape would be one way out, but I'm not complete happy with writing m.cast(shape=m.shape, readonly=True).

Perhaps m.cast(keep_shape=True, readonly=True) ?

keep_shape and shape would be mutually exclusive.

pitrou · 2018-04-01T20:52:52Z

While shape=m.shape isn't very satisfying, keep_shape=True doesn't look better to me, so I would vote for just keeping shape.

vadmium · 2018-04-02T00:33:36Z

      .. versionadded:: 3.2

-   .. method:: cast(format[, shape])
+   .. method:: cast(format[, shape, readonly])


What about something like

cast(format[, shape], *, readonly=<unchanged>)

That implies readonly is a keyword-only parameter, which matches your doc string and tests. Using the keyword notation also reduces the bracket nesting.

vadmium · 2018-04-02T00:34:54Z

      the buffer itself is not copied. Supported casts are 1D -> C-:term:`contiguous`
-      and C-contiguous -> 1D.
+      and C-contiguous -> 1D. If *readonly* is ``True`` the result view will
+      not be writable. Triying to cast a view with a readonly underlying buffer to


Spelling: Trying

vadmium · 2018-04-02T00:47:01Z

-      and C-contiguous -> 1D.
+      and C-contiguous -> 1D. If *readonly* is ``True`` the result view will
+      not be writable. Triying to cast a view with a readonly underlying buffer to
+      a writable view will raise a :class:`ValueError`.


What does readonly=False mean, and what is the default behaviour? How do I avoid casting a readonly view to writable? Will I have to add readonly=True to all my casts?

skrah · 2018-04-02T08:10:30Z

Okay, let's use shape=m.shape then.

Regarding Martin's points: Probably readonly should default to None, meaning unchanged.

While we're at it, shape should also default to None, which (roughly and imperfectly) corresponds to shape==NULL on the C level. At least there's the vague connotation of "casting to bytes" if shape isn't given.

pitrou · 2018-04-13T19:47:29Z

It strikes me that perhaps we're asking too much to the cast() method. Perhaps a separate to_readonly() method would be better? (from an implementation POV, at least, it would be much simpler :-))

skrah · 2018-04-13T19:53:55Z

I like that, probably converting to readonly will be used separately from casting most of the time anyway.

pitrou · 2018-04-13T20:32:26Z

See PR #6466 for the toreadonly() method.

pablogsal · 2018-04-14T16:51:28Z

@pitrou @skrah I agree that toreadonly() is more easy to implement and use. Should we close this one as superseded by #6466?

pitrou · 2018-04-14T17:02:36Z

@pablogsal, yes, I think so. Sorry for leading you into a dead end :-)

pablogsal · 2018-04-14T17:24:18Z

@pitrou No problem at all :)

the-knights-who-say-ni added the CLA signed label Mar 30, 2018

bedevere-bot added the awaiting review label Mar 30, 2018

pablogsal changed the title ~~bpo-44276 Add readonly mode to memoryviews~~ bpo-33176 Add readonly mode to memoryviews Mar 30, 2018

pitrou requested changes Mar 31, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Mar 31, 2018

pablogsal force-pushed the bpo33176 branch from 152a641 to f133362 Compare March 31, 2018 19:21

Add readonly mode to memoryviews

bf45691

pablogsal force-pushed the bpo33176 branch from f133362 to bf45691 Compare March 31, 2018 19:22

bedevere-bot added awaiting change review and removed awaiting changes labels Mar 31, 2018

Update documentation for memoryobject.cast

204f19a

pitrou requested changes Apr 1, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Apr 1, 2018

vadmium reviewed Apr 2, 2018

View reviewed changes

pablogsal closed this Apr 14, 2018

pablogsal deleted the bpo33176 branch April 14, 2018 17:24

Uh oh!

Conversation

pablogsal commented Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pablogsal commented Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal Mar 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Mar 31, 2018

Uh oh!

skrah commented Mar 31, 2018

Uh oh!

pablogsal commented Mar 31, 2018

Uh oh!

bedevere-bot commented Mar 31, 2018

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Apr 1, 2018

Uh oh!

pitrou commented Apr 1, 2018

Uh oh!

pablogsal commented Apr 1, 2018

Uh oh!

pitrou commented Apr 1, 2018

Uh oh!

pablogsal commented Apr 1, 2018

Uh oh!

skrah commented Apr 1, 2018

Uh oh!

pitrou commented Apr 1, 2018

Uh oh!

skrah commented Apr 1, 2018

Uh oh!

pitrou commented Apr 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal commented Mar 30, 2018 •

edited

Loading

pablogsal commented Mar 30, 2018 •

edited

Loading

pablogsal Mar 31, 2018 •

edited

Loading

pablogsal commented Apr 14, 2018 •

edited

Loading