-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
better warning filter for assert_* #6212
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
Conversation
e02b28b
to
8525240
Compare
@@ -29,7 +29,8 @@ def wrapper(*args, **kwargs): | |||
__tracebackhide__ = True | |||
|
|||
with warnings.catch_warnings(): | |||
warnings.simplefilter("always") | |||
# only remove filters that would "error" | |||
warnings.filters = [f for f in warnings.filters if f[0] != "error"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we rely on warnings.filters
? As far as I can tell, that's semi-public at best: the library is very consistent on using _
as a prefix for truly private attributes, but it also doesn't document filters
or list it in __all__
. However, the standard library doesn't change very often, so I guess it's not too risky...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I am not sure either. But I found no "public" way to unset a specific filter. I think even if it breaks at one point it should mainly affect downstream libraries and not users.
The alternatives are
- always warn
- never warn
- remove the decorator
I would prefer the "result" of what I propose here but I also understand if you deem it too dangerous. I had a quick look at the python issues but found nothing in this direction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warnings
will only change on upgrade to a new version of python
, which involves some manual work, anyways (and testing is usually blocked by numba
for a few months).
I guess this means it should be safe to use? @pydata/xarray, what do you think?
Edit: if filters
was renamed to something else we would get a AttributeError
, but that should be fairly easy to handle
Edit2: to be extra safe we could also ask the python core devs for clarification
Ok merging this. I think we got some to fix it if this ever causes trouble for a future version of python. cc @headtr1ck |
In #4864 I added a a decorator for the
xarray.testing.assert_*
functions to ensure warnings that were to errors (pytest.mark.filterwarnings("error")
) do not error inassert_*
(see #4760 (comment)). As a solution I addedxarray/xarray/testing.py
Line 32 in 5470d93
However, this is sub-optimal because this now removes all
ignore
filters! As dask stuff only gets evaluated inassert_*
filters likewarnings.filterwarnings("ignore", "Mean of empty slice")
don't work for dask arrays!I thought of setting
but this could suppress warnings we want to keep.
So now I remove all
"error"
warning filters and keep the rest. Note that the original filters get restored afterwith warnings.catch_warnings():
. ().I am not sure I expressed myself very clearly... let me know and I can try again. @keewis you had a look at #4864 maybe you can review this PR as well?