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

MNT: reduce number of implicit imports from toplevel __init__.py #26017

Closed

Conversation

tacaswell
Copy link
Member

Either hide or defer imports from the toplevel import

PR summary

When thinking about #26016 / #22247 I started to look into which of our public modules we import via #import matplotlib` and if that could be reduced further.

With this branch we are down to:

In [1]: import matplotlib

In [2]: import sys

In [3]: [k for k in sys.modules if k.startswith("matplotlib")]
Out[3]:
['matplotlib._api.deprecation',
 'matplotlib._api',
 'matplotlib._version',
 'matplotlib._c_internal_utils',
 'matplotlib.cbook',
 'matplotlib._docstring',
 'matplotlib._cm',
 'matplotlib._color_data',
 'matplotlib._colors',
 'matplotlib._fontconfig_pattern',
 'matplotlib._enums',
 'matplotlib.rcsetup',
 'matplotlib.ft2font',
 'matplotlib']

we need rcsetup to build rcParams, cbook is used everywhere, nad ft2font is imported in check_versions() to work around a wheel-related issue with kiwisolver + msft.

This has a minimal effect on import time (.01s of seconds) and as soon as you import anything else (e.g. import matplotlib.figure the most minimal thing you need to do ~anything with Matplotlib) the import list is 86 long so at best this defers a bit of work.

I'm torn if this is a good idea, I was curious what it would take to reduce the imports, so this is an answer to that. Most of the change is moving things out of colors.py to avoid the import of matplotlib.scale.

If we take this, then it should be with the understanding that this is the first step in trimming our imports or we should reject this as an indication that we are not going to worry about this sort of micro-optimizations.

PR checklist

Either hide or defer imports from the toplevel import
@tacaswell tacaswell added this to the v3.8.0 milestone May 31, 2023
@jklymak
Copy link
Member

jklymak commented May 31, 2023

My intuition would have been to go the other way and make the import actually useful.

@ksunden
Copy link
Member

ksunden commented May 31, 2023

Test failures:

Unit tests:

  • reaching into private functionality in test_gray_grey (_colors_full_map no longer accessible from mpl.colors)
  • TABLEAU_COLORS not being accessible from mpl.colors (probably actually should be public?, though the test that is failing is a little iffy, in my opinion... asserting that a statically defined dict has a values view that matches a statically defined list...)

Stubs:

  • the __getattr__ on class modules messes with stubs, causing them to need to be ignored
    • I think the .pyi should probably actually just drop the implementation detail of it being inside of a __getattr__ class, but still needs to be put into the ci/mypy-stubtest-allowlist.txt since those attributes, while they work at runtime with .access, do not get seen by stubtest.
    • Putting them in the class actually means we don't need the ignores for stubtest to be happy, but mypy proper doesn't see it as valid usage... (which is the actually more important bit)
  • One or two other relatively minor things largely surrounding a private dict subclass attribute... should be easy to manage.

Edit: to be clear, not necessarily advocating for the change, just writing down some observations that if we proceed down this path will hopefully help.

@timhoffm
Copy link
Member

timhoffm commented Jun 1, 2023

Without having looked through all the details: I'm somewhat sceptical on the _colors / colors sepearation. This makes reasoning where the code lives harder. Also it changes the fully qualified name e.g. to matplotlib._colors.Colormap. This can have some consequences. E.g. I assume that autodoc would not pick up the class (incidentally, we explicitly list the classes for colors https://github.com/matplotlib/matplotlib/blob/main/doc/api/colors_api.rst, so that still works). Can I still use the string "matplotlib.colors.Colormap" for typing? Maybe this all works but maybe we're breaking some tools / edge cases.


Semi-OT: The distribution of content between cm and colors is somewhat fuzzy. Not sure if that would help here, but if one starts to move things around internally, one should consider reordering the module contents. For example Colormap is in colors, but ColormapRegistry is in cm. IMHO norms and colormap should be in cm (and the name cm is not great but that's another topic).

@tacaswell
Copy link
Member Author

Discussed on the call, too much thrash for too little benefit.

@tacaswell tacaswell closed this Jun 1, 2023
@tacaswell tacaswell deleted the mnt/reduce_minimal_imports branch June 1, 2023 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants