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

Warn on import failures within optional dependencies #11522

Merged
merged 3 commits into from
Jan 11, 2024

Conversation

jakelishman
Copy link
Member

@jakelishman jakelishman commented Jan 9, 2024

Summary

This distinguishes the case of "failed to find an optional dependency" from "found the optional dependency, but it failed to import" within the lazy testers. Now, a warning containing the import failures will be shown to users, rather than silently treating the dependency as missing.

This occurred recently when a PR caused an import failure in Aer, which the CI suite failed to detect because the optional-dependency checkers allowed the test suite to pass regardless. Note that this commit alone won't reliably cause the test suite to fail on a failed import because:

  1. this only emits a warning, not an exception (by design).
  2. the unittest decorators are evaluated during test discovery, which happens before we activate our increased warning filters.

This commit also unifies the now two Qiskit-specific warnings (the other being QPYLoadingDeprecatedFeatureWarning) with a common QiskitWarning subclass, much as we do for QiskitError. This gives us a convenient way to globally deny these errors during CI runs, without turning all UserWarnings into errors, which we're not ready to do yet.

Details and comments

I think, in a follow-up PR, we ought to move the major warning filters (error::DeprecationWarning and error::qiskit.exceptions.QiskitWarning) into test.python.__init__ with:

import sys
import warnings

if not sys.warnoptions:
    warnings.filterwarnings("error", category=DeprecationWarning)

    import qiskit.exceptions
    warnings.filterwarnings("error", category=qiskit.exceptions.QiskitWarning)

so that the top-level defaults are set up in the recommended Python way before test discovery. This would let us fail the test suite if an optional has been installed but fails to import, without making the test suite dependent on what order the test cases happen to run or discover.

(It's possible that turning on deprecation warnings as errors during qiskit import might cause us some problems or require us to rewrite small parts of the test suite for old modules to delay the imports, but I think that might be a good thing.)

For example, if this PR is applied on top of #11488 using Aer 0.13.1, which will fail to import, the behaviour of trying to use the failed optional import now looks like this:

from qiskit import QuantumCircuit
from qiskit.providers.fake_provider import FakeAthens

FakeAthens().run(QuantumCircuit(2, 2))
/Users/jake/code/qiskit/terra/qiskit/utils/lazy_tester.py:323: OptionalDependencyImportWarning: While trying to import 'Qiskit Aer', some components were located but raised other errors during import. You might have an incompatible version installed. Qiskit will continue as if the optional is not available.
 - module 'qiskit.providers.aer' failed to import with: ImportError("cannot import name 'UnitaryGate' from 'qiskit.extensions' (unknown location)")
  warnings.warn(message, category=OptionalDependencyImportWarning)
/Users/jake/code/qiskit/terra/qiskit/providers/fake_provider/fake_backend.py:568: RuntimeWarning: Aer not found using BasicAer and no noise
  warnings.warn("Aer not found using BasicAer and no noise", RuntimeWarning)

Previously, it would just silently behave as if Aer wasn't installed at all.

This distinguishes the case of "failed to find an optional dependency"
from "found the optional dependency, but it failed to import" within
the lazy testers.  Now, a warning containing the import failures will be
shown to users, rather than silently treating the dependency as missing.

This occurred recently when a PR caused an import failure in Aer, which
the CI suite failed to detect because the optional-dependency checkers
allowed the test suite to pass regardless.  Note that this commit alone
won't reliably cause the test suite to fail on a failed import because:

1. this only emits a warning, not an exception (by design).
2. the `unittest` decorators are evaluated during test discovery, which
   happens before we activate our increased warning filters.

This commit also unifies the now two Qiskit-specific warnings (the other
being `QPYLoadingDeprecatedFeatureWarning`) with a common
`QiskitWarning` subclass, much as we do for `QiskitError`.  This gives
us a convenient way to globally deny these errors during CI runs,
without turning _all_ `UserWarning`s into errors, which we're not ready
to do yet.
@jakelishman jakelishman added type: qa Issues and PRs that relate to testing and code quality Changelog: New Feature Include in the "Added" section of the changelog labels Jan 9, 2024
@jakelishman jakelishman added this to the 1.0.0 milestone Jan 9, 2024
@jakelishman jakelishman requested a review from a team as a code owner January 9, 2024 14:23
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core
  • @mtreinish
  • @nkanazawa1989

@coveralls
Copy link

coveralls commented Jan 9, 2024

Pull Request Test Coverage Report for Build 7463946793

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 87.766%

Totals Coverage Status
Change from base Build 7453493244: 0.2%
Covered Lines: 59392
Relevant Lines: 67671

💛 - Coveralls

Cryoris
Cryoris previously approved these changes Jan 9, 2024
Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this fix!

Edit: Oh, actually it seems there are some new warnings raised upon sklearn imports...

@jakelishman
Copy link
Member Author

Ha - nice true error on this PR. We have a couple of optionals that drill into subpackages, and the handling I put in to detect whether it's a ModuleNotFoundError from the target module or not fails to account for attempts to import a submodule. That needs an actual fix.

@kevinhartman
Copy link
Contributor

Certainly an improvement to have a distinction!

I'm sure there's a historical reason for this, but why is MissingOptionalLibraryError an Exception, whereas OptionalDependencyImportWarning is a UserWarning?

@jakelishman
Copy link
Member Author

Kevin: do you mean why is the second on derived from UserWarning rather than Warning?

If so: when Elena added QPYLoadingDeprecatedFeatureWarning (#11260) we made it a subclass of UserWarning because we were basing it on what NumPy do with their VisibleDeprecationWarning. That's the only reason, I think - we mostly just trusted that whatever NumPy were doing was sensible.

@jakelishman
Copy link
Member Author

From offline discussion with Kevin: the question was actually along the lines of "why do we make it a warning instead of an error if an optional is found but fails to import?". The answer is that in a few cases, Qiskit only uses the optionals as accelerators and not as hard requirements. Examples of this are in the generation of approximation tables for the Solovay–Kitaev algorithm or in the FakeBackend.run methods, where (respectively) scikit-learn and Aer are used as accelerators, but Qiskit can fall back to internal-only methods in their absence.

The idea is that the warning triggers, then if the optional is required, a regular MissingOptionalLibraryError will be raised in addition right after.

Copy link
Contributor

@kevinhartman kevinhartman left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I'll wait to hit merge in case @Cryoris wants to take another look.

@Cryoris Cryoris added this pull request to the merge queue Jan 11, 2024
Merged via the queue into Qiskit:main with commit 4ce0049 Jan 11, 2024
13 checks passed
@jakelishman jakelishman deleted the tighter-lazy-tester branch January 11, 2024 12:49
ShellyGarion pushed a commit to ShellyGarion/qiskit-terra that referenced this pull request Jan 18, 2024
* Warn on import failures within optional dependencies

This distinguishes the case of "failed to find an optional dependency"
from "found the optional dependency, but it failed to import" within
the lazy testers.  Now, a warning containing the import failures will be
shown to users, rather than silently treating the dependency as missing.

This occurred recently when a PR caused an import failure in Aer, which
the CI suite failed to detect because the optional-dependency checkers
allowed the test suite to pass regardless.  Note that this commit alone
won't reliably cause the test suite to fail on a failed import because:

1. this only emits a warning, not an exception (by design).
2. the `unittest` decorators are evaluated during test discovery, which
   happens before we activate our increased warning filters.

This commit also unifies the now two Qiskit-specific warnings (the other
being `QPYLoadingDeprecatedFeatureWarning`) with a common
`QiskitWarning` subclass, much as we do for `QiskitError`.  This gives
us a convenient way to globally deny these errors during CI runs,
without turning _all_ `UserWarning`s into errors, which we're not ready
to do yet.

* Suppress unnecessary lint warnings

* Fix warning when failed import is parent package
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog type: qa Issues and PRs that relate to testing and code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants