perf: defer Matplotlib import until something actually plots#198
Open
petercorke wants to merge 1 commit into
Open
perf: defer Matplotlib import until something actually plots#198petercorke wants to merge 1 commit into
petercorke wants to merge 1 commit into
Conversation
`import spatialmath` unconditionally pulled in all of Matplotlib,
even for users who never plot anything: `spatialmath/base/__init__.py`
did `from spatialmath.base.animate import *` / `from
spatialmath.base.graphics import *` at package-import time, and those
two modules do `import matplotlib.pyplot`. `geom2d.py`, `geom3d.py`,
`spline.py`, and both `transforms2d.py`/`transforms3d.py` had their
own copies of the same pattern, some via a `try: import
matplotlib.pyplot ... except ImportError:` optional-dependency check
that still imported eagerly, just without crashing if it failed.
Measured on this machine: bare `import matplotlib` costs ~96ms,
`matplotlib.path` (needed structurally by Polygon2's geometry, not
just plotting) adds ~0ms on top of that, but `matplotlib.pyplot`
specifically (backend resolution + figure/state setup) adds another
~160ms. That pyplot cost is what this change removes from `import
spatialmath`.
Approach: don't touch matplotlib usage inside graphics.py/animate.py
at all (dozens of `plt.Axes`-style annotations in there - rewriting
those would be the disproportionate version of this fix). Instead,
defer the module import itself:
- spatialmath/base/__init__.py: replace the two blanket `import *`
lines with a PEP 562 module `__getattr__` that imports
animate.py/graphics.py lazily on first access of one of their
names (Animate, plot_box, tranimate, etc.) rather than
unconditionally at package import time.
- spatialmath/base/transforms2d.py, transforms3d.py: both had a
module-level `try: import matplotlib.pyplot ... except
ImportError: _matplotlib_exists = False` used purely to decide
whether to define trplot/trplot2/tranimate/tranimate2 at all.
Replaced with a cheap `importlib.util.find_spec("matplotlib")`
check (no real import), and moved the small number of actual
`plotvol2/3`, `axes_logic`, `Animate`/`Animate2` and `plt.show()`
call sites into the specific functions that use them - they were
already the only things those two files needed matplotlib for.
- spatialmath/spline.py, geom3d.py: same pattern, moved `import
matplotlib.pyplot as plt` from module top into the one or two
methods that actually plot.
- spatialmath/geom2d.py: nuance here - Polygon2.__init__ uses
`matplotlib.path.Path` for real geometry (point containment etc.),
and Polygon2.transformed() uses `matplotlib.transforms.Affine2D`
for the same reason, so both stay at module level (cheap anyway,
per the measurement above). Only `matplotlib.pyplot` itself, and
the `plot_ellipse` import (only used by Ellipse.plot), moved.
All of the above needed `from __future__ import annotations` added
where not already present, so a signature like `ax: Optional[plt.Axes]
= None` doesn't force `plt` to be a real bound name at function
*definition* time (which happens at module-import time, before the
deferred import ever runs) - annotations become lazy strings instead,
which type checkers still read fine. Matches the existing convention
in 8 other files in this codebase.
One real (if minor) bug this surfaced: tests/test_geom3d.py used
`plt.figure()` without importing matplotlib.pyplot itself - it was
only working because `from spatialmath.geom3d import *` used to leak
`plt` in as a wildcard-imported name. Fixed by importing it directly,
which is what the test should have done regardless of this change.
Verified:
- `import spatialmath` no longer puts `matplotlib.pyplot` in
sys.modules; `matplotlib` (the ~96ms base package, structurally
needed by Polygon2's geometry) still does.
- Wall-clock `import spatialmath`, same machine, same warm caches:
~670ms average on 3 runs before this change (upstream/master, via a
throwaway worktree), ~447ms average after.
- Full test suite green in both CI-like mode (`CI=true
MPLBACKEND=Agg`: 338 passed, 4 skipped) and a local-run simulation
(`MPLBACKEND=Agg`, `CI` unset): 332 passed (excluding two files that
depend on a separate, already-open PR for their own local-run
fixes, unrelated to this change).
- Manual smoke test: trplot, trplot2, Polygon2.plot/.animate/.contains,
Ellipse.plot, Line3.plot, Plane3.plot, BSplineSE3.visualize all still
produce real output under a forced Agg backend.
- `black --check` clean at the pinned 23.10.0.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
import spatialmathunconditionally pulls in all of Matplotlib, even forusers who never plot anything (RAI wishlist item: "Don't depend on
matplotlib unless you're actually plotting").
spatialmath/base/__init__.pydid
from spatialmath.base.animate import */from spatialmath.base.graphics import *at package-import time, and those twomodules do
import matplotlib.pyplot.geom2d.py,geom3d.py,spline.py,and both
transforms2d.py/transforms3d.pyhad their own copies of thesame pattern — some via a
try: import matplotlib.pyplot ... except ImportError:optional-dependency check that still imported eagerly, justwithout crashing if matplotlib turned out to be missing.
Measured on my machine: bare
import matplotlibcosts ~96ms,matplotlib.path(needed structurally byPolygon2's geometry, not justplotting) adds ~0ms on top of that, but
matplotlib.pyplotspecifically(backend resolution + figure/state setup) adds another ~160ms. That's the
cost this PR removes from
import spatialmath. (Caveat: on this machinematplotlib isn't the single biggest contributor to overall import time —
scipy.interpolateandsympy, when installed, are comparable — so thisalone may not fully explain reports of ~9s import time on other machines;
worth keeping an eye on.)
Approach
Deliberately not touching matplotlib usage inside
graphics.py/animate.pythemselves — dozens ofplt.Axes-style annotations live inthere, and rewriting those would be a much bigger, riskier change than this
needs to be. Instead, defer the module import of
graphics/animateas a whole, plus a few other files with the same eager-import pattern:
spatialmath/base/__init__.py: replace the two blanketimport *lines with a PEP 562 module
__getattr__that importsanimate.py/graphics.pylazily on first access of one of their names(
Animate,plot_box,tranimate, etc.), not unconditionally atpackage import time.
spatialmath/base/transforms2d.py,transforms3d.py: both had amodule-level
try: import matplotlib.pyplot ... except ImportError: _matplotlib_exists = Falseused purely to decide whether to definetrplot/trplot2/tranimate/tranimate2at all. Replaced with a cheapimportlib.util.find_spec("matplotlib")check (no real import), andmoved the small number of actual
plotvol2/3,axes_logic,Animate/Animate2, andplt.show()call sites into the specificfunctions that use them.
spatialmath/spline.py,geom3d.py: same pattern — movedimport matplotlib.pyplot as pltfrom module top into the one or two methodsthat actually plot.
spatialmath/geom2d.py: nuance —Polygon2.__init__usesmatplotlib.path.Pathfor real geometry (point containment etc.), andPolygon2.transformed()usesmatplotlib.transforms.Affine2Dfor thesame reason, so both stay at module level (cheap anyway, per the
measurement above). Only
matplotlib.pyplotitself, and theplot_ellipseimport (only used byEllipse.plot), moved.All of the above needed
from __future__ import annotationsadded wherenot already present, so a signature like
ax: Optional[plt.Axes] = Nonedoesn't force
pltto be a real bound name at function definition time(module-import time, before the deferred import ever runs) — annotations
become lazy strings instead, which type checkers still read fine. Matches
the existing convention already used in 8 other files in this codebase.
One real (minor) bug this surfaced:
tests/test_geom3d.pyusedplt.figure()without importingmatplotlib.pyplotitself — it onlyworked because
from spatialmath.geom3d import *used to leakpltin asa wildcard-imported name. Fixed by importing it directly.
Testing
import spatialmathno longer putsmatplotlib.pyplotinsys.modules;matplotlib(the ~96ms base package, structurally needed byPolygon2'sgeometry) still does.
import spatialmath, same machine, same warm caches: ~670msaverage on 3 runs before this change (
upstream/master, via a throwawayworktree), ~447ms average after.
CI=true MPLBACKEND=Agg: 338passed, 4 skipped) and a local-run simulation (
MPLBACKEND=Agg,CIunset): 332 passed (excluding two files whose local-run behaviour depends
on a separate, already-open PR, unrelated to this change).
trplot,trplot2,Polygon2.plot/.animate/.contains,Ellipse.plot,Line3.plot,Plane3.plot,BSplineSE3.visualizeall still produce real output.black --checkclean at the pinned 23.10.0.