Skip to content
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

TST: Suppressed warnings #7099

Merged
merged 13 commits into from
Sep 2, 2016
Merged

TST: Suppressed warnings #7099

merged 13 commits into from
Sep 2, 2016

Conversation

seberg
Copy link
Member

@seberg seberg commented Jan 23, 2016

OK, this is a start, a context manager to suppress warnings a little better (nothing is prefect, though maybe I am missing something). And trying to remove all those "ignore" stuff everywhere mostly.

A few quite general warnings are still suppressed globally (where possible, sometimes needs local duplication), that may be fine. The switch to only show warnings in release mode is missing, and since some things never showed warnings for me, I sometimes removed it completely. This is not beta or rc, but I think you can get the idea.

of setting a warning to "ignore", it is possible to add a suppression.
This has the advantage of ensuring a clean warning registry.

Suppressions are inserted either immediatly, or upon entering the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo 'immediatly'

@charris
Copy link
Member

charris commented Jan 23, 2016

The switch to only show warnings in release mode is missing

What do you mean? Currently it depends on the version string.

@seberg
Copy link
Member Author

seberg commented Jan 23, 2016

Hmmm, python 3.4 and later actually should invalidate the warning registry it appears, so the dance could be unnecessary there.... Not sure what that means.

@charris I mean in this version I broke it currently.

@seberg seberg force-pushed the suppressed_warnings branch 3 times, most recently from 43892e5 to 4320148 Compare January 23, 2016 11:47
@seberg
Copy link
Member Author

seberg commented Jan 23, 2016

Ok, this is getting somewhere. The biggest issue for me is that I bet scipy will explode in dev mode at least. I also added stacklevel to most warnings (that I found quickly with a grep), no tests for that sorry, but that would seem insane :).

There are no tests for the new warning context manager yet, but I will probably add them later. Comments?!

@seberg
Copy link
Member Author

seberg commented Jan 23, 2016

The solution is currently to keep some rather broad global suppressions, but I think that is somewhat fine, since those could be tried to be narrowed down later.

@seberg
Copy link
Member Author

seberg commented Jan 23, 2016

Hmmm, anyone has an idea why the benchmarks are not successful? Is it an asv problem?!

@seberg seberg force-pushed the suppressed_warnings branch 5 times, most recently from 7d26647 to 2fa8be6 Compare January 23, 2016 14:46
@seberg
Copy link
Member Author

seberg commented Jan 23, 2016

OK, I guess adding stacklevel is nonsense. Undid it. I avoided the ASV problem now.

@seberg
Copy link
Member Author

seberg commented Jan 23, 2016

Hmmm, I readded the stacklevel stuff. I guess we say it is right, I am not quite sure, also it does change things slightly, since the warnings.filterwarnings(module=...) will now be the caller instead of us.

@seberg
Copy link
Member Author

seberg commented Jan 23, 2016

@charris since AppVeyor is so slow, if you would go to teams there and add "GitHub teams", might that give us the option to abort test runs manually?

@charris
Copy link
Member

charris commented Jan 23, 2016

I can't get anywhere on AppVeyor, their interface is crap. I'll probably need to delete all the cookies and fool around to figure out how to do anything, but IIRC, you need to have an AppVeyor account and then I can add you to the team on AppVeyor. AppVeyor really needs someone who can do interfaces.

@seberg
Copy link
Member Author

seberg commented Jan 23, 2016

OK, this is actually in a state where it can be reviewed now.


Note that all of this is only necessary for Python versions before
python (about) 3.4. After that "ingnore" can be used safely within
the catch_warnings context.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I missed some issue where this was discussed in detail, but I don't really get the point of why this is really needed. Apparently there's a bug in Python 2.7 in the warnings module, which we cannot work around and therefore we need to replace catch_warnings with a new context manager?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all about the __warningregistry__ and avoiding getting stuff into it, which catch_warnings does not really help with at all. On newer pythons, this is solved (the registry is invalidated automatically).

@seberg
Copy link
Member Author

seberg commented Jan 24, 2016

OK, I have thought about it some more. The new context manager might be mostly not needed. There are three reasons why it is nice (I have to check whether I actually use the second one):

  1. Printing the warning is actually possible. Without this context manager, we have to filter out all warnings on the outer level in release mode. So it is either hide everything or raise as an error.
  2. It is possible to record more convenient:
with suppressed_warnings(record=True) as sup:
    sup.append(FutureWarning, "I can ignore this")
    warnings.filterwarnings("always", category=DeprecationWarning,
                                          message="This is interesting!")

    command_that_gives_both_warnings()
    assert_(len(sup.log) == 1)

If the futurewarning would not occur every time, counting things would get really annoying. I admit there may be only a single occurance of this (plus the deprecation tests could be made nicer with this, there I needed this kind of behaviour to make the test specific to a single deprecation).

  1. If you use it as the inner context manager, the outer suppressions are still active (without recording only), so applying more global suppressions can be far more convenient, and that might actually be used a few times (it is hard to tell without trying what happens when you remove the global ones).

In summery, I still don't care too much about this context manager, but would like to get somewhere with this, because this is just needed to test warnings reliably.
My take is this, if we say we are content with either raising warnings or hiding them and you are unhappy about this rather complicated context manager, then I can get rid of it again and work around it where it is annoying. There is also this clear_and_catch_warnings thing that can do the second thing as well with a little more precision needed.

EDIT: added third reason ;)

@seberg
Copy link
Member Author

seberg commented Aug 31, 2016

I think I got the last comments. Had some doc fixups and added a note to the release note. Can squash a little when the time comes.

@charris
Copy link
Member

charris commented Sep 1, 2016

I can put this in as is. If you can easily squash the reversions and some of the fixups, that would be fine also, but don't put too much effort into it.

Also modify the corresponding test to suppress the non Deprecation
warnings created to test specificity.
This means that warnings of different origin then the one tested for
behave normally. If the normal behaviour is to igonre them this might
decrease specificity in rare cases. In most cases the specificity
will be slightly higher.
Printing of datetime arrays used to cause warning due to
comparison warnings in NaT. This is circumvented by using views
to integer values. Part of this should be simplified when
the deprecation is over.

Also fixes a bug with non-native byteorder.
Comment mentions a speedup, but it seems unsure why it should
be there. Instead use an error state in divide_by_count.
Some extra complex warnings had to be ignored (but those seemed correct)
In some places, just remove aparently unnecessary filters.

After this, all cases of ignore filters should be removed from
the tests, making testing (even multiple runs) normally fully
predictable.
Making the outer context manager a suppress warnings gives good
control to print warnings only once in release mode and suppress
some specific warnings which cannot be easily avoided otherwise.
Also silences a spurious warning during tests (the multiplication
could give a warning).
These are warnings, which when raised as an error for one reason
or another are already silenced.
The PIPE in the tests caused a ResourceWarning during testing in
python 3.
they used to be called `..._w_...` and `..._wo_...`.
@seberg
Copy link
Member Author

seberg commented Sep 2, 2016

OK, got rid of the two REVs (and put that doc thing to the part where its implemented). Should be good.

@charris charris merged commit 9164f23 into numpy:master Sep 2, 2016
@charris
Copy link
Member

charris commented Sep 2, 2016

In it goes, one more step on the way to 1.12. Thanks Sebastian.

@seberg
Copy link
Member Author

seberg commented Sep 2, 2016

Thanks for the persistent reviewing :).

@seberg seberg deleted the suppressed_warnings branch September 2, 2016 14:59
@seberg
Copy link
Member Author

seberg commented Sep 2, 2016

Ahhh, just rebased my oindex branch on this, and I get a nice list of all the tests that need modification to be future proof with it :).

@charris charris changed the title Suppressed warnings TST: Suppressed warnings Nov 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants