[MRG+2] catch more expected warnings in common tests #11151

ShangwuYao · 2018-05-27T20:13:49Z

Reference Issues/PRs

Fixes #11109. See also #7006.

What does this implement/fix? Explain your changes.

Ignore the expected warning information in common tests, including DeprecationWarning, ConvergenceWarning, UserWarning and FutureWarning.
Removed unnecessary warnings.resetfilters in clean_warning_registry().
Changed ignore_warnings to ignore both local warnings and external warnings like suppress_warnings in numpy.
Fixed a bug in using ignore_warnings as context manager, restore warning filters upon exiting, avoid affecting non-local filters.

Any other comments?

rth · 2018-05-27T20:30:33Z

Thanks for this PR @ShangwuYao !

ignore_warnings will not work as a decorator in combination with pytest parametrization on Python 2, I think.

You could use it as a context manager with ignore_warnings(): inside the test though..

ShangwuYao · 2018-05-27T20:55:39Z

Thanks for the clarification @rth , I have tried several different ways of using ignore_warnings, including decorator and context manager, but whichever way I am doing it, I still have those warnings generated when running pytest on my local machine. Do you know why is that?

rth · 2018-05-27T21:20:40Z

Yes, I wonder if pytest warning capture interferes with ignore_warnings.

You could also try to use the pytest warnings filter mechanism, approximately,

ignore_common_warnings = pytest.mark.filterwarnings(
     'ignore:.*:UserWarning',
     'ignore:.*:ConvergenceWarning',
     ...)


@ignore_common_warnings
def some_test_function():
   pass

(I think, pytest filterwarnings arguments are not too well documented)

ShangwuYao · 2018-05-28T02:11:47Z

Yeah, I tried that, it didn't work either.

The next thing I tried is comparing this file (test_common.py) with other test files, and found that the ignore_warnings works everywhere except in test_non_meta_estimators and test_no_attributes_set_in_init, and those two methods both uses pytest.mark.parametrize, so I think this might be the reason why the ignore_warnings is not working. So I went to lower level of the test_non_meta_estimators, for example, the check_classifier_data_not_an_array in estimator_checks.py, and added this decorator to the method:

@ignore_warnings(category=(DeprecationWarning, ConvergenceWarning,
                                   UserWarning, FutureWarning))

And it did helped, but I think I should only ignore those warnings in the common test, so I am still looking into how to deal with the pytest.mark.parametrize problem.

ShangwuYao · 2018-05-29T23:51:44Z

Can you help me reopen it? @rth I was trying to rollback and commit. Thanks!

jnothman · 2018-05-30T02:47:14Z

I'm not sure what's happened, but the Reopen button is disabled.

ShangwuYao · 2018-06-05T17:43:15Z

def controlled_warning_context_manager():
    with ignore_warnings():
        warnings.warn('yo', UserWarning)
        print(warnings.filters)

@ignore_warnings
def controlled_warning_decorator():
    warnings.warn('yo', UserWarning)
    print(warnings.filters)

Using ignore_warnings as context manager:

warnings.resetwarnings()
warnings.simplefilter('ignore', UserWarning)
print(warnings.filters)
controlled_warning_context_manager()
print(warnings.filters)

[('ignore', None, <class 'UserWarning'>, None, 0)]
[('ignore', None, <class 'Warning'>, None, 0), ('ignore', None, <class 'UserWarning'>, None, 0)]
[('ignore', None, <class 'UserWarning'>, None, 0)]

Using ignore_warnings as decorator:

warnings.resetwarnings()
warnings.simplefilter('ignore', UserWarning)
print(warnings.filters)
controlled_warning_decorator()
print(warnings.filters)

[('ignore', None, <class 'UserWarning'>, None, 0)]
[('ignore', None, <class 'Warning'>, None, 0), ('ignore', None, <class 'UserWarning'>, None, 0)]
[('ignore', None, <class 'UserWarning'>, None, 0)]

Both are working as expected now, can see old erroneous behavior in #11109 .

ShangwuYao · 2018-06-05T20:44:28Z

This work is done, could some one review it? Thanks! @rth @jnothman

jnothman

In travis Python 2.7: there are 762 warnings in master, but 5555 warnings in this PR
In travis Python 3.4: there are 8718 warnings in master, but 6743 in this PR.
In travis Python 3.6: there are 8105 warnings in master, but 6004 in this PR.

So something is going wrong in Python 2.7. Perhaps this is one of the Python bugs that I noted was motivation for resetting warning filters in some contexts...

Search for "warnings in" in each of the respective logs to confirm these results. (Please report similar results in future updates to this PR.)

jnothman · 2018-06-06T03:41:13Z

sklearn/utils/testing.py

@@ -137,6 +137,7 @@ def assert_warns(warning_class, func, *args, **kw):

    """
    # very important to avoid uncontrolled state propagation
+    warnings.resetwarnings()


Elsewhere there was a suggestion that we should never need to resetwarnings.

Yes please see #11109 (comment) (if you have not seen it yet).

Ah actually looking at the diff, warnings.resetwarnings has been removed from clean_warning_registry which is the change I was advocating.

It would be great if you could give us some reason why you need warnings.resetwarnings here. I am hoping we don't need to use it anywhere. In case we do need to use warnings.resetwarnings, it should be used inside a warnings.catch_warnings context manager so that the global warnings.filters is not affected.

I have looked at #11109 before I did this. After I removed the warnings.resetwarnings() from clean_warning_registry, I couldn't pass one test case - the test_warn in test_testing:

# This class is inspired from numpy 1.7 with an alteration to check # the reset warning filters after calls to assert_warns. # This assert_warns behavior is specific to scikit-learn because # `clean_warning_registry()` is called internally by assert_warns # and clears all previous filters. class TestWarns(unittest.TestCase): def test_warn(self): def f(): warnings.warn("yo") return 3 # Test that assert_warns is not impacted by externally set # filters and is reset internally. # This is because `clean_warning_registry()` is called internally by # assert_warns and clears all previous filters. warnings.simplefilter("ignore", UserWarning) assert_equal(assert_warns(UserWarning, f), 3) # Test that the warning registry is empty after assert_warns assert_equal(sys.modules['warnings'].filters, []) assert_raises(AssertionError, assert_no_warnings, f) assert_equal(assert_no_warnings(lambda x: x, 1), 1)

It looks like former developers expect to use warnings.resetwarnings in assert_warns , while I think we should store the filters, and restore them upon exiting, but this seems like another issue, I intended to create another PR for this. Right now I am just making it behave the same as I haven't remove warnings.resetwarnings in clean_warning_registry . If you could tell me what you want for the assert_warns, I can do it.

I commented somewhere else that you should remove these two lines:

# Test that the warning registry is empty after assert_warns assert_equal(sys.modules['warnings'].filters, [])

Expecting a assert_warns call to affect the global warnings.filters is not something I would expect. I can't see a good reason for it.

lesteve · 2018-06-06T07:32:50Z

sklearn/utils/estimator_checks.py

@@ -304,7 +304,7 @@ def check_estimator(Estimator):
        except SkipTest as message:
            # the only SkipTest thrown currently results from not
            # being able to import pandas.
-            warnings.warn(message, SkipTestWarning)
+            warnings.warn(str(message), SkipTestWarning)


For the record I bumped into the same problem. This is clearly a bug in our code, trying to provide an exception object into warnings.warn only work if none of the warnings filters use a regex. Here is a snippet to show what I means:

import warnings exc = ValueError() warnings.resetwarnings() warnings.warn(exc) # works fine warnings.filterwarnings('always', message='regex_goes_here') warnings.warn(exc) # exception here TypeError: expected string or bytes-like object

While you are at it, you may as well rename message -> exception to improve readability.

jnothman · 2018-06-06T21:56:32Z

Does anyone have an idea why warnings would increase on py2?

lesteve · 2018-06-06T22:40:50Z

Not off the top of my head no. I would be quite happy with just reducing the warnings on Python 3 to a manageable number to be perfectly honest.

More generally, something quite useful to reduce the number of warnings would be to break it down by warning class as I did in #10158 (comment) for example.

ShangwuYao · 2018-06-07T00:28:55Z

hmm, I compared the warning log before and after the change, seems like the new warnings after my change are pretty legitimate warnings. Maybe because I fixed those bugs, we actually get to see those warnings that we are supposed to see.
And I don't even understand why python 2.7 have so much less warnings in the first place, this might be the reason?
newpy27log.txt
oldpy27log.txt

jnothman · 2018-06-07T00:54:27Z

Yes, I suppose that's a reasonable explanation. LGTM

ShangwuYao · 2018-06-07T02:20:32Z

Thank you all a lot! should I do anything to merge?

jnothman · 2018-06-07T03:55:03Z

Wait for a second reviewer to approve. :)

lesteve · 2018-06-07T08:30:16Z

Can I have a breakdown of warnings by warning class just for test_common.py as I asked above? Without this I feel this is a bit a shot in the dark ...

I have looked at this closely and based on what I found, I think these changes makes sense to me, but 1. I am not 100% sure of the subtle impacts of the changes and the Py27 increase is a bit weird
2. to be reassured, it would be nice to check that we can reduce the warnings in Py3 to a manageable number.

jnothman · 2018-06-07T12:31:12Z

And yes, I agree, the reduction is not a lot; the warnings in Py3 are still not manageable.

ShangwuYao · 2018-06-07T12:42:34Z

This PR is initially intended for catching warnings in common tests, which is now reduced from thousands to 5-6. And during the process, I fixed the bugs in ignore_warnings, assert_warns and clean_warning_registry , if you intend to clear all expected warnings for the entire library, I can add filters to the tests, but those are not the intention for this PR.

jnothman · 2018-06-07T12:44:51Z

I agree.

Why is "4403 /home/lesteve/dev/alt-scikit-learn/sklearn/utils/deprecation.py:77: DeprecationWarning: Function squared_exponential is deprecated; The function squared_exponential of correlation_models is deprecated in version 0.19.1 and will be removed in 0.22." apparently appearing after this PR and not before??

ShangwuYao · 2018-06-07T13:07:34Z

well, if it is deprecated, and no filters are set, it should give the warnings. As I checked, that is the case.
The external filters were messed up before, so these warnings somehow get ignored. I didn't figure out exactly why either.
The deprecation warnings you talked about came from test_gaussian_process.py, this one file generates 4752 warnings. After I set the warning filters, all those warnings disappeared as I tested locally, shall I commit this change as well?

lesteve · 2018-06-07T14:20:05Z

sklearn/utils/testing.py

        if self._entered:
            raise RuntimeError("Cannot enter %r twice" % self)
        self._entered = True
        self._filters = self._module.filters
        self._module.filters = self._filters[:]
        self._showwarning = self._module.showwarning
+        warnings.simplefilter("ignore", self.category)


Can you explain why you moved it here?

~~Should there not be an equivalent warnings.filters.pop() in __exit__?~~ Sorry for the noise this is not needed as warnings.filters are restored in __exit__.

Suppose there are some external warning filters and local warning filters, and this change means that in the local domain, both the external warning filters and local warning filters are working, and upon exiting this local domain, the external warning filters are restored to be the way they were (before entering the local domain). This is intended to match the behavior of context manager as decorator, since the decorator use with warnings.catch_warnings(): to do so.
I have proposed to use this in the previous posts several times, I think this makes sense but no one confirms this, and then I found numpy's suppress_warnings has the same behavior.

Without moving this here, the warning.filters will restore both the local filters as well as the external filters upon exiting.

lesteve · 2018-06-07T14:22:46Z

While you are at it, I think you can remove this warnings.simplefilter in esttimator_checks.py:

scikit-learn/sklearn/utils/estimator_checks.py

Line 330 in 1c61b8a

warnings.simplefilter("ignore", ConvergenceWarning)

ShangwuYao · 2018-06-07T19:12:30Z

I don't quite understand why the build failed.

lesteve · 2018-06-08T04:57:43Z

I don't quite understand why the build failed.

There were due to flake8 errors that you introduced.

I pushed some tweaks in the last commit, let me know if you have comments about them. I think this is looking good, being able to remove 4k warnings in test_gaussian_process.py in a straightforward manner makes me confident that the way we are handling the warnings is a lot less hacky than before.

This is the only thing I can get to work under Python 2.7

lesteve · 2018-06-08T09:55:57Z

LGTM. The number of warnings are:
Python 2.7 build: 797
Python 3.4 build: 2028
Python 3.6 build: 1448

I am strongly in favour of creating separate PRs to investigate how to reduce the number of warnings further.

FYI there were quirks on Python 2.7 due to the interaction of pytest.mark.parametrize and ignore_warnings/pytest.mark.filterwarnings which I did not investigate further. This is why my last commit reverts to using a ignore_warnings context manager as was the case originally.

rth

LTGM (apart for the minor comment below), thanks for working on this!

I am strongly in favour of creating separate PRs to investigate how to reduce the number of warnings further.

+1

rth · 2018-06-09T08:27:42Z

sklearn/gaussian_process/tests/test_gaussian_process.py

@@ -143,7 +144,6 @@ def test_batch_size():


 def test_random_starts():
-    # Test that an increasing number of random-starts of GP fitting only


Is this removal intentional?

ShangwuYao · 2018-06-09T13:55:25Z

alright, then I will create another PR to further reduce warnings.

lesteve · 2018-06-11T06:53:32Z

Merging this one, thanks a lot for working on this @ShangwuYao!

ShangwuYao · 2018-06-11T13:31:06Z

This is exciting, thank you all for reviewing!!

jorisvandenbossche · 2018-06-13T12:23:09Z

sklearn/gaussian_process/tests/test_gaussian_process.py

@@ -15,6 +15,7 @@
 from sklearn.datasets import make_regression
 from sklearn.utils.testing import assert_greater, assert_true, assert_raises

+pytestmark = pytest.mark.filterwarnings('ignore', category=DeprecationWarning)


What is the reason that DeprecationWarnings are ignored here? (this is not part of the common tests)

This PR fixed bugs in ignore_warnings and clean_warning_registry, see here, not just common tests. Before this, some warnings that should be raised in test_gaussian_process was somehow suppressed, after this change, they are correctly raised, and this file alone raises more than 4000 new DeprecationWarnings, so we ignored it to see this fix is really working, see here. We probably shouldn't ignore this warning as you said.

What is the reason that DeprecationWarnings are ignored here? (this is not part of the common tests)

Unless I am missing something sklearn/gaussian_process/gaussian_process.py is deprecated and scheduled to be removed in 0.20.

Ah, that's a good reason :-)
Ideally I would say that deprecation warnings are asserted once and then explicitly ignored for other tests, but since this is deprecating a full sub-module and not a method or so, then probably makes sense to just ignore them for the full submodule.

ShangwuYao changed the title ~~ignoring expected warnings~~ catch more expected warnings in common tests May 27, 2018

ShangwuYao closed this May 29, 2018

ShangwuYao force-pushed the sw branch from bdf4314 to 20cb37e Compare May 29, 2018 23:50

removed reset filtering to correctly ignore expected warnings

1007334

ShangwuYao reopened this May 30, 2018

added back __warningregistry__, remove resetfilters only

5127659

ShangwuYao changed the title ~~catch more expected warnings in common tests~~ [WIP] catch more expected warnings in common tests Jun 5, 2018

shangwuyao added 2 commits June 5, 2018 13:18

fixed bug in context manager, restore filters upon exiting

7d5db52

removed debug folder

9335661

shangwuyao added 2 commits June 5, 2018 14:28

fixed type error and incorrect test case

d417646

clear warning filters for assert_warn

58e4f8c

jnothman reviewed Jun 6, 2018

View reviewed changes

lesteve reviewed Jun 6, 2018

View reviewed changes

remove reset warning in assert_warns

0b41dfa

jnothman approved these changes Jun 7, 2018

View reviewed changes

lesteve reviewed Jun 7, 2018

View reviewed changes

shangwuyao added 2 commits June 7, 2018 12:58

clear anticipated warnings for test_gaussion_process

e030adb

fixed merge error

a8280b0

ShangwuYao force-pushed the sw branch from dc14749 to a8280b0 Compare June 7, 2018 18:22

Merge branch 'master' into sw

e9e6553

Tweaks

e5f3fa2

lesteve added 3 commits June 8, 2018 07:23

Python 2 fix

a9b5960

For some reason parametrize + filterwarnings does not work

28e740e

Use ignore_warnings context manager

52e7e42

This is the only thing I can get to work under Python 2.7

lesteve changed the title ~~[WIP] catch more expected warnings in common tests~~ [MRG+1] catch more expected warnings in common tests Jun 8, 2018

amueller mentioned this pull request Jun 8, 2018

Enable warnings on travis (and make sure they are reasonable) #10158

Open

3 tasks

lesteve mentioned this pull request Jun 8, 2018

[MRG+2] Pytest parametrize unit tests #11074

Merged

3 tasks

rth approved these changes Jun 9, 2018

View reviewed changes

rth changed the title ~~[MRG+1] catch more expected warnings in common tests~~ [MRG+2] catch more expected warnings in common tests Jun 9, 2018

lesteve added 2 commits June 11, 2018 08:56

Add deleted comment

e68f4bb

Fix removed docstring

493f966

lesteve merged commit cb1dbc2 into scikit-learn:master Jun 11, 2018

ShangwuYao mentioned this pull request Jun 12, 2018

Further reduce the number of warnings in testing #11240

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG+2] catch more expected warnings in common tests #11151

[MRG+2] catch more expected warnings in common tests #11151

		@@ -143,7 +144,6 @@ def test_batch_size():


		def test_random_starts():
		# Test that an increasing number of random-starts of GP fitting only

[MRG+2] catch more expected warnings in common tests #11151

[MRG+2] catch more expected warnings in common tests #11151

Conversation

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment