WIP: Remove fragile use of `__array_interface__` in ctypeslib.as_array by eric-wieser · Pull Request #10970 · numpy/numpy

eric-wieser · 2018-04-25T05:06:33Z

At this point, we can rely on PEP3118 support within ctypes and numpy

Fixes gh-10968
Fixes gh-2671
Closes gh-6214 (commit is cherry-picked, thanks @tynn!)

eric-wieser · 2018-04-25T05:07:11Z

Monkey-patching ctypes was undocumented, and has confused me in the past!

eric-wieser · 2018-04-25T05:07:38Z

This is already encoded in the implementation of dtype.str.__get__

eric-wieser · 2018-04-25T05:38:51Z

While we're talking about PEP3118 and ctypes - could I ask someone who's worked with PEP3118 (@ahaldane?) to give a review to python/cpython#5561 and python/cpython#5576? It would be great if numpy would work with ctype structs as well, and those PRs have been sitting idle for a while

mattip · 2018-04-25T13:30:35Z

it would be safer to have shape=None and in the body of the code if shape is None: shape = ()
While in the current version shape is read-only, this might future-proof the function

Safer from what perspective?

whoops, shape is a zero-length-tuple so immutable. Never mind, sorry for the noise.

You've made me realize that defaulting to None and erroring is probably closer to the old behaviour

Updated to reject shape=None

eric-wieser · 2018-04-26T06:21:00Z

I suspect this fixes gh-6511 too

charris · 2018-05-03T19:36:20Z

I'm hoping that this also fixes the daily wheels builds that have been broken since #10968 went in.

________________ TestNewBufferProtocol.test_error_too_many_dims ________________
self = <numpy.core.tests.test_multiarray.TestNewBufferProtocol object at 0x7f02878c8210>
    def test_error_too_many_dims(self):
        # construct a memoryview with 33 dimensions
        m = memoryview(self.c_u8_33d())
        assert_equal(m.ndim, 33)
    
        assert_raises_regex(
            RuntimeError, "ndim",
>           np.array, m)
m          = <memory at 0x7f028afb6cc8>
self       = <numpy.core.tests.test_multiarray.TestNewBufferProtocol object at 0x7f02878c8210>
/venv/local/lib/python2.7/site-packages/numpy/core/tests/test_multiarray.py:6543: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
self = <numpy.testing._private.utils._Dummy testMethod=nop>
expected_exception = <type 'exceptions.RuntimeError'>, expected_regexp = 'ndim'
callable_obj = <built-in function array>, args = (<memory at 0x7f028afb6cc8>,)
kwargs = {}
context = <unittest.case._AssertRaisesContext object at 0x7f02878c82d0>
    def assertRaisesRegexp(self, expected_exception, expected_regexp,
                           callable_obj=None, *args, **kwargs):
        """Asserts that the message in a raised exception matches a regexp.
    
            Args:
                expected_exception: Exception class expected to be raised.
                expected_regexp: Regexp (re pattern object or string) expected
                        to be found in error message.
                callable_obj: Function to be called.
                args: Extra args.
                kwargs: Extra kwargs.
            """
        context = _AssertRaisesContext(expected_exception, self, expected_regexp)
        if callable_obj is None:
            return context
        with context:
>           callable_obj(*args, **kwargs)
E           ValueError: '(1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1)<B

eric-wieser · 2018-05-04T00:01:00Z

It looks like you've truncated that error message by at least one character (a '?). I'd hope the error message has more context than just the format itself.

I don't predict this patch will fix that. Strange that it works fine on travis, but not the wheel builds.

charris · 2018-05-04T00:24:04Z

Also works on Mac and Windows, only manylinux1 on Python 2.7 is a problem. See at https://travis-ci.org/MacPython/numpy-wheels/jobs/374373146.

Yeah, I kind of thought this PR wouldn't help, but wanted to get the ball rolling ...

eric-wieser · 2018-05-04T16:44:39Z

@charris: First step would be to not replace the error message coming from the internal PEP3118 parser with that less useful one.

eric-wieser · 2018-05-20T07:22:45Z

In light of #11115, I now worry that #10968 and this PR are relying on stable ctypes implementations that did not exist in non-recent versions of python 2.7. Do we have a stance on which python 2.7 versions are supported?

charris · 2018-05-20T15:13:50Z

Do we have a stance on which python 2.7 versions are supported?

No, but maybe we should. Python 2.7.7 came out about 4 years ago and these days it is pretty easy to upgrade. Be worth raising the issue on the list. There were several ctypes fixes in the recently released Python 2.7.15, so if we do pick a version, it should only be new enough that our tests pass and otherwise recommend that folks stay current.

charris · 2018-05-20T15:30:55Z

I think this would read better if the } was at the end of the previous line. This looks too much like C.

Maybe do the dictionary creation separately from the return statement.

I think we have different stylistic opinions here. How about I just put it on one line, since it fits nicely anyway?

charris · 2018-05-20T15:34:37Z

What is el_type? A complete docstring would help here.

charris · 2018-05-20T15:35:27Z

Maybe """Return ...

charris · 2018-05-20T15:43:00Z

I wonder if we still need this. Much of the code here is ancient and predates the inclusion of ctypes into the standard library in Python 2.5.

charris · 2018-05-20T15:45:09Z

A standard docstring would help here.

I'm going to argue this is out of scope for this patch

charris · 2018-05-20T15:46:38Z

Is using private class safe?

I'm not a huge fan of using attribute testing here, which will give a false-positive for np.recarray with the wrong dtype. I suppose I could switch it back to how the detection was done before, if you'd prefer that.

charris · 2018-05-20T15:49:10Z

See above, ctypes should be available in the Python versions we support.

Discussed below

Sign in to view

charris · 2018-05-20T15:51:13Z

at could be more descriptive.

Now pair_t

charris · 2018-05-20T15:52:36Z

Blank line after imports is standard.

charris · 2018-05-20T15:52:49Z

charris · 2018-05-20T15:53:19Z

charris · 2018-05-20T15:55:22Z

The presence of ctypes since Python 2.5 can be assumed. The correctness of ctypes is something else :)

eric-wieser · 2018-05-21T04:19:29Z

The presence of ctypes since Python 2.5 can be assumed.

I disagree - we've had bug reports about this in the past. It seems that python can be built without ctypes on some platforms?

eric-wieser · 2018-05-21T04:22:03Z

See this comment for a summary of how to end up without ctypes

charris · 2018-05-21T20:14:14Z

Hmm, it actually seems reasonable that ctypes may not be available on all platforms/environments. It certainly doesn't hurt to deal with that case.

eric-wieser · 2018-05-26T08:17:17Z

Some nits addressed. I think this does not change the patch-version-worry introduced by #10882

Everything behaves a lot better if we let the array constructor handle it, which will use the ctypes PEP3118 support. Bugs this fixes: * Stale state being attached to pointer objects (fixes numpygh-2671, closes numpygh-6214) * Weird failure modes on structured arrays (fixes-10978) * A regression in numpygh-10882 (fixes numpygh-10968)

charris · 2018-06-07T23:04:38Z

Thanks Eric.

tynn and others added 3 commits April 24, 2018 21:52

TST: Add tests for numpy.ctypeslib.as_array

1ec8f6d

MAINT: Use assert_equal

58e40ce

MAINT: Pull repeated decorators up to their containing class

4ec8930

eric-wieser added 00 - Bug 05 - Testing 03 - Maintenance labels Apr 25, 2018

eric-wieser commented Apr 25, 2018

View reviewed changes

eric-wieser mentioned this pull request Apr 25, 2018

BUG: Fix shape update in numpy.ctypeslib.as_array(pointer, shape) #6214

Closed

mattip reviewed Apr 25, 2018

View reviewed changes

eric-wieser changed the title ~~BUG: Remove fragile use of __array_interface__ in ctypeslib.as_array~~ BUG/WIP: Remove fragile use of __array_interface__ in ctypeslib.as_array Apr 25, 2018

eric-wieser force-pushed the cut-down-ctypeslib branch from 7038d28 to 2bb3d77 Compare April 26, 2018 05:59

eric-wieser changed the title ~~BUG/WIP: Remove fragile use of __array_interface__ in ctypeslib.as_array~~ WIP: Remove fragile use of __array_interface__ in ctypeslib.as_array Apr 26, 2018

This was referenced Apr 28, 2018

np.ctypeslib.as_array leaks memory when used on a pointer #6511

Closed

Fix for issue 6176 Cannot convert boolean arrays to ctypes with ctypeslib #6177

Closed

charris mentioned this pull request May 18, 2018

Test failures TestNewBufferProtocol.test_error_too_many_dims #11115

Closed

charris reviewed May 20, 2018

View reviewed changes

Comment thread numpy/ctypeslib.py Outdated

Copy link
Copy Markdown

Member

charris May 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is el_type? A complete docstring would help here.

charris reviewed May 20, 2018

View reviewed changes

Comment thread numpy/tests/test_ctypeslib.py Outdated

This comment was marked as resolved.

Sign in to view

charris reviewed May 20, 2018

View reviewed changes

eric-wieser mentioned this pull request May 25, 2018

BUG: np.ctypeslib.as_array(pointer, shape) fails #10968

Closed

eric-wieser force-pushed the cut-down-ctypeslib branch from 2bb3d77 to f6a6009 Compare May 26, 2018 08:08

eric-wieser force-pushed the cut-down-ctypeslib branch from f6a6009 to 8a8be50 Compare May 26, 2018 08:23

charris merged commit bfaaf9d into numpy:master Jun 7, 2018

charris changed the title ~~WIP: Remove fragile use of __array_interface__ in ctypeslib.as_array~~ WIP: Remove fragile use of __array_interface__ in ctypeslib.as_array Jun 16, 2018

Uh oh!

Conversation

eric-wieser commented Apr 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser commented Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

eric-wieser commented Apr 26, 2018

Uh oh!

charris commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eric-wieser commented May 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

charris commented May 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eric-wieser commented May 4, 2018

Uh oh!

eric-wieser commented May 20, 2018

Uh oh!

charris commented May 20, 2018

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!

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!

This comment was marked as resolved.

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!

eric-wieser commented Apr 25, 2018 •

edited

Loading

charris commented May 3, 2018 •

edited

Loading

eric-wieser commented May 4, 2018 •

edited

Loading

charris commented May 4, 2018 •

edited

Loading