BUG: Move ctypes ImportError catching to appropriate place #8898

davidjn · 2017-04-05T23:33:16Z

The current implementation has a try/except ImportError block around a statement that has no imports. This is leftover from some previous changes that didn't fully clean up the code.

eee00f8 introduced functionality for this module to work when ctypes is not available.
3c1a13d Moved the import statement out of the try/except block and to top file, but did not account for behavior when ctypes is not importable.

The current implementation has a try/except ImportError block around a statement that has no imports. This is leftover from some previous changes that didn't fully clean up the code. Commit [1] introduced functionality for this module to work when ctypes is not available. Commit [2] Moved the import statement out of the try/except block and to top file, but did not account for behavior when ctypes is not importable. [1] numpy@eee00f8 [2] numpy@3c1a13d

eric-wieser · 2017-04-06T11:08:19Z

Commit message should start with MAINT: or perhaps BUG:.

I don't really follow here what we were trying to achieve in eee00f8 by substituting ctypes with _missing_ctypes - _getintp_ctype still fails, but now does so with a more cryptic message.

davidjn · 2017-04-06T18:00:37Z

Sorry about the commit message title- first time contributor here :).

I agree that eee00f8 left _getintp_ctype still failing. Prior to my proposal, the top of file "import _ctypes" will fail with an ImportError. With my proposal, _getintp_ctype (which is called by get_shape() and get_stride() in the _ctypes class) will fail with an AttributeError.

Do you have a recommendation here? It seems like options are to either (a) provide fakes for ctypes.c_int, ctypes.c_long, and ctypes.c_longlong or (b) catch the behavior in _getintp_ctype and raise a "ctypes not available" message there. There is a similar approach to option (b) here (introduced with 2b6af2c).

The context for this is that I'm trying to get matplotlib to run in an environment where the _ctypes module is not available. Things seem work if I either use an older version of numpy, or I make the proposed change to my local numpy. Perhaps there are also some mpl calls that run _getintp_ctype under the hood, and I just haven't hit those yet.

eric-wieser · 2017-04-06T18:15:30Z

Sorry about the commit message title- first time contributor here :).

That's alright, I can edit the commit message the expense of squashing (which for one commit, is fine)

To be clear, the patch you have is clearly an improvement, and does the job of mostly fixing what 3c1a13d broke. I was hoping you

To me, the best fix would be:

class dummy_ctype(object):
    def __init__(self, cls):
        self._cls = cls
    def __mul__(self, other):
        return self
    def __call__(self, other):
        return cls(other)

And have _getintp_ctype just return dummy_ctype(np.intp).

While you're there, get_shape could be reduced to:

def get_shape(self):
    return self.shape_as(_getintp_ctype())

The dummy_ctype object is a naive fake used when ctypes is not importable. It does not intend to fake any ctypes behavior, but provides a duck-typed interface so that method calls do not fail. The get_shape and get_strides refactors take advantage of other instance methods and reduce code duplication.

davidjn · 2017-04-06T19:08:55Z

Thanks Eric, I've added your recommendations and also reduced get_strides accordingly.

eric-wieser · 2017-04-06T19:12:07Z

numpy/core/_internal.py

+        self._cls = cls
+    def __mul__(self, other):
+        return self
+    def __call__(self, other):


Hmm, this might actually need to be *other, since we call it with * further down. Is there any easy way to test a lack of ctypes?

I think you're right. Updated with the next commit.

Adds tests for numpy/core/_internal.py to test the _ctypes class when the ctypes module is or isn't available.

davidjn · 2017-04-07T00:10:57Z

Apologies again on the commit title not including "BUG".

I've updated the dummy_ctypes object per your comment, and added numpy/core/tests/test_internal.py to validate that the _ctypes object (and its get_shape method, which calls _getintp_ctype) work properly when the ctypes module is and is not available.

eric-wieser

Nice job! Lets see whether those run...

eric-wieser · 2017-04-07T00:11:06Z

numpy/core/tests/test_internal.py

+        from numpy.core import _internal
+
+        _ctypes = _internal._ctypes(self.test_arr)
+        shape = _ctypes.get_shape()


Should just test the .shape property here - get_shape is an implementation detail

I removed the shape assertions altogether- the intent was to test getintp_ctype through the get_shape method. I've added a simple test for getintp_ctype instead so that now the shape test is redundant with the assert_array_equal assertion.

eric-wieser · 2017-04-07T00:11:58Z

numpy/core/tests/test_internal.py

+        # sys.modules.
+        sys.modules['ctypes']  = None
+        del np.core._internal
+        del sys.modules['numpy.core._internal']


Oh geez, that's pretty scary...

Worried this will break other tests, but I guess we'll see

Updated to set _internal.ctypes to None at the beginning of the test, and resetting _internal.ctypes = ctypes in the tearDown method.

eric-wieser · 2017-04-07T00:13:08Z

numpy/core/tests/test_internal.py

+        shape = _ctypes.get_shape()
+
+        self.assertEqual(ctypes, _ctypes._ctypes)
+        self.assertEqual(len(shape), 2)


assert_equal(shape, [2, 3]) would be shorter here. Also, we seem to prefer assert_equal over self.assertEqual

Updated self.assertEqual usage to assert_equal. I removed the shape assertions altogether- the intent was to test getintp_ctype through the get_shape method. I've added a simple test for getintp_ctype instead so that now the shape test is redundant with the assert_array_equal assertion.

intent was to test getintp_ctype through the get_shape method

This was the correct thing to do. But even better would be to test through the .shape property

So that we can use np.intp.

eric-wieser · 2017-04-07T00:15:38Z

@charris: And AxisError and TooHardError not being in _globals rear their head...

davidjn · 2017-04-07T00:21:12Z

@charris: And AxisError and TooHardError not being in _globals rear their head...

Hmm, yea I didn't consider how this may affect other tests. I'm not sure what else to do about this... suggestions are welcomed!

eric-wieser · 2017-04-07T00:28:31Z

As a stopgap, you could try adding a try/finally that attempts to put the modules back together again afterwards

davidjn · 2017-04-07T00:38:29Z

I think that last commit may do the trick, I've removed the sys.modules munging. I also learned how to run the full test suite locally, and this makes things pass on my machine. Lets see if it builds...

davidjn · 2017-04-07T01:04:28Z

Alright, that seems to have cleared the cross-test issues. I'll add another commit to take care of your few suggestions on the tests, and we'll hopefully be good to go.

Fwiw, I was calling get_shape to indirectly test the _getintp_ctype method. I'll update the test as you suggest, and add a simple standalone test class for the _getintp_ctype method.

davidjn · 2017-04-07T19:02:00Z

@eric-wieser, I've addressed your comments and added a simple test class for _getintp_ctype. Thanks for the reviews, comments, and assistance!

eric-wieser · 2017-04-07T19:58:26Z

Fwiw, I was calling get_shape to indirectly test the _getintp_ctype method.

Yes, I agree, we should test get_shape. But because shape = property(get_shape, ...), we should test that by just testing .shape instead

eric-wieser

Thanks for the persistenceSorry to keep complaining, but I think these tests should be structured in terms of the public interface.

This _ctypes class is actually visible through something like np.array([1]).ctypes, so really we should be testing that interface instead, rather than the implementation details.

A test of np.array([1]).ctypes.shape with and without ctypes present should do the job.

I think that all these tests probably belong in a TestCtypes class in test_multiarray.py

eric-wieser · 2017-04-07T19:59:17Z

numpy/core/_internal.py

 def _getintp_ctype():
+    if ctypes is None:
+        import numpy as np
+        return dummy_ctype(np.intp) 


This should make use of the cache, that the normal code path takes

eric-wieser · 2017-04-07T20:03:28Z

numpy/core/tests/test_internal.py

+
+    def tearDown(self):
+        # Ensure that ctypes is set on _internal after tests run.
+        _internal.ctypes = ctypes  


I think this might be better handled as just

def test_ctypes_is_not_available(self): _internal.ctypes = None try: do_the_test() finally: _internal.ctypes = ctypes

Or if you want to avoid repetition:

def without_ctypes(f): @functools.wraps(f) def wrapper(*args, **kwargs): _internal.ctypes = None try: return f(*args, **kwargs) finally: _internal.ctypes = ctypes @without_ctypes def test_ctypes_is_not_available(self): do_the_test

It's a bit confusing as you have it to replace it in contexts where it shouldn't have changed

eric-wieser · 2017-04-07T20:55:51Z

numpy/core/tests/test_multiarray.py

+            _ctypes = np.array(self.test_arr).ctypes
+
+            self.assertIsInstance(_ctypes._ctypes, _internal._missing_ctypes)
+            assert_equal(_ctypes._arr.shape, (2, 3))


this should be _ctypes.shape, not _ctypes._arr.shape. Preferably, I'd spell it test_arr.ctypes.shape, and eliminate the _ctypes variable

eric-wieser · 2017-04-07T20:56:22Z

numpy/core/tests/test_multiarray.py

+
+class TestCTypes(TestCase):
+    def setUp(self):
+        self.test_arr = np.array([[1, 2, 3], [4, 5, 6]])


It's good practice to put cheap setup (like this) in the tests themselves, so that its self contained

eric-wieser · 2017-04-07T20:58:58Z

Also looks like you forgot to commit the cache changes you mention in the commit message

eric-wieser · 2017-04-07T21:44:56Z

numpy/core/tests/test_multiarray.py

@@ -21,6 +21,7 @@

 import numpy as np
 from numpy.compat import strchar, unicode
+from numpy.core import _internal


Might be tempted to move this inside the test, so the hack is all in one place

Moving in the next commit.

eric-wieser · 2017-04-07T21:45:50Z

numpy/core/tests/test_multiarray.py

+            test_arr = np.array([[1, 2, 3], [4, 5, 6]])
+
+            self.assertIsInstance(
+                test_arr.ctypes._ctypes, _internal._missing_ctypes)


Debatable whether testing implementation details like this (and the one above) makes any sense

If you feel strongly, I'll remove it. I think its a good check to keep. For instance, if someone decides to add an "import ctypes" statement into one of these methods in the future, this assertion will catch that. Otherwise, the test will continue to pass.

Yeah, my concernt is that that's somewhat artificial, and doesn't really catch us doing import ctypes anywhere else - these tests wouldn't catch the bug that caused you to patch this in the first place, for instance.

I don't feel strongly about it, but would appreciate input from someone else on a better way to test this.

Fair enough. Will wait for additional input.

I'm almost tempted to spin off a new python interpreter without ctypes to run the test, to ensure it can't break anything else

You might also be able to get your sys.modules hackery to work if you try to put everything back together again, but also a little risky.

Hey Eric,

I tried again with the sys.modules hackery, and trying to put things back together again with the try/finally block. Unfortunately, other tests continue to fail. I guess this is because tests are run in parallel. With the current setup (setting ctypes = None), we will have similar cross-test side effects, but I guess we are getting lucky in that it isn't causing any errant failures.

By "spin off a new python interpreter", are you suggesting to execute the test with python's subprocess?

Eg (?):
def some_test(self):
self.assertEqual(0, subprocess.call(sys.executable, "-c", test_code)

Suggesting to execute the test with python's subprocess?

Yes, that is what I mean. I'm not actually sure that's a good idea though, and I don't think you should risk wasting time trying it until someone else weighs in.

eric-wieser · 2017-04-07T21:46:18Z

numpy/core/tests/test_multiarray.py

+
+            self.assertIsInstance(
+                test_arr.ctypes._ctypes, _internal._missing_ctypes)
+            assert_equal(test_arr.shape, (2, 3))


This needs to be test_arr.ctypes.shape. We specifically need to test the ctypes shape, not the normal one.

Sorry for the confusion here, updating in the next commit.

Great, thanks

eric-wieser · 2017-04-07T22:07:33Z

numpy/core/tests/test_multiarray.py

@@ -6734,16 +6733,17 @@ def test_ctypes_is_available(self):
        test_arr = np.array([[1, 2, 3], [4, 5, 6]])

        self.assertEqual(ctypes, test_arr.ctypes._ctypes)
-        assert_equal(test_arr.shape, (2, 3))
+        assert_equal(list(test_arr.ctypes.shape), (2, 3))


assert_equal(list(...), some_tuple) is a little jarring - maybe replace list with tuple?

Sure thing, commit incoming.

eric-wieser · 2017-04-10T18:56:48Z

Perhaps we should merge this, and just create a separate issue for properly testing without ctypes?

Can you summarize how exactly you ended up with a ctypes-less python installation in the first place?

davidjn · 2017-04-10T19:22:47Z

Can you summarize how exactly you ended up with a ctypes-less python installation in the first place?

Sure, I work on Google's App Engine, and matplotlib is enabled by default as a third party library [1]. For security sandboxing, we disable some of the standard library's C-based modules, including _ctypes. Our production numpy version is an older version from before commit 3c1a13d, so things work fine when _ctypes is not available in production.

I'm working now to enable matplotlib functionality out-of-the-box with App Engine's local development server. The development server uses import hooks to similarly inhibit standard library C-based modules. We do this so that the local development environment matches the production environment as close as possible. Without the proposed fix, users will have to install an older version of numpy for matplotlib to work out of the box.

[1] https://cloud.google.com/appengine/docs/standard/python/tools/built-in-libraries-27

davidjn · 2017-04-17T15:58:21Z

Hey Eric, just checking in to see what you think the next steps here might be. Should we merge and open another issue for properly testing later? Or should we go ahead with the subprocess testing approach?

Are there other numpy contributors who may have a better recommendation for testing?

eric-wieser · 2017-04-17T16:18:51Z

Should we merge and open another issue for properly testing later?

I think this is the best path.

Are there other numpy contributors who may have a better recommendation for testing?

I'm hoping that one will turn up, and confirm that what you have here is a pretty reasonable compromise.

eric-wieser · 2017-05-06T17:21:00Z

@charris: It'd be nice to get this into 1.13, especially since this is a regression in 1.10.0.

charris · 2017-05-06T18:17:08Z

@eric-wieser I'd be happy to move AxisError and TooHardError into _globals if that helps. Should be easy to avoid problems I think.

charris · 2017-05-06T18:19:32Z

@eric-wieser If you feel it is in good shape to merge, go ahead. The milestone is for things that need to be in 1.13, not all things that can go into it.

eric-wieser · 2017-05-06T18:47:19Z

Should be easy to avoid problems I think.

I'd worry that npy_cache_import is in general bad news for a module reload, so I think that it's best to stay away from reloading modules in a testsuite.

If you feel it is in good shape to merge, go ahead

I felt a little unqualified to make that judgement because parts of the patch are based on code I suggested - but if this looked ok to you after a quick once-over, then I'm content to pull the trigger.

The milestone is for things that need to be in 1.13, not all things that can go into it.

Is the milestone also (retrospectively) for things that are in 1.13? Seems like a convenient way to attach PRs to the releases that they appear in.

charris · 2017-05-06T18:58:37Z

Is the milestone also (retrospectively) for things that are in 1.13?

No. I've seen that done in SciPy for generating the set of PRs for the release, but I think it is a bit too tricky to rely on. Plus I've been deleting old release milestones. There is a script that could be modified to add milestones if it seems helpful.

eric-wieser · 2017-05-06T19:00:59Z

I'll stop doing that then!

charris · 2017-05-06T21:03:50Z

I'd worry that npy_cache_import is in general bad news for a module reload

The C modules cannot be reloaded, so I expect the import would keep a reference to the original function/class and thus keep it in memory even if the defining python module is reloaded. That is what ISTR from the discussion of the reload problem anyway.

charris · 2017-05-07T22:33:51Z

@eric-wieser Still looking to put this in 1.13? I'm thinking of branching tomorrow and pushing off everything still unmerged to 1.14.

eric-wieser · 2017-05-07T23:21:57Z

Yeah, I think I'll just merge this. This definitely fixes the problem it set out to, and it doesn't look like it affects a normal setup with ctypes present.

Worst case scenario, the test does something screwy regarding threading, but the only way we'll spot that it through merging anyway, and it won't affect end-users.

This fixes a regression in 3c1a13d, restoring the behaviour intended by eee00f8. This also extends the support on ctype-less systems for `arr.ctypes.shape` and `arr.ctypes.strides`. The dummy_ctype object is a naive fake used when ctypes is not importable. It does not intend to fake any ctypes behavior, but provides a duck-typed interface so that method calls do not fail. The get_shape and get_strides refactors take advantage of other instance methods and reduce code duplication.

eric-wieser added 03 - Maintenance 00 - Bug component: numpy._core labels Apr 6, 2017

davidjn changed the title ~~Move ctypes ImportError catching to appropriate place~~ BUG: Move ctypes ImportError catching to appropriate place Apr 6, 2017

eric-wieser reviewed Apr 6, 2017

View reviewed changes

davidjn added 2 commits April 6, 2017 16:57

BUG: Updating dummy_ctype's behavior.

c9fe3ba

Adding numpy/core/tests/test_internal.py.

b29ae31

Adds tests for numpy/core/_internal.py to test the _ctypes class when the ctypes module is or isn't available.

eric-wieser reviewed Apr 7, 2017

View reviewed changes

Add numpy import to _internal.py.

bb56070

So that we can use np.intp.

BUG: move numpy import into _getintp_ctype

3af4fe5

BUG: Removing sys mangling from test_ctypes_is_not_available

4e2ab70

David Nicholson added 2 commits April 7, 2017 11:24

Adding GetIntPCtypeTest and minor clean up

bd3011e

Fixing GetIntPCtypeTest for x-platform

9b19671

eric-wieser reviewed Apr 7, 2017

View reviewed changes

BUG: Use cache in _getintp_ctype, move tests.

0e4942a

eric-wieser reviewed Apr 7, 2017

View reviewed changes

_cyptes._arr.shape -> _ctypes.shape

82bebd2

David Nicholson added 2 commits April 7, 2017 14:23

BUG: Fixing tests and actually udating _getintp_ctype to use cache.

647a75e

BUG: Fixing tests and actually udating _getintp_ctype to use cache.

e75a486

eric-wieser reviewed Apr 7, 2017

View reviewed changes

BUG: Updates to TestCTypes test class.

06b3a82

eric-wieser reviewed Apr 7, 2017

View reviewed changes

BUG: list casting -> typle casting.

2652a1e

eric-wieser added the 58 - Ready for review label Apr 22, 2017

eric-wieser added this to the 1.13.0 release milestone May 6, 2017

eric-wieser added the 06 - Regression label May 6, 2017

eric-wieser merged commit d7d1b2a into numpy:master May 7, 2017

eric-wieser mentioned this pull request Jul 1, 2017

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

Closed

eric-wieser mentioned this pull request May 21, 2018

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

Merged

BUG: Move ctypes ImportError catching to appropriate place #8898

BUG: Move ctypes ImportError catching to appropriate place #8898

Conversation

davidjn commented Apr 5, 2017 • edited by eric-wieser Loading

eric-wieser commented Apr 6, 2017

davidjn commented Apr 6, 2017

eric-wieser commented Apr 6, 2017 • edited Loading

davidjn commented Apr 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidjn commented Apr 7, 2017

eric-wieser left a 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

eric-wieser commented Apr 7, 2017

davidjn commented Apr 7, 2017

eric-wieser commented Apr 7, 2017

davidjn commented Apr 7, 2017

davidjn commented Apr 7, 2017

davidjn commented Apr 7, 2017

eric-wieser commented Apr 7, 2017

eric-wieser left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser commented Apr 7, 2017

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

eric-wieser Apr 7, 2017 • 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 10, 2017

davidjn commented Apr 10, 2017

davidjn commented Apr 17, 2017

eric-wieser commented Apr 17, 2017

eric-wieser commented May 6, 2017 • edited Loading

charris commented May 6, 2017

charris commented May 6, 2017

eric-wieser commented May 6, 2017

charris commented May 6, 2017

eric-wieser commented May 6, 2017

charris commented May 6, 2017 • edited Loading

charris commented May 7, 2017

eric-wieser commented May 7, 2017 • edited Loading

davidjn commented Apr 5, 2017 •

edited by eric-wieser

Loading

eric-wieser commented Apr 6, 2017 •

edited

Loading

eric-wieser left a comment •

edited

Loading

eric-wieser Apr 7, 2017 •

edited

Loading

eric-wieser Apr 7, 2017 •

edited

Loading

eric-wieser commented May 6, 2017 •

edited

Loading

charris commented May 6, 2017 •

edited

Loading

eric-wieser commented May 7, 2017 •

edited

Loading