Do not import scipy.signal and matplotlib at package import time - #3463
Open
VenishPaneliya wants to merge 2 commits into
Open
Do not import scipy.signal and matplotlib at package import time#3463VenishPaneliya wants to merge 2 commits into
VenishPaneliya wants to merge 2 commits into
Conversation
`import torchmetrics` pulled in two optional stacks that nothing on the import path uses. This is on the critical path for downstream libraries — Lightning imports torchmetrics only to compare versions — so everyone paid for them, including users who never plot or touch audio metrics. scipy.signal came from the SRMRpy `hamming` back-compat patch duplicated across three eagerly executed `__init__.py` files. `srmrpy` is only ever imported by `tests/unittests/audio/test_srmr.py`, so the patch moves there. matplotlib came from `utilities/plot.py`, which imported it at module level purely to build the `_AX_TYPE`, `_PLOT_OUT_TYPE` and `_CMAP_TYPE` annotation aliases and to alias `plt.style.context`. The aliases now resolve to the real classes under `TYPE_CHECKING` and to `object` at runtime, and `style_change` imports pyplot on first use. Because the aliases are no longer real classes at runtime, the four places that tested them with `isinstance` would have matched every object — silently skipping the trimming in `trim_axs` and disabling the argument validation in `MetricCollection.plot` and `MultitaskWrapper.plot`. They now go through `_is_axes`, which resolves the actual class. Measured with only scipy and matplotlib installed, `import torchmetrics` goes from 5559 ms / 2158 modules to 3571 ms / 1371 modules. Refs Lightning-AI#3457. Leaves the torchvision half to Lightning-AI#3432.
VenishPaneliya
requested review from
SkafteNicki and
justusschock
as code owners
August 17, 2026 21:10
for more information, see https://pre-commit.ci
Author
|
Apologies — I should have checked the open PR list before opening this. #3459 already covers the Flagging the overlap so you can decide rather than review both:
If you'd prefer, I'm happy to close this and rebase just the matplotlib half on top of #3459 once that lands, so the two don't conflict. Let me know which you'd rather have. |
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.
What does this PR do?
Refs #3457 — the
scipy.signalandmatplotlibhalves. Leaves thetorchvision/arniqa half to #3432 to avoid conflicting.import torchmetricspulled in two optional stacks that nothing on the import path uses. This sits on the critical path for downstream libraries — Lightning imports torchmetrics only to compare versions — so everyone paid for it, including users who never plot or touch audio metrics.scipy.signalCame from the SRMRpy
hammingback-compat patch, duplicated across three eagerly executed__init__.pyfiles.srmrpyis only ever imported bytests/unittests/audio/test_srmr.py— the library reimplements SRMR in pure PyTorch and never imports it — so the patch moves into that test module.matplotlibCame from
utilities/plot.py, which imported it at module level purely to build the_AX_TYPE/_PLOT_OUT_TYPE/_CMAP_TYPEannotation aliases and to aliasstyle_change = plt.style.context. The aliases now resolve to the real classes underTYPE_CHECKINGand toobjectat runtime, andstyle_changeimportspyploton first use.I kept runtime
objectfallbacks rather than making the aliasesTYPE_CHECKING-only, because that would requirefrom __future__ import annotationsin the ~140 modules that annotate with them. This way the diff stays insideplot.py(plus the two call sites below).The subtle part
Once the aliases stop being real classes at runtime,
isinstance(x, _AX_TYPE)becomesisinstance(x, object)— always true. Four places relied on that check:plot.pytrim_axscollections.py×2MetricCollection.plotargument validation stopped raisingwrappers/multitask.pyMultitaskWrapper.plotargument validation stopped raisingThe
trim_axsone was caught bytest_confusion_matrix_plotter[multilabel confusion matrix]failing withAttributeError: 'numpy.ndarray' object has no attribute 'set_title'. All four now go through a new_is_axeshelper that resolves the actual class, and there is a regression test pinning the behaviour.Measurements
import torchmetrics, median of 5 cold subprocesses, Python 3.13 / Windows:With only scipy and matplotlib installed — the configuration this PR targets:
sys.modulesscipy.signal,matplotlib.pyplotWith
transformersandtorchvisionalso installed, where other eager imports dominate:sys.modulesscipy.signal,matplotlib.pyplot,torchvisiontorchvisionTests
New
tests/unittests/utilities/test_lazy_imports.pychecks in a subprocess that a bareimport torchmetricsleavesscipy.signalandmatplotlibout ofsys.modules, that the aliases still resolve at runtime, thatstyle_changeworks as both a context manager and a repeatable decorator, and that_is_axesdiscriminates where the alias cannot.All 6 failures are missing optional packages locally (
pesq,gammatone,pycocotools) and are identical on unpatchedmaster:— the 3 extra being this PR's own new tests, which correctly fail without the fix.
Lint and types, compared against
masterfor the same files:One thing found along the way, not fixed here
When
transformersis installed,import torchmetricsstill pulls inscipy— but via a different path than this issue describes:bert.pyimportstransformersat module level, which pulls inscipy.sparse(and a lot more). That is a separate eager import from the SRMRpy one, so I left it alone and scoped the regression test toscipy.signalaccordingly — asserting on thescipyroot would make the test fail for an unrelated reason. Happy to open a follow-up issue if that is worth tracking.Before submitting
scipy.signal,torchvisionandmatplotlibare imported eagerly byimport torchmetrics#3457)