ENH: switch mpl_toolkits to implicit namespace package (PEP 420) #25381

neutrinoceros · 2023-03-04T12:25:41Z

PR Summary

closes #25244

PR Checklist

Documentation and Tests

Has pytest style unit tests (and pytest passes)
Documentation is sphinx and numpydoc compliant (the docs should build without error).
[N/A] New plotting related features are documented with examples.

Release Notes

[N/A] New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
[N/A] API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

oscargus · 2023-03-04T14:27:20Z

doc/users/next_whats_new/mpl_toolkit_pep420.rst

+`PEP 420 <https://peps.python.org/pep-0420/>`_.
+
+This change should be backward compatible. In case of problems please `file an issue on GitHub
+<https://github.com/matplotlib/matplotlib/issues/new/choose>`_.


While I sort of think it is a good idea to point directly to "New issue", it may still be better to point to the issues. If nothing else to avoid duplicates (if any...). But I'd wait and hear what other say.

I think this is implicit (other wise would would put this on every change note!).

should I just drop line 8 and 9 then ?

ksunden · 2023-03-06T22:31:06Z

setup.py

@@ -301,7 +301,6 @@ def make_release_tree(self, base_dir, files):

    package_dir={"": "lib"},
    packages=find_packages("lib"),


Does this need to be changed to find_namespace_packages?

The docs for setuptools indicate that packages without __init__ will be omitted, and as mpl_toolkits no longer has a __init__ I suspect it falls into that category

Good catch, I missed that ! I just updated the PR to include this. I don't have time right now to verify that it's safe to leave include and exclude empty, but I can spend more time on it tomorrow if needed

ksunden · 2023-03-06T22:31:46Z

Building the wheels as this change affects packaging

neutrinoceros · 2023-03-07T16:57:28Z

I ran inspect-wheel to see the before/after diff for a given wheel. Currently there are a couple errors to be fixed:

Missing

<             "matplotlib.tight_bbox",
<             "matplotlib.tight_layout",

Wrongly included

>             "matplotlib.tests.tinypages.conf",
>             "matplotlib.tests.tinypages.range4",
>             "matplotlib.tests.tinypages.range6",

neutrinoceros · 2023-03-07T18:36:08Z

I realized that matplotlib.tight_bbox and matplotlib.tight_layout are actually not on the dev branch any more, so I will revert my (doomed) include. On the other hand I havent found a reference run of the "wheels" workflow for the 3.8 (dev) branch yet, so I'm not sure what to do about matplotlib.tests.tinypages.*. My guess is that it should still be excluded (this directory doesn't have a __init__.py

tacaswell · 2023-03-07T19:15:13Z

https://github.com/matplotlib/matplotlib/actions/runs/4348842943 is an example run of the build-wheels on main (to save CI time we mostly do not build the wheels on every PR commit, but do build them on every merge to main and on the PRs we have the tag on).

👍 that the tinypages packaging should be excluded, that is something that exists purely for testing that the plot sphinx extension can read out parts of a .py file.

neutrinoceros · 2023-03-08T11:21:36Z

thanks !
It seems that excluding matplotlib.tests.tinypages is trickier than expected because of how setuptools.find_namespace_packages work. Quoting from its docstring:

'exclude' is a sequence of names to exclude; '*' can be used
as a wildcard in the names.
When finding packages, 'foo.*' will exclude all subpackages of 'foo'
(but not 'foo' itself).

effectively this means that, in order to exclude matplotlib.tests.tinypages, would need to specify exclude=["matplotlib.tests.*"] and then manually re-include all subpackages that are supposed to be included. I don't mind doing that, but is this worth the maintenance cost ?

tacaswell · 2023-03-08T17:31:02Z

I think we only hove one namespace package (mpl_toolkits) and I am very 👎🏻 on adding any more. Maybe we should just hard-code the one we have and leave it at that?

neutrinoceros · 2023-03-08T18:03:39Z

That would be my preference too. Unfortunately using

    packages=find_packages("lib"),
    namespace_packages=["mpl_toolkits"],

just like on the main branch, doesn't work:

$ python -m build
* Creating venv isolated environment...
* Installing packages in isolated environment... (certifi>=2020.06.20, oldest-supported-numpy, pybind11>=2.6, setuptools_scm>=7)
* Getting build dependencies for sdist...

Edit mplsetup.cfg to change the build options; suppress output with --quiet.

BUILDING MATPLOTLIB
      python: yes [3.11.2 (main, Feb 19 2023, 11:54:34) [Clang 14.0.0
                  (clang-1400.0.29.202)]]
    platform: yes [darwin]
       tests: no  [skipping due to configuration]
      macosx: yes [installing]

error in matplotlib setup command: Distribution contains no modules or packages for namespace package 'mpl_toolkits'

ERROR Backend subprocess exited when trying to invoke get_requires_for_build_sdist

I'm tended to think that's a shortcoming of setuptools. If anyone agrees with me I'll report upstream.

ksunden · 2023-03-08T18:14:35Z

Any reason we can't just do find_packages("lib") + ["mpl_toolkits"]

That adds the one implicit namespace package we want while ignoring the edgecase behavior of excludes.

I mean, find_packages is just a list of strings convenience method anyway... if the tuning options are not what we want, we can override them pretty easily.

I may suggest a comment to indicate that it is namespace package related/that get_namespace_packages doesn't do what we want. Such that future us can at least be reminded of it should we ever decide to add a namespace package again (somewhat doubt, I'd tend to reach for entry points for a branch of similar ideas these days, but could happen)

tacaswell · 2023-03-08T20:27:34Z

There is now discussion going on upstream with projects (e.g. zope and plone) who use this orders of magnitude more than we so it might be worth seeing how that shakes out.

neutrinoceros · 2023-03-08T20:29:20Z

@tacaswell can you link those discussions here ?

tacaswell · 2023-03-08T20:33:45Z

pypa/setuptools#3434

neutrinoceros changed the title ~~ENH: switch mpl_toolkit to implicit namespace package (PEP 420)~~ ENH: switch mpl_toolkits to implicit namespace package (PEP 420) Mar 4, 2023

neutrinoceros force-pushed the hotfix_25244 branch from f9abea4 to 086e7c3 Compare March 4, 2023 12:45

oscargus reviewed Mar 4, 2023

View changes

neutrinoceros marked this pull request as ready for review March 4, 2023 14:28

ksunden reviewed Mar 6, 2023

View changes

ksunden added the CI: Run cibuildwheel Run wheel building tests on a PR label Mar 6, 2023

melissawm added the topic: mpl_toolkit label Mar 6, 2023

neutrinoceros force-pushed the hotfix_25244 branch from 086e7c3 to fabec26 Compare March 6, 2023 23:05

neutrinoceros marked this pull request as draft March 7, 2023 17:01

neutrinoceros force-pushed the hotfix_25244 branch from 8d0cd58 to 63afcd3 Compare March 7, 2023 18:36

neutrinoceros marked this pull request as ready for review March 7, 2023 19:08

neutrinoceros added 2 commits March 8, 2023 12:23

ENH: switch mpl_toolkits to implicit namespace package (PEP 420)

0c70552

link comment

5b8ae1b

neutrinoceros force-pushed the hotfix_25244 branch from 63afcd3 to 5b8ae1b Compare March 8, 2023 11:23

oscargus mentioned this pull request Mar 13, 2023

deleting __init__.py from matplotlib/lib/mpl_toolkits #25441

Closed

6 tasks

Feb	MAR	Jul
	23
2022	2023	2025

ENH: switch mpl_toolkits to implicit namespace package (PEP 420) #25381

ENH: switch mpl_toolkits to implicit namespace package (PEP 420) #25381

neutrinoceros commented Mar 4, 2023 •

edited

oscargus Mar 4, 2023

tacaswell Mar 7, 2023

neutrinoceros Mar 7, 2023

neutrinoceros Mar 8, 2023

ksunden Mar 6, 2023

neutrinoceros Mar 6, 2023

ksunden commented Mar 6, 2023

neutrinoceros commented Mar 7, 2023

neutrinoceros commented Mar 7, 2023

tacaswell commented Mar 7, 2023

neutrinoceros commented Mar 8, 2023

tacaswell commented Mar 8, 2023

neutrinoceros commented Mar 8, 2023

ksunden commented Mar 8, 2023

tacaswell commented Mar 8, 2023

neutrinoceros commented Mar 8, 2023

tacaswell commented Mar 8, 2023

		@@ -301,7 +301,6 @@ def make_release_tree(self, base_dir, files):

		package_dir={"": "lib"},
		packages=find_packages("lib"),

ENH: switch mpl_toolkits to implicit namespace package (PEP 420) #25381

Are you sure you want to change the base?

ENH: switch mpl_toolkits to implicit namespace package (PEP 420) #25381

Conversation

neutrinoceros commented Mar 4, 2023 • edited

PR Summary

PR Checklist

oscargus Mar 4, 2023

Choose a reason for hiding this comment

tacaswell Mar 7, 2023

Choose a reason for hiding this comment

neutrinoceros Mar 7, 2023

Choose a reason for hiding this comment

neutrinoceros Mar 8, 2023

Choose a reason for hiding this comment

ksunden Mar 6, 2023

Choose a reason for hiding this comment

neutrinoceros Mar 6, 2023

Choose a reason for hiding this comment

ksunden commented Mar 6, 2023

neutrinoceros commented Mar 7, 2023

neutrinoceros commented Mar 7, 2023

tacaswell commented Mar 7, 2023

neutrinoceros commented Mar 8, 2023

tacaswell commented Mar 8, 2023

neutrinoceros commented Mar 8, 2023

ksunden commented Mar 8, 2023

tacaswell commented Mar 8, 2023

neutrinoceros commented Mar 8, 2023

tacaswell commented Mar 8, 2023

neutrinoceros commented Mar 4, 2023 •

edited