-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Optimise import qiskit
with lazy imports
#7525
Conversation
This introduces new `HAS_X` variables for each of Qiskit's optional dependencies, and provides a simple unified interface to them from `qiskit.utils.optionals`. These objects lazily test for their dependency when evaluated in a Boolean context, and have two `require_` methods to unify the exception handling. `require_now` tests immediately for the dependency and raises `MissingOptionalLibraryError` if it is not present, and `require_in_call` is a decorator that lazily tests for the dependencies when the function is called. These remove the burden of raising nice exceptions from the usage points; a function marked `HAS_MATPLOTLIB.require_in_call` can now safely `import matplotlib` without special handling, for example. This also provides a unified way for consumers of `qiskit` (such as the test suite) to query the presence of libraries. All tests are now lazy, and imports are moved to the point of usage, not the point of import of the module. This means that `import qiskit` is significantly faster for people who have many of the optional dependencies installed; rather than them all being loaded at initial import just to test their presence, they will now be loaded on demand.
This makes several imports lazy, only being imported when they are actually called and used. In particular, no component of `scipy` is imported during `import qiskit` now, nor is `pkg_resources` (which is surprisingly heavy). No changes were made to algorithms or opflow, since these are not immediately imported during `import qiskit`, and likely require more significant work than the rest of the library.
I guess one downside of the new way, from a user perspective, I write code with a bunch of imports and it all seems fine as it loads and starts running. But instead of it failing fast as it used to, I now find it fails some time later if some optional dependency is not present when it finally goes to construct/uses something. The code may have expended significant computational resources by that point. And I might fix that and run again, go further and hit another missing optional. The fail early that was done before would catch the optional dependence early; but has other considerations that you point out such as loading resources that are never used in practice. |
This doesn't actually change anything about that - before, all the imports would silently fail immediately, setting (e.g.) |
Ah right, as you say the behavior in terms of it failing later is unaltered by your change. I know in Aqua we used to indicate the failure to load but guess that was removed as it may have been too noisy if you did not care about installing the optional. |
Yeah, I don't know when it changed, but it was probably the right call - this PR identified 26 different optional dependencies across Terra alone. The code from Aqua seems to be responsible for about 4 of them, which is a bit more reasonable, but still, they're pretty niche optimisers, and I'd imagine most people wouldn't be bothered. In general, you should get the error when you instantiate the optimiser classes, which is likely going to be one of the first things you do. |
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.
Awesome thanks for doing this. The API around managing optional dependencies is so much cleaner here and much easier to manage than the ad-hoc solution we were doing before. It's nice enough that I'm slightly tempted to violate the stable branch policy so we can start using this in downstream qiskit packages sooner rather than later :) That coupled with fixing the stray debug logging and the import time improvements this really nice!
Just have a few questions and comments inline
# This weird duplicated lazy structure is for backwards compatibility; Qiskit has historically | ||
# always made ``two_qubit_cnot_decompose`` available publicly immediately on import, but it's quite | ||
# expensive to construct, and we want to defer the obejct's creation until it's actually used. We | ||
# only need to pass through the public methods that take `self` as a parameter. Using `__getattr__` | ||
# doesn't work because it is only called if the normal resolution methods fail. Using | ||
# `__getattribute__` is too messy for a simple one-off use object. |
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.
Oh, too bad I was excited when I opened #7498 that we'd have the first use case for module level getattr now that we're >=3.7 but I guess not. Having this comment here is useful for explaining this I should have added one for the lazy aer and ibmq wrappers
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.
Oh, I forgot that module-level getattr
was available now - the getattr
I'm referring to here is the class one (i.e. why I defined all the methods manually). I think it might end up a bit convoluted here - it also needs to be reachable from qiskit.quantum_info.synthesis
, and we'd need to protect an internal Qiskit import in other places, which would be a bit weird.
@@ -79,6 +79,7 @@ | |||
RemoveDiagonalGatesBeforeMeasure | |||
RemoveResetInZeroState | |||
CrosstalkAdaptiveSchedule | |||
HoareOptimizer |
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.
This is a good catch but kind of an unrelated change.
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.
Oh yeah, it's because I put in a Sphinx cross-ref to it somewhere (I think in the HAS_Z3
docs?), and it didn't link correctly - it looks totally unrelated, but there was some method in the madness.
@@ -127,11 +127,13 @@ | |||
from qiskit.visualization.transition_visualization import visualize_transition | |||
from qiskit.visualization.array import array_to_latex | |||
|
|||
from .circuit_visualization import circuit_drawer, HAS_PIL, HAS_PDFLATEX, HAS_PDFTOCAIRO | |||
# NOTE (Jake Lishman, 2022-01-12): for backwards compatibility. Deprecate these paths in Terra 0.21. |
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.
Is there a reason to not deprecate these now?
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.
The alternate paths aren't available in Terra 0.19, so we can't warn until the alternative is already in place, and I know that both Ignis and ibmq-provider import HAS_MATPLOTLIB
from here (technically qiskit.tools.visualization
, but that's just a pass-through to here).
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.
Wow, something is still using qiskit.tools.visualization
we made that move like 3 years ago. :)
Effectively this is just a wrapper around `__init__`, except that this class-decorator form will do the right thing even if `__init__` isn't explicitly defined on the given class. The implementation of `wrap_method` is a replacement for the older `test.decorators._wrap_method`, which didn't handle all the possible special cases as well, and messed up the documentation of its wrapped functions. That wasn't so important when it was just a private function, but now it has become public (so that `test.decorators.enforce_subclasses_call` can still use it from `qiskit.utils`), it needed reworking to be more polished.
A couple of lines in |
Added a section into The dependency-checker classes themselves I suppose could be split off into a separate package, but there would be some additional work around making the error handling nice and extensible there. I think the majority of the benefit comes from defining all the |
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.
This LGTM. I think everything is covered here. Thanks for doing this. I left a few questions/comments inline but nothing work blocking this over. Some things we can always adjust in follow ups as necessary, especially given the size of this PR it'll be good to merge this sooner rather than later.
return types.MethodType(self._method, obj) | ||
|
||
|
||
class _WrappedMethod: |
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.
Heh, I appreciate the thoroughness on this class and the details look good to me. I'm just wondering if these edge cases came up in practice in the use of the class decorator, or this was just to be defensive against something needs them currently. Like I guess I'm wondering if something is trying to use _WrappedMethod
on a dict
subclass or something.
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.
They come up far more than you'd expect - things like wrapping method from type
and object
are part of the core usage - wrapping a non-existent __init__
needs to access something from object
, and wrapping __init_subclass__
(which we do in the test classes) needs to access something from type
. Wrapping classmethod
and staticmethod
instances I think already happens in what we use it for now.
Using builtins as the wrappers happened when I was designing the class in the interpreter - I tried to use print
as a wrapper for a really poor man's debugger.
@@ -479,7 +476,7 @@ def test_state_hessian(self, method): | |||
state_hess.assign_parameters(value_dict).eval(), correct_values[i], decimal=1 | |||
) | |||
|
|||
@unittest.skipIf(not _HAS_JAX, "Skipping test due to missing jax module.") | |||
@unittest.skipIf(not optionals.HAS_JAX, "Skipping test due to missing jax module.") |
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.
A potential follow-on, not something we should do in this pr. Is to add a skip test decorator to the lazy importer class. So we can just make this @optionals.HAS_JAX.test_requires
or something
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.
I'm not 100% sure on that - it would couple the objects to our testing framework, and that's not ideal for something we're publicly exporting.
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.
Well I was thinking more just having the decorator raise unittest.SkipTest
which is in stdlib and while it's specifically part of unittest framework I think all the python testing frameworks support it. But it's not critical and I agree a gray area of whether we want that as part of the api. We can discuss it later if we think it's something worthwhile
if test_generator(): | ||
self.fail("did not evaluate false") |
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.
Is there an advantage to this vs L71? This should be equivalent to self.assertFalse(test_generator())
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.
Yeah, it's a slightly superfluous test - I'm not 100% sure what I was thinking, but I suspect it was along the lines of "check a few possible implicit Boolean contexts". Strictly, all three tests here should be identical, I think. Happy to remove it (and the similar test on line 65 if you'd prefer to reduce noise.
Marking this as on hold until we get all the 0.19.2 PRs backported (just to avoid potential merge conflicts). After we're ready to release 0.19.2 I'll tag this as automerge. |
* Unify lazy handling of optional dependencies This introduces new `HAS_X` variables for each of Qiskit's optional dependencies, and provides a simple unified interface to them from `qiskit.utils.optionals`. These objects lazily test for their dependency when evaluated in a Boolean context, and have two `require_` methods to unify the exception handling. `require_now` tests immediately for the dependency and raises `MissingOptionalLibraryError` if it is not present, and `require_in_call` is a decorator that lazily tests for the dependencies when the function is called. These remove the burden of raising nice exceptions from the usage points; a function marked `HAS_MATPLOTLIB.require_in_call` can now safely `import matplotlib` without special handling, for example. This also provides a unified way for consumers of `qiskit` (such as the test suite) to query the presence of libraries. All tests are now lazy, and imports are moved to the point of usage, not the point of import of the module. This means that `import qiskit` is significantly faster for people who have many of the optional dependencies installed; rather than them all being loaded at initial import just to test their presence, they will now be loaded on demand. * Optimise time taken for `import qiskit` This makes several imports lazy, only being imported when they are actually called and used. In particular, no component of `scipy` is imported during `import qiskit` now, nor is `pkg_resources` (which is surprisingly heavy). No changes were made to algorithms or opflow, since these are not immediately imported during `import qiskit`, and likely require more significant work than the rest of the library. * Import missing to-be-deprecated names * Convert straggler tests to require_now * Correct requirements in test cases * Add `require_in_instance` class decorator Effectively this is just a wrapper around `__init__`, except that this class-decorator form will do the right thing even if `__init__` isn't explicitly defined on the given class. The implementation of `wrap_method` is a replacement for the older `test.decorators._wrap_method`, which didn't handle all the possible special cases as well, and messed up the documentation of its wrapped functions. That wasn't so important when it was just a private function, but now it has become public (so that `test.decorators.enforce_subclasses_call` can still use it from `qiskit.utils`), it needed reworking to be more polished. * Privatise non-public names rather than del * Add tests of `require_in_instance` * Fix typos in documentation * Add section on requirements to CONTRIBUTING * Update documentation on HoareOptimizer error * Remove UK localisation * Mention more uses of PIL in documentation Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
* Unify lazy handling of optional dependencies This introduces new `HAS_X` variables for each of Qiskit's optional dependencies, and provides a simple unified interface to them from `qiskit.utils.optionals`. These objects lazily test for their dependency when evaluated in a Boolean context, and have two `require_` methods to unify the exception handling. `require_now` tests immediately for the dependency and raises `MissingOptionalLibraryError` if it is not present, and `require_in_call` is a decorator that lazily tests for the dependencies when the function is called. These remove the burden of raising nice exceptions from the usage points; a function marked `HAS_MATPLOTLIB.require_in_call` can now safely `import matplotlib` without special handling, for example. This also provides a unified way for consumers of `qiskit` (such as the test suite) to query the presence of libraries. All tests are now lazy, and imports are moved to the point of usage, not the point of import of the module. This means that `import qiskit` is significantly faster for people who have many of the optional dependencies installed; rather than them all being loaded at initial import just to test their presence, they will now be loaded on demand. * Optimise time taken for `import qiskit` This makes several imports lazy, only being imported when they are actually called and used. In particular, no component of `scipy` is imported during `import qiskit` now, nor is `pkg_resources` (which is surprisingly heavy). No changes were made to algorithms or opflow, since these are not immediately imported during `import qiskit`, and likely require more significant work than the rest of the library. * Import missing to-be-deprecated names * Convert straggler tests to require_now * Correct requirements in test cases * Add `require_in_instance` class decorator Effectively this is just a wrapper around `__init__`, except that this class-decorator form will do the right thing even if `__init__` isn't explicitly defined on the given class. The implementation of `wrap_method` is a replacement for the older `test.decorators._wrap_method`, which didn't handle all the possible special cases as well, and messed up the documentation of its wrapped functions. That wasn't so important when it was just a private function, but now it has become public (so that `test.decorators.enforce_subclasses_call` can still use it from `qiskit.utils`), it needed reworking to be more polished. * Privatise non-public names rather than del * Add tests of `require_in_instance` * Fix typos in documentation * Add section on requirements to CONTRIBUTING * Update documentation on HoareOptimizer error * Remove UK localisation * Mention more uses of PIL in documentation Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
* Unify lazy handling of optional dependencies This introduces new `HAS_X` variables for each of Qiskit's optional dependencies, and provides a simple unified interface to them from `qiskit.utils.optionals`. These objects lazily test for their dependency when evaluated in a Boolean context, and have two `require_` methods to unify the exception handling. `require_now` tests immediately for the dependency and raises `MissingOptionalLibraryError` if it is not present, and `require_in_call` is a decorator that lazily tests for the dependencies when the function is called. These remove the burden of raising nice exceptions from the usage points; a function marked `HAS_MATPLOTLIB.require_in_call` can now safely `import matplotlib` without special handling, for example. This also provides a unified way for consumers of `qiskit` (such as the test suite) to query the presence of libraries. All tests are now lazy, and imports are moved to the point of usage, not the point of import of the module. This means that `import qiskit` is significantly faster for people who have many of the optional dependencies installed; rather than them all being loaded at initial import just to test their presence, they will now be loaded on demand. * Optimise time taken for `import qiskit` This makes several imports lazy, only being imported when they are actually called and used. In particular, no component of `scipy` is imported during `import qiskit` now, nor is `pkg_resources` (which is surprisingly heavy). No changes were made to algorithms or opflow, since these are not immediately imported during `import qiskit`, and likely require more significant work than the rest of the library. * Import missing to-be-deprecated names * Convert straggler tests to require_now * Correct requirements in test cases * Add `require_in_instance` class decorator Effectively this is just a wrapper around `__init__`, except that this class-decorator form will do the right thing even if `__init__` isn't explicitly defined on the given class. The implementation of `wrap_method` is a replacement for the older `test.decorators._wrap_method`, which didn't handle all the possible special cases as well, and messed up the documentation of its wrapped functions. That wasn't so important when it was just a private function, but now it has become public (so that `test.decorators.enforce_subclasses_call` can still use it from `qiskit.utils`), it needed reworking to be more polished. * Privatise non-public names rather than del * Add tests of `require_in_instance` * Fix typos in documentation * Add section on requirements to CONTRIBUTING * Update documentation on HoareOptimizer error * Remove UK localisation * Mention more uses of PIL in documentation Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
* Unify lazy handling of optional dependencies This introduces new `HAS_X` variables for each of Qiskit's optional dependencies, and provides a simple unified interface to them from `qiskit.utils.optionals`. These objects lazily test for their dependency when evaluated in a Boolean context, and have two `require_` methods to unify the exception handling. `require_now` tests immediately for the dependency and raises `MissingOptionalLibraryError` if it is not present, and `require_in_call` is a decorator that lazily tests for the dependencies when the function is called. These remove the burden of raising nice exceptions from the usage points; a function marked `HAS_MATPLOTLIB.require_in_call` can now safely `import matplotlib` without special handling, for example. This also provides a unified way for consumers of `qiskit` (such as the test suite) to query the presence of libraries. All tests are now lazy, and imports are moved to the point of usage, not the point of import of the module. This means that `import qiskit` is significantly faster for people who have many of the optional dependencies installed; rather than them all being loaded at initial import just to test their presence, they will now be loaded on demand. * Optimise time taken for `import qiskit` This makes several imports lazy, only being imported when they are actually called and used. In particular, no component of `scipy` is imported during `import qiskit` now, nor is `pkg_resources` (which is surprisingly heavy). No changes were made to algorithms or opflow, since these are not immediately imported during `import qiskit`, and likely require more significant work than the rest of the library. * Import missing to-be-deprecated names * Convert straggler tests to require_now * Correct requirements in test cases * Add `require_in_instance` class decorator Effectively this is just a wrapper around `__init__`, except that this class-decorator form will do the right thing even if `__init__` isn't explicitly defined on the given class. The implementation of `wrap_method` is a replacement for the older `test.decorators._wrap_method`, which didn't handle all the possible special cases as well, and messed up the documentation of its wrapped functions. That wasn't so important when it was just a private function, but now it has become public (so that `test.decorators.enforce_subclasses_call` can still use it from `qiskit.utils`), it needed reworking to be more polished. * Privatise non-public names rather than del * Add tests of `require_in_instance` * Fix typos in documentation * Add section on requirements to CONTRIBUTING * Update documentation on HoareOptimizer error * Remove UK localisation * Mention more uses of PIL in documentation Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Summary
import qiskit
significantly - 1.17s to 0.57s (2x faster) for a Python environment with almost all the optional dependencies, and 0.78s to 0.52s (33% faster) for a Python environment with only Terra and its requirements.qiskit.utils.optionals
, with many lazy-evaluationHAS_X
variables (e.g.HAS_MATPLOTLIB
,HAS_AER
), which can be used for lazy testing of optional dependencies, and unified handling of errors if they are not present.Details and comments
These two commits significantly speed up
import qiskit
by deferring many imports to their call sites. In particular, no component of Scipy is loaded, nor ispkg_resources
(which is surprisingly heavy). I didn't change anyscipy
imports in algorithms or opflow, because these aren't imported by default, and I thought they might be a bit more work for lesser benefit.The first commit in this patch also introduces many new
HAS_X
variables, to unify handling of optional dependencies all the way through the codebase. Previously, many modules were doing ad-hoc testing withThis form has a couple of downsides:
import qiskit
if the dependency is installed, even if it won't be used.The first commit introduces some new lazy-loader objects (
LazyImportTester
andLazySubprocessTester
), which can be evaluated as Booleans, and will lazily test the presence of the dependencies. This is similar to the oldHAS_MATPLOTLIB
variable. There is a new moduleqiskit.utils.optionals
that exposes manyHAS_X
variables of these types. In addition to the lazy Boolean checkers, they also come with unified error handling: modules now can doBoth
require
forms will raise a suitableMissingOptionalLibraryError
if the dependency is not available, at the point of call - the decorator raises when the inner function is called, not when the decorator is applied.Moving the variables into
qiskit.utils.optionals
is also nice for the test suite - instead of it having to repeat the code or import names from random locations, there's a simple namespace where all the objects exist (with documentation on each one in Sphinx).Fix #7498, as a side-effect - all the instances where initialisation code was logging are now deferred to their usage sites, so the logs will actually make sense and be useful. You can import every single module defined in Terra without us issuing a log message now.