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

Move matplotlib imports to runtime #5485

Merged
merged 9 commits into from
Feb 4, 2021
Merged

Conversation

mtreinish
Copy link
Member

@mtreinish mtreinish commented Dec 8, 2020

Summary

This commit moves all the matplotlib imports to runtime. Prior to this
commit when importing the qiskit namespace matplotlib would always be
imported if it was installed. This however adds a noticeable bottleneck
to the import time, even if visualization isn't used. To avoid this
overhead we can lazily import matplotlib at runtime when it is
available. This commit makes this change so that matplotlib is only
imported at runtime when visualization methods using it are called. This
does slow down the first visualization function call using matplotlib,
but since visualization isn't a performance critical path this an
acceptable tradeoff.

For backwards compibility the HAS_MATPLOTLIB module attribute has
changed from a bool to an object of a new class HasMatplotlibWrapper,
which implements a single method __bool__ which returns true if
matplotlib is installed, otherwise it's false. However it does this at
runtime instead of at import time. This way it be used as a replacement
for the previous HAS_MATPLOTLIB boolean variable but without forcing
matplotlib be imported with the entire package.

Details and comments

Related to: #5100

This commit moves all the matplotlib imports to runtime. Prior to this
commit when importing the qiskit namespace matplotlib would always be
imported if it was installed. This however adds a noticeable bottleneck
to the import time, even if visualization isn't used. To avoid this
overhead we can lazily import matplotlib at runtime when it is
available. This commit makes this change so that matplotlib is only
imported at runtime when visualization methods using it are called. This
does slow down the first visualization function call using matplotlib,
but since visualization isn't a performance critical path this an
acceptable tradeoff.

For backwards compibility the HAS_MATPLOTLIB module attribute has
changed from a bool to an object of a new class HasMatplotlibWrapper,
which implements a single method __bool__ which returns true if
matplotlib is installed, otherwise it's false. However it does this at
runtime instead of at import time. This way it be used as a replacement
for the previous HAS_MATPLOTLIB boolean variable but without forcing
matplotlib be imported with the entire package.
@mtreinish
Copy link
Member Author

Looking at the import times without this PR the profile looks like:

Screenshot_2020-12-08_10-05-59

Then applying this PR speeds up the import noticeably to:

Screenshot_2020-12-08_10-06-29

After this PR the only easy thing left is to remove the fastjsonschema usage. That is just a matter of time since it is deprecated (although only on master so the timer starts after the next release).

@mtreinish mtreinish removed the on hold Can not fix yet label Jan 27, 2021
@mtreinish mtreinish added this to the 0.17 milestone Jan 27, 2021
@Cryoris Cryoris self-assigned this Feb 2, 2021
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.

Just one question, otherwise I think this looks good.

@@ -175,7 +173,7 @@ def pulse_drawer(data: Union[Waveform, Union[Schedule, Instruction]],

if get_backend() in ['module://ipykernel.pylab.backend_inline',
'nbAgg']:
_matplotlib.plt.close(image)
plt.close(image)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is calling close on qiskit.visualization.pulse.matplotlib (which it was before) the same as calling it on matplotlib (which it is now)?

Copy link
Member Author

@mtreinish mtreinish Feb 4, 2021

Choose a reason for hiding this comment

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

It should be, matplotlib.pyplot is a global state instance.

That being said this comment reminded me that I need to check the pulse v2 drawer because I originally wrote this before that merged. Let me do a quick pass on that now and verify that matplotlib doesn't get imported at import time from there.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, just double checked and it looks like @nkanazawa1989 already made the matplotlib imports occur at runtime for the v2 drawer so there aren't any changes I need to make there.

@Cryoris Cryoris merged commit 5a9973b into Qiskit:master Feb 4, 2021
@mtreinish mtreinish deleted the runtime-mpl branch February 4, 2021 17:45
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Feb 5, 2021
In Qiskit#5582 we added a script that is used to ensure we don't import
slow optional dependencies in the default 'import qiskit' path. Since
that PR merged we've managed to remove a few more packages from the
default import path (see Qiskit#5619, Qiskit#5485, and Qiskit#5620) which we should add
the same checks to to ensure we don't regress and accidently start
importing them again in the default path. This commit adds all these
packages to the list of imports not allowed as part of 'import qiskit'.
mergify bot pushed a commit that referenced this pull request Feb 5, 2021
In #5582 we added a script that is used to ensure we don't import
slow optional dependencies in the default 'import qiskit' path. Since
that PR merged we've managed to remove a few more packages from the
default import path (see #5619, #5485, and #5620) which we should add
the same checks to to ensure we don't regress and accidently start
importing them again in the default path. This commit adds all these
packages to the list of imports not allowed as part of 'import qiskit'.
@kdk kdk added the Changelog: None Do not include in changelog label Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants