Imports: Speed improvement for importing subpackages #5100

drewrisinger · 2020-09-22T12:34:54Z

What is the expected enhancement?

IMPORT SPEED of top-level `qiskit/init.py`

Currently, it takes on the order of seconds to import just the base qiskit package, and that doesn't improve if you only import subpackages (e.g. qiskit.pulse) due to the top-level import of heavy packages in ./qiskit/__init__.py, e.g. qiskit.providers.ibmq which are always imported.

Here, I am mostly looking in this issue at import speed improvements in qiskit/__init__.py.

Possible Import Improvements

I see a few possible solutions, ranked in order of more breaking changes:

(low-hanging fruit): defer matplotlib.animation import in qiskit.visualization.transition_visualization to the animation function
(low-hanging fruit): defer networkx import in qiskit.circuit.equivalence to the draw() and _build_basis_graph() (only used in draw()) functions so this isn't on the critical path every import.
Add an environment variable on the Aer & IBMQ provider imports to optionally disable them
Add an environment variable on the qiskit.circuit imports to optionally disable them. (e.g. QISKIT_DONT_IMPORT_TOP_LEVEL)

Import Time visualization

This can be checked visually with:

pip install tuna
pip install qiskit-terra qiskit-ibmq-provider
python -X importtime -c "import qiskit.pulse" 2> import_pulse.log
tuna import_pulse.log

Which yields (qiskit-terra==0.15.1, qiskit-ibmq-provider==0.8.0):

which is similar to that in #4958 (I must have a slower computer/not be running on SSD):

We see that import qiskit.pulse ends up importing the top-level qiskit init file anyways. As a side note, I'd really prefer not to spend ~3 seconds importing qiskit-terra every time.

The text was updated successfully, but these errors were encountered:

mtreinish · 2020-09-22T12:55:01Z

This a good direction, I've done similar analysis in the past and agree we can do better. For your proposed steps:

Lets just do this asap, it's straightforward and will be good to get in before 0.16.0. It's a good catch.
This actually will just shift the path, networkx gets used in the coupling map which is a core construct. The equivalence library drawer is just being imported first in your example. I have an outstanding todo to migrate the coupling map to retworkx, but there are some feature gaps in retworkx that need to be closed. (for the drawer though we could do something like Convert dag_drawer to use retworkx internally #5087 to avoid the networkx import there altogether).
Even with this we'll still end up importing them. The issue will just shift those imports to be in version.py to get qiskit.__qiskit_version__ set it imports all the elements to grab the __version__ attribute.
I don't know if this would solve an import performance issue. Even if we optional don't re-export the QuantumCircuit class other things we do import in the top level will pull in the circuits subpackage. For example, everything in transpile depends on circuit (to get bits, registers, etc). The same is true with dagcircuit and visualization.

The other thing which I think we can do here which was going to be a follow-up from #4958 as a multi-step process is to move matplotlib to not be imported at import time. The problem is which I found during #4958 is that doing this as I did there removes qiskit.visualization from the core import which was problematic for backwards compat. Then the other way to move the import is to make matplotlib a run time import everywhere, but this had issues because we export HAS_MATPLOTLIB and have for some time (and other projects may rely on this, I think ignis might). Either way it's used a lot of places in terra. So to do this we'll have to first deprecate HAS_MATPLOTLB and untangle the HAS_MATPLOTLIB usage everywhere. Then after that deprecation cycle we can migrate to run time matplotlib imports.

As laid out in Qiskit#5100, ``qiskit.visualization.transition_visualization`` is on the critical path for any ``qiskit`` import. This means that the imports in this file will always be imported. Profiling of import time showed that ``matplotlib`` was a slow import, and so should be deferred from the critical path.

As laid out in #5100, ``qiskit.visualization.transition_visualization`` is on the critical path for any ``qiskit`` import. This means that the imports in this file will always be imported. Profiling of import time showed that ``matplotlib`` was a slow import, and so should be deferred from the critical path. Co-authored-by: Kevin Krsulich <[email protected]>

drewrisinger · 2020-09-22T18:52:02Z

This isn't a top priority, but I found another leaf import that could potentially be removed/delayed. scipy.stats seems to only be used once in the repository
https://github.com/Qiskit/qiskit-terra/blob/19bf1a7b8d75616bd587f927a71f9b2eb8041dd4/qiskit/quantum_info/operators/random.py#L19
, and takes >= 0.1s of the import time (~8%). This could be solved with a similar approach as in #5102.

mariuskava · 2020-10-07T05:40:29Z

Did you consider using lazy imports? Sometimes it can make troubleshooting more tricky. But the behaviour could be controlled by env variable.

The scipy.stats submodule is a relatively slow import and takes up roughly 9% of our total import time. This is disproportionately large compares to its single use in the random operator generator functions which is not critical to any of the core functionality or performance critical. This commit shifts the module level import to be a runtime import to avoid this import cost, while this does add runtime overhead (and an initial cost on the first execution) that is worth the tradeoff to remove it from the default import path. Related Qiskit#5100

mtreinish · 2020-10-19T18:37:18Z

@drewrisinger yeah, scipy.stats was a good call. I pushed a PR to make that change in: #5258

I really want to push one to move matplotlib to be a lazy runtime import too. It'll probably meaning breaking anyone who is relying on qiskit.visualization.HAS_MATPLOTLIB today. Which might be a fair tradeoff, but after we drop python3.6 support (which the process for starts in #5189 ) we can use a module level getattr functions (which was added in 3.7) to do this more gracefully. So I'm a bit torn on whether to make the breaking change or just wait 6 months from the next release.

mtreinish · 2020-10-19T18:38:14Z

Also, we should be able to remove networkx from the import path (which is surprisingly not cheap either) after #5183 merges which will speed things up a bit more

* Move scipy.stats imports to runtime The scipy.stats submodule is a relatively slow import and takes up roughly 9% of our total import time. This is disproportionately large compares to its single use in the random operator generator functions which is not critical to any of the core functionality or performance critical. This commit shifts the module level import to be a runtime import to avoid this import cost, while this does add runtime overhead (and an initial cost on the first execution) that is worth the tradeoff to remove it from the default import path. Related #5100 * For probability_distributions circuit library too Co-authored-by: Kevin Krsulich <[email protected]>

* Move scipy.stats imports to runtime The scipy.stats submodule is a relatively slow import and takes up roughly 9% of our total import time. This is disproportionately large compares to its single use in the random operator generator functions which is not critical to any of the core functionality or performance critical. This commit shifts the module level import to be a runtime import to avoid this import cost, while this does add runtime overhead (and an initial cost on the first execution) that is worth the tradeoff to remove it from the default import path. Related #5100 * For probability_distributions circuit library too Co-authored-by: Kevin Krsulich <[email protected]> (cherry picked from commit c7c110d)

* Move scipy.stats imports to runtime The scipy.stats submodule is a relatively slow import and takes up roughly 9% of our total import time. This is disproportionately large compares to its single use in the random operator generator functions which is not critical to any of the core functionality or performance critical. This commit shifts the module level import to be a runtime import to avoid this import cost, while this does add runtime overhead (and an initial cost on the first execution) that is worth the tradeoff to remove it from the default import path. Related #5100 * For probability_distributions circuit library too Co-authored-by: Kevin Krsulich <[email protected]> (cherry picked from commit c7c110d) Co-authored-by: Matthew Treinish <[email protected]>

This commit removes the networkx usage in the ApproximateTokenSwapper to use retworkx instead. Besides being speeding up the execution of the pass what's more important here is that this was the last networkx usage in qiskit-terra (with the exeception of a couple compatibility converters in qiskit.dagcircuit). So this commit also makes networkx an optional dependency and updates the 3 methods (which are outside the common usage scenarios) to use runtime imports and emit a more detailed import error if networkx isn't installed. Fixes: Qiskit#5470 Related to: Qiskit#5100

…x optional (#5471) * Migrate ApproximateTokenSwapper to only use retworkx This commit removes the networkx usage in the ApproximateTokenSwapper to use retworkx instead. Besides being speeding up the execution of the pass what's more important here is that this was the last networkx usage in qiskit-terra (with the exeception of a couple compatibility converters in qiskit.dagcircuit). So this commit also makes networkx an optional dependency and updates the 3 methods (which are outside the common usage scenarios) to use runtime imports and emit a more detailed import error if networkx isn't installed. Fixes: #5470 Related to: #5100 * Remove unecessary duplicate None check * Add networkx to requirements-dev.txt There is a single dagcircuit test that uses the to_networkx() method. So we need to install networkx in test environments. * Remove _T type * Update releasenotes/notes/nx-optional-6b8ad63529fe32ca.yaml Co-authored-by: Lauren Capelluto <[email protected]> * Remove _V type Co-authored-by: Lauren Capelluto <[email protected]>

mtreinish · 2020-12-08T15:37:17Z

After #5471 and #5485 the import performance here is a lot better. The only 2 things after #5485 is finished and merges that will be good to do is remove the fastjsonschema import time compilation (which is deprecated and is just a matter of time) and then something for __qiskit_version__. __qiskit_version__ is a bit more involved to do lazily because it's defined as a module attribute that's a dict. So the hack I used for HAS_MATPLOTLIB won't work because there is no hidden method being called when it's accessed. We won't have module __getattr__ on modules until Python 3.7 is the minimum supported version.

This commit moves the import of pydot and pillow import that were happening at the module level to be runtime imports. Both of these imports are only used if you're visualizing an equivalence library, which is normally only used for debugging the transpiler and basis translator which is not a common usage scenario. While pillow is pretty quick to import since it's mostly a C FFI package with a bit of stdlib xml. pydot on the other hand is a bit slower to import taking ~3% of the import time. While neither is a a huge bottleneck it's something we can remove from the default import path unless they're actually needed. Related to: Qiskit#5100

* Move equivalence drawer visualization imports to runtime This commit moves the import of pydot and pillow import that were happening at the module level to be runtime imports. Both of these imports are only used if you're visualizing an equivalence library, which is normally only used for debugging the transpiler and basis translator which is not a common usage scenario. While pillow is pretty quick to import since it's mostly a C FFI package with a bit of stdlib xml. pydot on the other hand is a bit slower to import taking ~3% of the import time. While neither is a a huge bottleneck it's something we can remove from the default import path unless they're actually needed. Related to: #5100 * Fix lint Co-authored-by: Kevin Krsulich <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

This commit migrates the qiskit.Aer and qiskit.IBMQ module attributes to be lazy loading instances of lazy loading wrapper classes. The intent here is to avoid importing from qiskit-aer or qiskit-ibmq-provider from qiskit-terra, while this is safe while the packages share a shared namespace we've been actively working to remove the use of namespace packaging (see Qiskit#5089, Qiskit#4767, and Qiskit#559) and the circular dependency caused by re-exporting these attributes is blocking progress on this. By using a lazy loading wrapper class we avoid an import type circular dependency and opportunistically use qiskit-aer and/or qiskit-ibmq-provider at runtime only if they're present after everything is imported. This also may have some benefit for the overall import performance of qiskit (being tracked in Qiskit#5100) as it removes qiskit-aer and qiskit-ibmq-provider from the import path unless they're being used. Although the presence of qiskit.__qiskit_version__ might prevent any performance improvements as it still imports all the elements to get version information (and will be tackled in a separate PR). Fixes Qiskit#5532

This commit migrates the __qiskit_version__ module attribute away from using a dict constructed at import time to being an instance of a lazy loading class that will import the elements to get their version information only at run time if needed. This will improve the overall import performance and also reduce the possibility of introducing an import cycle when we remove namespace packaging. Related to Qiskit#5100 and Qiskit#5532

* Add a lazy loading wrapper class for __qiskit_version__ This commit migrates the __qiskit_version__ module attribute away from using a dict constructed at import time to being an instance of a lazy loading class that will import the elements to get their version information only at run time if needed. This will improve the overall import performance and also reduce the possibility of introducing an import cycle when we remove namespace packaging. Related to #5100 and #5532 * Rename qiskit.execute module to qiskit.execute_function The qiskit.execute module has a name conflict with the re-exported path to it's sole public function qiskit.execute(). Whether python uses the module or the function is an import time race condition and by making the element imports occur at runtime this switches the normal evaluation to use the module instead of the function. However, this was always a bug and could have been triggered in other ways too. To make the imports deterministic either the function or the module needs to be renamed and since the module is less commonly used than the function that is renamed. * Rename qiskit.compiler inner modules to avoid name conflict The qiskit.compiler module has a name conflict with the re-exported path to public functions qiskit.compiler. Whether python uses the module or the function is an import time race condition, amd could be triggered by subtle changes in import evaluation order. To make the imports deterministic either the function or the module needs to be renamed and since the module is less commonly used than the function that is renamed. * Fix docs build * Apply suggestions from code review Co-authored-by: Kevin Krsulich <[email protected]>

mtreinish · 2021-02-02T22:22:31Z

I think after #5619 and #5485 are merged we can close this issue as fixed. There is also #5582 which will hopefully help prevent regressions on this in the future after we have this fixed.

drewrisinger · 2021-02-03T13:44:36Z

Thanks @mtreinish for being responsive and the tedious work that goes into this!

* Wrap qiskit.Aer and qiskit.IBMQ with lazy loading object This commit migrates the qiskit.Aer and qiskit.IBMQ module attributes to be lazy loading instances of lazy loading wrapper classes. The intent here is to avoid importing from qiskit-aer or qiskit-ibmq-provider from qiskit-terra, while this is safe while the packages share a shared namespace we've been actively working to remove the use of namespace packaging (see #5089, #4767, and #559) and the circular dependency caused by re-exporting these attributes is blocking progress on this. By using a lazy loading wrapper class we avoid an import type circular dependency and opportunistically use qiskit-aer and/or qiskit-ibmq-provider at runtime only if they're present after everything is imported. This also may have some benefit for the overall import performance of qiskit (being tracked in #5100) as it removes qiskit-aer and qiskit-ibmq-provider from the import path unless they're being used. Although the presence of qiskit.__qiskit_version__ might prevent any performance improvements as it still imports all the elements to get version information (and will be tackled in a separate PR). Fixes #5532 * Fix lint * Fix test lint * DNM: test with ignis patch * Revert "DNM: test with ignis patch" This reverts commit ac9611c. * Use ignis from source for tutorial job * Update release note to be more clear

mtreinish · 2021-02-05T18:57:58Z

ok, I'm going to close this as all 3 of those PRs have merged now. To wrap it I just ran an importtime profile of import qiskit with all optional dependencies and all the qiskit components installed and this is what it returned:

which I think is a good enough improvement to go from ~1.1 sec to ~0.49 sec.

With all the jsonschema usage currently being deprecated, >=3 months after 0.17.0's release when we remove the jsonschema bits we'll shave another ~0.05 sec off the import time. There is also cvxpy in there which is only used in one place and takes ~0.06 sec to import. But both of those times seem pretty minor. We can open up follow up issues or PRs if they get to be a bottleneck for people.

* Wrap qiskit.Aer and qiskit.IBMQ with lazy loading object This commit migrates the qiskit.Aer and qiskit.IBMQ module attributes to be lazy loading instances of lazy loading wrapper classes. The intent here is to avoid importing from qiskit-aer or qiskit-ibmq-provider from qiskit-terra, while this is safe while the packages share a shared namespace we've been actively working to remove the use of namespace packaging (see Qiskit#5089, Qiskit#4767, and Qiskit#559) and the circular dependency caused by re-exporting these attributes is blocking progress on this. By using a lazy loading wrapper class we avoid an import type circular dependency and opportunistically use qiskit-aer and/or qiskit-ibmq-provider at runtime only if they're present after everything is imported. This also may have some benefit for the overall import performance of qiskit (being tracked in Qiskit#5100) as it removes qiskit-aer and qiskit-ibmq-provider from the import path unless they're being used. Although the presence of qiskit.__qiskit_version__ might prevent any performance improvements as it still imports all the elements to get version information (and will be tackled in a separate PR). Fixes Qiskit#5532 * Fix lint * Fix test lint * DNM: test with ignis patch * Revert "DNM: test with ignis patch" This reverts commit ac9611c. * Use ignis from source for tutorial job * Update release note to be more clear

…it#5619) * Wrap qiskit.Aer and qiskit.IBMQ with lazy loading object This commit migrates the qiskit.Aer and qiskit.IBMQ module attributes to be lazy loading instances of lazy loading wrapper classes. The intent here is to avoid importing from qiskit-aer or qiskit-ibmq-provider from qiskit-terra, while this is safe while the packages share a shared namespace we've been actively working to remove the use of namespace packaging (see Qiskit/qiskit#5089, Qiskit/qiskit#4767, and Qiskit/qiskit#559) and the circular dependency caused by re-exporting these attributes is blocking progress on this. By using a lazy loading wrapper class we avoid an import type circular dependency and opportunistically use qiskit-aer and/or qiskit-ibmq-provider at runtime only if they're present after everything is imported. This also may have some benefit for the overall import performance of qiskit (being tracked in Qiskit/qiskit#5100) as it removes qiskit-aer and qiskit-ibmq-provider from the import path unless they're being used. Although the presence of qiskit.__qiskit_version__ might prevent any performance improvements as it still imports all the elements to get version information (and will be tackled in a separate PR). Fixes Qiskit/qiskit#5532 * Fix lint * Fix test lint * DNM: test with ignis patch * Revert "DNM: test with ignis patch" This reverts commit ac9611c3ace30d101a70bda06e1987df14662182. * Use ignis from source for tutorial job * Update release note to be more clear

…t#5620) * Add a lazy loading wrapper class for __qiskit_version__ This commit migrates the __qiskit_version__ module attribute away from using a dict constructed at import time to being an instance of a lazy loading class that will import the elements to get their version information only at run time if needed. This will improve the overall import performance and also reduce the possibility of introducing an import cycle when we remove namespace packaging. Related to Qiskit/qiskit#5100 and Qiskit/qiskit#5532 * Rename qiskit.execute module to qiskit.execute_function The qiskit.execute module has a name conflict with the re-exported path to it's sole public function qiskit.execute(). Whether python uses the module or the function is an import time race condition and by making the element imports occur at runtime this switches the normal evaluation to use the module instead of the function. However, this was always a bug and could have been triggered in other ways too. To make the imports deterministic either the function or the module needs to be renamed and since the module is less commonly used than the function that is renamed. * Rename qiskit.compiler inner modules to avoid name conflict The qiskit.compiler module has a name conflict with the re-exported path to public functions qiskit.compiler. Whether python uses the module or the function is an import time race condition, amd could be triggered by subtle changes in import evaluation order. To make the imports deterministic either the function or the module needs to be renamed and since the module is less commonly used than the function that is renamed. * Fix docs build * Apply suggestions from code review Co-authored-by: Kevin Krsulich <[email protected]>

drewrisinger added the type: enhancement It's working, but needs polishing label Sep 22, 2020

mtreinish added the performance label Sep 22, 2020

drewrisinger mentioned this issue Sep 22, 2020

Transition Visualization: defer matplotlib import #5102

Merged

drewrisinger mentioned this issue Oct 8, 2020

python3Packages.qiskit: 0.20.0 -> 0.23.1, update dependencies & subpackages NixOS/nixpkgs#100007

Merged

10 tasks

mtreinish mentioned this issue Oct 19, 2020

Move scipy.stats imports to runtime #5258

Merged

mtreinish mentioned this issue Dec 4, 2020

Migrate ApproximateTokenSwapper to retworkx #5470

Closed

mtreinish mentioned this issue Dec 4, 2020

Migrate ApproximateTokenSwapper to only use retworkx and make networkx optional #5471

Merged

mtreinish mentioned this issue Dec 8, 2020

Move matplotlib imports to runtime #5485

Merged

mtreinish mentioned this issue Dec 8, 2020

Move equivalence drawer visualization imports to runtime #5488

Merged

mtreinish mentioned this issue Dec 15, 2020

Avoid potential import cycles from Aer and IBMQ #5532

Closed

mtreinish mentioned this issue Jan 12, 2021

Wrap qiskit.Aer and qiskit.IBMQ with lazy loading object #5619

Merged

mtreinish mentioned this issue Jan 13, 2021

Add a lazy loading wrapper class for __qiskit_version__ #5620

Merged

mtreinish closed this as completed Feb 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Imports: Speed improvement for importing subpackages #5100

Imports: Speed improvement for importing subpackages #5100

drewrisinger commented Sep 22, 2020

mtreinish commented Sep 22, 2020

drewrisinger commented Sep 22, 2020

mariuskava commented Oct 7, 2020

mtreinish commented Oct 19, 2020

mtreinish commented Oct 19, 2020

mtreinish commented Dec 8, 2020

mtreinish commented Feb 2, 2021

drewrisinger commented Feb 3, 2021

mtreinish commented Feb 5, 2021

Imports: Speed improvement for importing subpackages #5100

Imports: Speed improvement for importing subpackages #5100

Comments

drewrisinger commented Sep 22, 2020

What is the expected enhancement?

IMPORT SPEED of top-level qiskit/__init__.py

Possible Import Improvements

Import Time visualization

mtreinish commented Sep 22, 2020

drewrisinger commented Sep 22, 2020

mariuskava commented Oct 7, 2020

mtreinish commented Oct 19, 2020

mtreinish commented Oct 19, 2020

mtreinish commented Dec 8, 2020

mtreinish commented Feb 2, 2021

drewrisinger commented Feb 3, 2021

mtreinish commented Feb 5, 2021

IMPORT SPEED of top-level `qiskit/init.py`