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

Remove overrides of standard warning behaviour #7204

Merged

Conversation

jakelishman
Copy link
Member

Summary

Previously, qiskit attempted to override the deprecation warning filters
for internal qiskit code as soon as it was imported, so that they were
shown with the default filters. This isn't something we should be
doing; it's a user's choice, and it's unexpected that we would override
their settings in order to violate the normal Python behaviour. It also
had no effect on the tests, since test runners hook into the warnings
control at various times during the execution. If we do want to do
something similar, it would have been better to use
warnings.filterwarnings, instead of a private function.

The other change is to remove the "only warn once" filter from
qiskit.utils.deprecate_function. It's the default behaviour of Python
to only show one warning from each (category, message, module, line)
set, so users already shouldn't be too overwhelmed (and they should
change their code if they're seeing them anyway). Within the test
suite, it's important that all warnings are raised all the time, so we
can assert that deprecated calls warn and nothing else does; if things
only warn once, then the execution order of the tests matters for
correctness.

This should have no effect on the test suite as it stands right now,
because even one warning is enough to fail the entire run, and the
filtering behaviour was overridden by the runner anyway.

Details and comments

Having this will make it easier to put full warning assertions into the test suite (part of the solution to #6904). Currently, using self.assertWarns is unreliable if the cause is something decorated deprecate_function, because the "only warn once" behaviour means that sometimes the assertion will fail. It depends on execution order, whether it's in a ddt block, and a few other factors.

Previously, qiskit attempted to override the deprecation warning filters
for internal qiskit code as soon as it was imported, so that they were
shown with the default filters.  This isn't something we should be
doing; it's a user's choice, and it's unexpected that we would override
their settings in order to violate the normal Python behaviour.  It also
had no effect on the tests, since test runners hook into the warnings
control at various times during the execution.  If we do want to do
something similar, it would have been better to use
`warnings.filterwarnings`, instead of a private function.

The other change is to remove the "only warn once" filter from
`qiskit.utils.deprecate_function`.  It's the default behaviour of Python
to only show one warning from each `(category, message, module, line)`
set, so users already shouldn't be too overwhelmed (and they should
change their code if they're seeing them anyway).  Within the test
suite, it's important that all warnings are raised all the time, so we
can assert that deprecated calls warn and nothing else does; if things
only warn once, then the execution order of the tests matters for
correctness.

This should have no effect on the test suite as it stands right now,
because even one warning is enough to fail the entire run, and the
filtering behaviour was overridden by the runner anyway.
levbishop
levbishop previously approved these changes Nov 2, 2021
Copy link
Member

@levbishop levbishop left a comment

Choose a reason for hiding this comment

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

I've been thinking to do this for some months, but you beat me to it. Thanks.

@jakelishman
Copy link
Member Author

It's possible people have seen issues with too many warnings in the past because it can be a little mind-bending to set stacklevel correctly in warnings.warn, and we've not always got it right. When you do, Python should only display each "logical" triggering of a warning once. If you set the stack level too high (something we've done before), you may get more warnings than we'd like.

@levbishop
Copy link
Member

Does this warrant a release note or something to advise users on how to make sure they learn about qiskit deprecations even outside of main? I agree its not really our place to fiddle with the filter, but some the extant behaviour does raise the probability these get seen

@jakelishman
Copy link
Member Author

jakelishman commented Nov 2, 2021

I could do a release note to highlight it, but I think for most users not much should actually change. Running in Jupyter will display deprecation warnings without the filter override.

The exception is if they're using a downstream package that depends on Qiskit and uses deprecated functionality, then they'll stop seeing warnings that weren't their fault. It's the downstream package's responsibility to run tests with deprecation warnings shown, and I think all major Python test runners immediately remove the "ignore" filter of deprecation warnings. I suppose they could also be missed if users are packaging up their own code into libraries that they're then calling from Jupyter, but I think we're better optimising the experience for people writing new code. People packaging up libraries should (in theory) know how to test them as well.

For example, here's the behaviour in Jupyter with #7212 (so Aer currently throws a warning on import, because it triggers a deprecation) but without this PR, i.e. the filter override is still in place:

Screenshot 2021-11-02 at 16 37 41

Now with Python's default warning behaviour (i.e. with this PR), but the deprecation warning still being triggered by Aer shows

Screenshot 2021-11-02 at 16 40 03

So the user still sees things that they're doing wrong, but doesn't get blamed for things we're doing wrong, which we should be catching in tests.

@jakelishman
Copy link
Member Author

jakelishman commented Nov 2, 2021

@levbishop: added release notes. It's not entirely an API change, but the "upgrade notes" section seemed like the best bet.

@jakelishman jakelishman added the Changelog: API Change Include in the "Changed" section of the changelog label Nov 2, 2021
@levbishop
Copy link
Member

I could do a release note to highlight it, but I think for most users not much should actually change. Running in Jupyter will display deprecation warnings without the filter override.

I take your point and this is the philosophy of the default filters. Nevertheless I'm sure a non-zero number of users have some kind of local package of functionality that they maintain, and import from it for their experimentation eg in jupyter, but this local package may be more a way to keep a few of their common workflows, not a fully-engineered solution with CI and test suite and so on, so they may miss deprecations.

Agreed that this is the users' responsibility to change the warning filter in this case (or deal with the consequences) but not all will do so....

@coveralls
Copy link

coveralls commented Nov 2, 2021

Pull Request Test Coverage Report for Build 1417954246

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.0002%) to 82.318%

Totals Coverage Status
Change from base Build 1417605609: -0.0002%
Covered Lines: 49270
Relevant Lines: 59853

💛 - Coveralls

@mergify mergify bot merged commit 9dc0966 into Qiskit:main Nov 3, 2021
@jakelishman jakelishman deleted the fix/remove-manual-warning-overrides branch November 4, 2021 10:54
@kdk kdk added this to the 0.19 milestone Nov 15, 2021
ElePT pushed a commit to ElePT/qiskit-algorithms that referenced this pull request Jul 27, 2023
* Remove overrides of standard warning behaviour

Previously, qiskit attempted to override the deprecation warning filters
for internal qiskit code as soon as it was imported, so that they were
shown with the default filters.  This isn't something we should be
doing; it's a user's choice, and it's unexpected that we would override
their settings in order to violate the normal Python behaviour.  It also
had no effect on the tests, since test runners hook into the warnings
control at various times during the execution.  If we do want to do
something similar, it would have been better to use
`warnings.filterwarnings`, instead of a private function.

The other change is to remove the "only warn once" filter from
`qiskit.utils.deprecate_function`.  It's the default behaviour of Python
to only show one warning from each `(category, message, module, line)`
set, so users already shouldn't be too overwhelmed (and they should
change their code if they're seeing them anyway).  Within the test
suite, it's important that all warnings are raised all the time, so we
can assert that deprecated calls warn and nothing else does; if things
only warn once, then the execution order of the tests matters for
correctness.

This should have no effect on the test suite as it stands right now,
because even one warning is enough to fail the entire run, and the
filtering behaviour was overridden by the runner anyway.

* Add upgrade release notes

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: API Change Include in the "Changed" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants