catch more warnings in common tests #11109
Comments
|
Do you mean deprecation warnings like the one below? If you can guide me a little, I am happy to work on this issue. |
|
I meant deprecation warnings raised in scikit-learn, not in Numpy. That one you mention is during compilation. I meant during testing. |
|
On your machine, you can run the tests with Example: To silence warnings, see scikit-learn/sklearn/utils/testing.py Lines 269 to 292 in 4b24fbe |
|
Hi, I am looking into this issue, I figure the "common tests" means the test_common.py under the tests directory? I tried running the test but no warning is generated, could you tell me what warning is expected? Thanks a lot. |
Yes
You need to add the |
|
Huh, That's weird. I did have |
|
figured out, I was using an older pytest verision. |
|
Last time I looked at this, I realised that one of the problem (there may be more) was that import warnings
import pytest
from sklearn.utils.testing import ignore_warnings
from sklearn.utils.testing import assert_warns_message
def warns():
warnings.warn("some warning")
return 1
@ignore_warnings()
def test_1():
print('before:', warnings.filters)
assert_warns_message(UserWarning, 'some warning', warns)
print('after:', warnings.filters)
# This warning is visible because assert_warns_message resets
# warnings.filters.
warns()
def test_12():
print('test12:', warnings.filters)
ignore_common_warnings = pytest.mark.filterwarnings('ignore::UserWarning')
@ignore_common_warnings
def test_2():
warns()
@ignore_common_warnings
def test_3():
assert_warns_message(UserWarning, 'some warning', warns)
# This warning is visible
warns() |
|
Interesting. I would be in favor of replacing with pytest.warns(UserWarning, match='some warning'):
warn()to avoid dealing with it.. |
|
If someone has a semi-automated way of replacing the Maybe implementing def assert_warns_regex(warning_cls, warning_message, func, *args, **kwargs):
with pytest.warns(warning_cls, match=re.escape(warning_message)):
func(*args, **kwargs) |
|
The reason is that there were bugs in Python around warning registry.
pytest might have ironed out the wrinkles, so I'm +1 for using or wrapping
it if possible.
|
We could still keep
True, that would be faster, but then there isn't that many lines to change, % git grep assert_warns_message | wc -l [master]
162
% git grep assert_warns | wc -l [master]
319to warant keeping a wrapper, I think. Directly using pytest warning capture in the code base without a wrapper would be IMO cleaner for contributors reading the code... That could be a part of cleaning up |
|
That makes sense, thanks a lot. But when I tried replacing the And there is sth that wasn't clear to me: when using Isn't the |
|
Not sure what you have tried but if I were you I would try to look at a single test that shows a problem, for example, I get a ConvergenceWarning with this test: print |
|
Can I just comment out the |
|
We may not have strong enough tests to check it, but I promise
clean_warnings_registry is there for a reason!
Oh, but what I hadn't realised is that:
when using ignore_warnings as a wrapper, it uses a catch_warnings context
manager.
when using ignore_warnings as a context manager, it does not use a
catch_warnings context manager. I think this is a bug. Could using a
catch_warnings context in ignore_warnings.__enter__ help solve the issue
here?
|
|
After more investigation, the issue with using |
|
That really helped me understand, thanks a lot! @jnothman def test_non_meta_estimators(name, Estimator, check):
with ignore_warnings(category=(DeprecationWarning, ConvergenceWarning,
UserWarning, FutureWarning)): # this will be reset by check's ignore_warnings
# Common tests for non-meta estimators
estimator = Estimator()
set_checking_parameters(estimator)
check(name, estimator)
# this will reset ignore_warnings, leaving only DeprecationWarning and FutureWarning filters
@ignore_warnings(category=(DeprecationWarning, FutureWarning))
def check_dont_overwrite_parameters(name, estimator_orig):I have to say that I don't see why it has to overwrite the filters passed to it, I guess, to me, it makes more sense to add more filters (as defined in the check's decorator) instead of reset the filters. I could implement the adding more filters thing, but if you are worried about affecting other code, I could also just remove all the |
|
Could someone confirm this? Appreciate it |
|
I think the discrepancy between the context manager and decorator forms of
ignore_warnings should be fixed. I am not sure if that will fix all the
issues here.
Clearing the warning registry is not about changing the filters, iirc, but
affects filters that only display the first of duplicate warnings
|
|
I am pretty sure I got it right in my previous post. def resetwarnings():
"""Clear the list of warning filters, so that no filters are active."""
filters[:] = []
_filters_mutated()I have tried adding |
|
Yes, I see. But this only applies to the current In [9]: warnings.resetwarnings()
In [10]: warnings.filters
Out[10]: []
In [11]: warnings.simplefilter('ignore')
In [12]: with warnings.catch_warnings() as c:
...: warnings.resetwarnings()
...: print(warnings.filters)
...:
[]
In [13]: print(warnings.filters)
[('ignore', None, <class 'Warning'>, None, 0)] |
|
This means that it does indeed matter whether the |
|
I have not understood your proposals @ShangwuYao. A PR with an example would help clarify. I think we could remove Another solution would be to backport from __future__ import absolute_import, division, print_function
import inspect
import py
import sys
import warnings
import re
def warns(expected_warning, *args, **kwargs):
"""Assert that code raises a particular class of warning.
Specifically, the parameter ``expected_warning`` can be a warning class or
sequence of warning classes, and the inside the ``with`` block must issue a warning of that class or
classes.
This helper produces a list of :class:`warnings.WarningMessage` objects,
one for each warning raised.
This function can be used as a context manager, or any of the other ways
``pytest.raises`` can be used::
>>> with warns(RuntimeWarning):
... warnings.warn("my warning", RuntimeWarning)
In the context manager form you may use the keyword argument ``match`` to assert
that the exception matches a text or regex::
>>> with warns(UserWarning, match='must be 0 or None'):
... warnings.warn("value must be 0 or None", UserWarning)
>>> with warns(UserWarning, match=r'must be \d+$'):
... warnings.warn("value must be 42", UserWarning)
>>> with warns(UserWarning, match=r'must be \d+$'):
... warnings.warn("this is not here", UserWarning)
Traceback (most recent call last):
...
Failed: DID NOT WARN. No warnings of type ...UserWarning... was emitted...
"""
match_expr = None
if not args:
if "match" in kwargs:
match_expr = kwargs.pop("match")
return WarningsChecker(expected_warning, match_expr=match_expr)
else:
func = args[0]
with WarningsChecker(expected_warning, match_expr=match_expr):
return func(*args[1:], **kwargs)
class WarningsRecorder(warnings.catch_warnings):
"""A context manager to record raised warnings.
Adapted from `warnings.catch_warnings`.
"""
def __init__(self):
super(WarningsRecorder, self).__init__(record=True)
self._entered = False
self._list = []
@property
def list(self):
"""The list of recorded warnings."""
return self._list
def __getitem__(self, i):
"""Get a recorded warning by index."""
return self._list[i]
def __iter__(self):
"""Iterate through the recorded warnings."""
return iter(self._list)
def __len__(self):
"""The number of recorded warnings."""
return len(self._list)
def pop(self, cls=Warning):
"""Pop the first recorded warning, raise exception if not exists."""
for i, w in enumerate(self._list):
if issubclass(w.category, cls):
return self._list.pop(i)
__tracebackhide__ = True
raise AssertionError("%r not found in warning list" % cls)
def clear(self):
"""Clear the list of recorded warnings."""
self._list[:] = []
def __enter__(self):
if self._entered:
__tracebackhide__ = True
raise RuntimeError("Cannot enter %r twice" % self)
self._list = super(WarningsRecorder, self).__enter__()
warnings.simplefilter("always")
return self
def __exit__(self, *exc_info):
if not self._entered:
__tracebackhide__ = True
raise RuntimeError("Cannot exit %r without entering first" % self)
super(WarningsRecorder, self).__exit__(*exc_info)
class WarningsChecker(WarningsRecorder):
def __init__(self, expected_warning=None, match_expr=None):
super(WarningsChecker, self).__init__()
msg = (
"exceptions must be old-style classes or " "derived from Warning, not %s"
)
if isinstance(expected_warning, tuple):
for exc in expected_warning:
if not inspect.isclass(exc):
raise TypeError(msg % type(exc))
elif inspect.isclass(expected_warning):
expected_warning = (expected_warning,)
elif expected_warning is not None:
raise TypeError(msg % type(expected_warning))
self.expected_warning = expected_warning
self.match_expr = match_expr
def __exit__(self, *exc_info):
super(WarningsChecker, self).__exit__(*exc_info)
# only check if we're not currently handling an exception
if all(a is None for a in exc_info):
if self.expected_warning is not None:
if not any(issubclass(r.category, self.expected_warning) for r in self):
__tracebackhide__ = True
raise AssertionError(
"DID NOT WARN. No warnings of type {} was emitted. "
"The list of emitted warnings is: {}.".format(
self.expected_warning, [each.message for each in self]
)
)
elif self.match_expr is not None:
for r in self:
if issubclass(r.category, self.expected_warning):
if re.compile(self.match_expr).search(str(r.message)):
break
else:
raise AssertionError(
"DID NOT WARN. No warnings of type {} matching"
" ('{}') was emitted. The list of emitted warnings"
" is: {}.".format(
self.expected_warning,
self.match_expr,
[each.message for each in self],
)
)(under a compatible MIT licence: https://docs.pytest.org/en/latest/license.html) |
|
Thank you so much for the clarification @jnothman , I absolutely agree with what you said. Not using In [38]: print(warnings.filters)
[]
In [39]: warnings.simplefilter('ignore', UserWarning)
In [40]: print(warnings.filters)
[('ignore', None, <class 'UserWarning'>, None, 0)]
In [41]: with ignore_warnings(category=(DeprecationWarning)): #[a test in test_common.py]
...: print(warnings.filters)
...: with ignore_warnings(category=(UserWarning)): #[a check in estimator_checks.py]
...: print(warnings.filters)
...: warnings.warn("yo", DeprecationWarning)
...:
[('ignore', None, <class 'DeprecationWarning'>, None, 0)]
[('ignore', None, <class 'UserWarning'>, None, 0)]
/anaconda/bin/ipython:5: DeprecationWarning: yo
In [42]: print(warnings.filters)
[]tests in |
|
BTW, I think neither decorator nor context manager is working correctly for 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)context manager: warnings.resetwarnings()
warnings.simplefilter('ignore', UserWarning)
print(warnings.filters)
controlled_warning_context_manager()
print(warnings.filters)output: decorator: warnings.resetwarnings()
warnings.simplefilter('ignore', UserWarning)
print(warnings.filters)
controlled_warning_decorator()
print(warnings.filters)but I am not too sure about this, correct me if I were wrong |
|
no I think the current output looks right. within the ignore context, there
should only be a single ignore-all-Warnings filter
|
|
that's true, but the non-local scope is affected, (I mean the third output is wrong, not the second). In both examples, I set a filter for (and there is another post above that, could you please take a look as well, thanks!) |
|
I looked at this one a bit more in details and I am in favour of never calling import warnings
from sklearn.utils.testing import ignore_warnings
def warns():
print('inside warns:', warnings.filters)
warnings.warn('some warning', UserWarning)
def check():
with ignore_warnings(category=DeprecationWarning):
warns()
def test():
with ignore_warnings(category=UserWarning):
check()
test()Output: In other words the innermost I completely understand the need for the If I comment 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, []) # Note: test fails hereI really don't think that affecting |
|
You might be right @lesteve that we don't need to reset warning filters.
@dengemann, do you recall much about this?
|
Right now the output of travis is too large to render, partially because a lot of deprecation warnings and partially because of warnings from the common tests. The common tests should catch all the deprecation warnings and probably also convergence warnings or numerical warnings (we decrease the number of iterations for faster testing).
The text was updated successfully, but these errors were encountered: