WIP: Remove fragile use of `__array_interface__` in ctypeslib.as_array #10970

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

numpy/ctypeslib.py

-                    'data': (ct.addressof(self), False),
-                    }
-
-        simple_type.__array_interface__ = property(__array_interface__)


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

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

numpy/ctypeslib.py

-    # Prep that numerical ctypes types:
-    for types, code in simple_types:
-        for tp in types:
-            prep_simple(tp, "%c%d" % (code, ct.sizeof(tp)))


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

numpy/ctypeslib.py


-    def as_array(obj, shape=None):
+    def as_array(obj, shape=()):


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

numpy/ctypeslib.py

+    return {
+        _dtype(ctype).str: ctype
+        for ctype in simple_types
+    }


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

numpy/ctypeslib.py

-                 'data': (ct.addressof(contents), False),
-                 'shape': shape}
+def _ctype_ndarray(el_type, shape):
+    """ Create an ndarray of the given element type and shape """


What is el_type? A complete docstring would help here.

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

numpy/ctypeslib.py


+def _get_typecodes():
+    """ Get a dictionary mapping __array_interface__ formats to ctypes types """


Maybe """Return ...

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

numpy/ctypeslib.py


-    ################################################################
-    # public functions
+if ctypes is not None:


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

numpy/ctypeslib.py

        The numpy array shares the memory with the ctypes object.

-        The size parameter must be given if converting from a ctypes POINTER.
-        The size parameter is ignored if converting from a ctypes array
+        The shape parameter must be given if converting from a ctypes POINTER.


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

numpy/ctypeslib.py

-                prep_pointer(obj, shape)
-            else:
-                prep_array(tp)
+        if isinstance(obj, ctypes._Pointer):


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

numpy/tests/test_ctypeslib.py

@@ -21,11 +21,12 @@
 except ImportError:
    _HAS_CTYPE = False

+
+@pytest.mark.skipif(not _HAS_CTYPE,


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

Discussed below

numpy/tests/test_ctypeslib.py

@@ -113,3 +111,59 @@ def test_cache(self):
        a1 = ndpointer(dtype=np.float64)
        a2 = ndpointer(dtype=np.float64)
        assert_(a1 == a2)
+
+
+@pytest.mark.skipif(not _HAS_CTYPE,


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

numpy/tests/test_ctypeslib.py

+class TestAsArray(object):
+    def test_array(self):
+        from ctypes import c_int
+        at = c_int * 2


at could be more descriptive.

Now pair_t

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

numpy/tests/test_ctypeslib.py

+                    reason="ctypes not available on this python installation")
+class TestAsArray(object):
+    def test_array(self):
+        from ctypes import c_int


Blank line after imports is standard.

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

numpy/tests/test_ctypeslib.py

+        assert_array_equal(a, np.array([[1, 2], [3, 4], [5, 6]]))
+
+    def test_pointer(self):
+        from ctypes import c_int, cast, POINTER


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

numpy/tests/test_ctypeslib.py

+        assert_raises(TypeError, as_array, p)
+
+    def test_struct_array_pointer(self):
+        from ctypes import c_int16, Structure, pointer


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

numpy/tests/test_ctypeslib.py

@@ -113,3 +111,59 @@ def test_cache(self):

a1 = ndpointer(dtype=np.float64)

a2 = ndpointer(dtype=np.float64)

assert_(a1 == a2)

@pytest.mark.skipif(not _HAS_CTYPE,

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


		def _get_typecodes():
		""" Get a dictionary mapping __array_interface__ formats to ctypes types """

WIP: Remove fragile use of __array_interface__ in ctypeslib.as_array #10970

WIP: Remove fragile use of __array_interface__ in ctypeslib.as_array #10970

Conversation

eric-wieser commented Apr 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser commented Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser commented Apr 26, 2018

charris commented May 3, 2018 • edited Loading

eric-wieser commented May 4, 2018 • edited Loading

charris commented May 4, 2018 • edited Loading

eric-wieser commented May 4, 2018

eric-wieser commented May 20, 2018

charris commented May 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charris commented May 20, 2018

eric-wieser commented May 21, 2018

eric-wieser commented May 21, 2018

charris commented May 21, 2018

eric-wieser commented May 26, 2018

charris commented Jun 7, 2018

WIP: Remove fragile use of `__array_interface__` in ctypeslib.as_array #10970

WIP: Remove fragile use of `__array_interface__` in ctypeslib.as_array #10970

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