Skip to content

Pytest 3.8.0 broke some NumPy tests. #3945

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
4 tasks
charris opened this issue Sep 6, 2018 · 16 comments
Closed
4 tasks

Pytest 3.8.0 broke some NumPy tests. #3945

charris opened this issue Sep 6, 2018 · 16 comments
Labels
type: question general question, might be closed after 2 weeks of inactivity

Comments

@charris
Copy link

charris commented Sep 6, 2018

See numpy/numpy#11895 for comments and https://travis-ci.org/numpy/numpy/jobs/425358526 for failing tests. The tests pass with pytest 3.6.1, but fail with 3.8.0. The failing warning is disabled in the tests by

# As we are testing matrices, we ignore its PendingDeprecationWarnings
try:
    import pytest
    pytestmark = pytest.mark.filterwarnings(
        'ignore:the matrix subclass is not:PendingDeprecationWarning')
except ImportError:
    pass

But this seems to no longer succeed for parametrized tests. The error message with latest NumPy master is

charris@fc [numpy.git (master)]$ python runtests.py -t numpy/matrixlib/tests/test_defmatrix.py 
Building, see build.log...
Build OK
NumPy version 1.16.0.dev0+b8f3be9
NumPy relaxed strides checking option: True

=============================================================== ERRORS ================================================================
________________ ERROR collecting build/testenv/lib64/python2.7/site-packages/numpy/matrixlib/tests/test_defmatrix.py _________________
numpy/matrixlib/tests/test_defmatrix.py:344: in <module>
    class TestNewScalarIndexing(object):
numpy/matrixlib/tests/test_defmatrix.py:345: in TestNewScalarIndexing
    a = matrix([[1, 2], [3, 4]])
numpy/matrixlib/defmatrix.py:118: in __new__
    PendingDeprecationWarning, stacklevel=2)
E   PendingDeprecationWarning: the matrix subclass is not the recommended way to represent matrices or deal with linear algebra (see https://docs.scipy.org/doc/numpy/user/numpy-for-matlab-users.html). Please adjust your code to use regular ndarray.
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 errors during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
1 error in 0.09 seconds

Thanks for submitting an issue!

Here's a quick checklist in what to include:

  • Include a detailed description of the bug or suggestion
  • pip list of the virtual environment you are using
  • pytest and operating system versions
  • Minimal example if possible
@nicoddemus
Copy link
Member

nicoddemus commented Sep 6, 2018

Hi @charris,

Here's what's happening:

The class TestNewScalarIndexing creates a matrix at class level (during collection):

class TestNewScalarIndexing(object):
    a = matrix([[1, 2], [3, 4]])

(from here).

This construct emits the warning you posted, which was being ignored in previous pytest versions because we did not capture warnings during collection (#3251). Now in 3.8 this issue has been fixed, so pytest is capturing the warning during collection and it is raising an exception according to your configuration. The pytestmark declaration won't have any effect because the error happens before we can finish collecting the module.

In summary things are working as they should, it is just that fixing #3251 now makes it clear that a warning was being raised during collection where it was being ignored in previous pytest versions.

I see two easy ways to fix this:

  1. Move the warning filter configuration to your pytest.ini.

  2. No longer create the matrix during collection, perhaps changing it into a property:

    class TestNewScalarIndexing(object):
        @property
        def a(self):
            return matrix([[1, 2], [3, 4]])

@nicoddemus nicoddemus added the type: question general question, might be closed after 2 weeks of inactivity label Sep 6, 2018
@charris
Copy link
Author

charris commented Sep 6, 2018

Moving into pytest.ini works, but passing via pytest -W options does not. We need the latter as the presence or absence of pytest.ini is how we distinguish releases from development. Do you have any suggestions regarding that?

@nicoddemus
Copy link
Member

nicoddemus commented Sep 6, 2018

Hmm it should work with -W, seems like a bug...

Changing the code as suggested in 2) is not an option? I was about to open a PR moving the configuration to pytest.ini, hehehe.

@charris
Copy link
Author

charris commented Sep 6, 2018

Note that I had to adjust the stacklevel for the option approach to work for 3.7.4, but it does not work in 3.8. Could the option order make a difference?

@nicoddemus
Copy link
Member

It shouldn't, I will look into that.

I've applied option 2) locally and it passes, here's the diff:

diff --git a/numpy/matrixlib/tests/test_defmatrix.py b/numpy/matrixlib/tests/test_defmatrix.py
index 272cd8d52..dcadbfa7b 100644
--- a/numpy/matrixlib/tests/test_defmatrix.py
+++ b/numpy/matrixlib/tests/test_defmatrix.py
@@ -342,7 +342,10 @@ class TestIndexing(object):


 class TestNewScalarIndexing(object):
-    a = matrix([[1, 2], [3, 4]])
+
+    @property
+    def a(self):
+        return matrix([[1, 2], [3, 4]])

     def test_dimesions(self):
         a = self.a
@@ -421,8 +424,13 @@ class TestPower(object):

 class TestShape(object):

-    a = np.array([[1], [2]])
-    m = matrix([[1], [2]])
+    @property
+    def a(self):
+        return np.array([[1], [2]])
+
+    @property
+    def m(self):
+        return matrix([[1], [2]])

     def test_shape(self):
         assert_equal(self.a.shape, (2, 1))
@@ -460,12 +468,14 @@ class TestShape(object):
         assert_equal(x.T.ravel(order='A'), [[1, 2, 3, 4, 5, 6]])

     def test_array_memory_sharing(self):
-        assert_(np.may_share_memory(self.a, self.a.ravel()))
-        assert_(not np.may_share_memory(self.a, self.a.flatten()))
+        a = self.a
+        assert_(np.may_share_memory(a, a.ravel()))
+        assert_(not np.may_share_memory(a, a.flatten()))

     def test_matrix_memory_sharing(self):
-        assert_(np.may_share_memory(self.m, self.m.ravel()))
-        assert_(not np.may_share_memory(self.m, self.m.flatten()))
+        m = self.m
+        assert_(np.may_share_memory(m, m.ravel()))
+        assert_(not np.may_share_memory(m, m.flatten()))

     def test_expand_dims_matrix(self):
         # matrices are always 2d - so expand_dims only makes sense when the

If that sounds reasonable to you, I can open a PR with this fix.

@charris
Copy link
Author

charris commented Sep 6, 2018

Changing the code as suggested in 2) is not an option?

Well, we could revisit that. But there are some warnings we want to error for development testing but not in releases, so for development we set the default warning behavior to error in pytest.ini.

@nicoddemus
Copy link
Member

But there are some warnings we want to error for development testing but not in releases, so for development we set the default warning behavior to error in pytest.ini.

Oh but option 2) doesn't require changing any of that, we just change the code slightly so the warning is not triggered during collection.

@charris
Copy link
Author

charris commented Sep 6, 2018

There are more tests :) That fix does not make me happy, it is a bit obscure, but it looks like the approach is lazy generation of the test matrices. We could go that way, setup/teardown would have the same effect I suppose, but if you plan to fix pytest so that the option approach works, I'll just pin pytest to an earlier version.

@nicoddemus
Copy link
Member

but if you plan to fix pytest so that the option approach works

Oh definitely. Pinning the version to !=3.8.0 is fine. 👍

@nicoddemus
Copy link
Member

I investigated the reason why -W appeared to not have an effect, and discovered that the problem is that pytest takes in account the command-line options before the ini options. Because of how the warnings filter works, the last filter configured takes precedence over the others, so what happens is that -W is applied, but then error from the ini file takes precedence and so the error still persists.

But IIUC, for your use case where the tox.ini file is not present, the -W option should work, right?

@charris
Copy link
Author

charris commented Sep 6, 2018

Let me check... Yes, the command line options work when pytest.ini is not present.

@charris
Copy link
Author

charris commented Sep 6, 2018

The order of precedence almost seems backward, but that is probably one of those things that could be argued at length.

@nicoddemus
Copy link
Member

Great, so I think you can go with that route now then. 👍

I opened #3946 to change the precedence order, but I believe this kind of change will have to wait for 3.9 because there might be users which rely on the current precedence of the options.

@charris
Copy link
Author

charris commented Sep 6, 2018

Thanks for all the help.

@charris
Copy link
Author

charris commented Sep 6, 2018

I'll close this for now.

@charris charris closed this as completed Sep 6, 2018
@nicoddemus
Copy link
Member

Sure, glad to help. Thanks.

charris added a commit to charris/numpy that referenced this issue Sep 6, 2018
Pytest < 3.8 ignored warnings issued during test collection, but that
changed in pytest 3.8 and the method NumPy used to suppress the
PendingDeprecationWarning for matrices no longer worked, or rather, was
exposed as not working. The fix here is to suppress the warning in
pytest.ini and pytesttester.py , which should work as long as the tests
are the only places left where NumPy uses matrices.

An alternate fix is to delay the construction of matrices in the tests
until they are actually run, which has the virtue of test localization
but is a bit more complicated.

See pytest-dev/pytest#3945 for discussion.
charris added a commit to charris/numpy that referenced this issue Sep 7, 2018
Pytest < 3.8 ignored warnings issued during test collection, but that
changed in pytest 3.8 and the method NumPy used to suppress the
PendingDeprecationWarning for matrices no longer worked, or rather, was
exposed as not working. The fix here is to suppress the warning in
pytest.ini and pytesttester.py , which should work as long as the tests
are the only places left where NumPy uses matrices.

An alternate fix is to delay the construction of matrices in the tests
until they are actually run, which has the virtue of test localization
but is a bit more complicated.

See pytest-dev/pytest#3945 for discussion.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: question general question, might be closed after 2 weeks of inactivity
Projects
None yet
Development

No branches or pull requests

2 participants