FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Deprecate the MATPLOTLIBDATA environment variable. by anntzer · Pull Request #11241 · matplotlib/matplotlib · GitHub

Repository navigation

Deprecate the MATPLOTLIBDATA environment variable. - #11241

Merged
tacaswell merged 1 commit into
matplotlib:masterfrom
anntzer:deprecate-MATPLOTLIBDATA
Mar 1, 2019
Merged

tacaswell merged 1 commit into
matplotlib:masterfrom
anntzer:deprecate-MATPLOTLIBDATA

Conversation

anntzer commented May 13, 2018

Copy link
Copy Markdown
Contributor

The MATPLOTLIBDATA environment variable is only relevant to non-standard
installs (e.g., debian) that move the mpl-data directory to a separate
location (in debian's case, /usr/share/matplotlib/mpl-data); but for
whoever is already patching Matplotlib to achieve this, they may as well
also patch get_data_path to always return the new correct path (which
debian already does in their 20_matplotlibrc_path_search_fix.patch),
reproduced here:

diff --git a/lib/matplotlib/__init__.py b/lib/matplotlib/__init__.py
index 9339707..563b0a8 100644
--- a/lib/matplotlib/__init__.py
+++ b/lib/matplotlib/__init__.py
@@ -738,10 +738,12 @@ def _get_data_path():
        return path

    _file = _decode_filesystem_path(__file__)
-    path = os.sep.join([os.path.dirname(_file), 'mpl-data'])
+    path = '/usr/share/matplotlib/mpl-data'
    if os.path.isdir(path):
        return path

+    raise RuntimeError('Could not find the matplotlib data files')
+
    # setuptools' namespace_packages may highjack this init file
    # so need to try something known to be in matplotlib, not basemap
    import matplotlib.afm
@@ -836,7 +838,7 @@ def matplotlib_fname():
            yield matplotlibrc
            yield os.path.join(matplotlibrc, 'matplotlibrc')
        yield os.path.join(_get_configdir(), 'matplotlibrc')
-        yield os.path.join(get_data_path(), 'matplotlibrc')
+        yield '/etc/matplotlibrc'

    for fname in gen_candidates():
        if os.path.exists(fname):

attn @sandrotosi I guess :)

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

anntzer added this to the v3.0 milestone May 13, 2018

Copy link
Copy Markdown
Contributor

in Debian we also use MATPLOTLIBDATA during matplotlib build, in order to point to a not-yet-installed mpl-data directory, for example:

to build the documentation:

        # build the doc
        MPLCONFIGDIR=. MATPLOTLIBDATA=$(CURDIR)/lib/matplotlib/mpl-data/ PYTHONPATH=$(CURDIR)/build/lib.$(PY_PLATFORM)-$(DEFPY) $(MAKE) -C doc html

to run the test suite:

        -PYTHONPATH=$(shell python$* -c "from distutils.command.build import build ; \
                                        from distutils.core import Distribution ; \
                                        b = build(Distribution()) ; \
                                        b.finalize_options() ; \
                                        print(b.build_platlib)") \
        MATPLOTLIBDATA=$(CURDIR)/lib/matplotlib/mpl-data/ \
        MPLCONFIGDIR=. \
                xvfb-run -a python$* tests.py --no-network --verbose --full-trace

        -PYTHONPATH=$(shell python$*-dbg -c "from distutils.command.build import build ; \
                                            from distutils.core import Distribution ; \
                                            b = build(Distribution()) ; \
                                            b.finalize_options() ; \
                                            print(b.build_platlib)") \
        MATPLOTLIBDATA=$(CURDIR)/lib/matplotlib/mpl-data/ \
        MPLCONFIGDIR=. \
                xvfb-run -a python$*-dbg tests.py --no-network --verbose --full-trace
        rm -f matplotlibrc

how would we achieve the same result when MATPLOTLIBDATA is gone?

anntzer commented May 13, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Is installing Matplotlib in a venv and building the docs from there acceptable for Debian?
Note that our devel guides strongly encourage doing stuff with an installed Matplotlib.

Or apply your mpl-data-moving patch after building the docs.

Or patch doc/conf.py to make it apply a patch to matplotlib/__init__.py to find the sample data where needed.

Copy link
Copy Markdown
Contributor

sadly it's not possible to build mpl in a venv and use it to build the documentation (it's just how the debian packaging system works). i understand your suggestions, but i have to make something acceptable by the debian ecosystem, in this case:

  • patches get applied after unpacking the upstream source code from the released tarball
  • python modules are built with (setup.py build)
  • tests are run against the just-built code
  • the documentation is pointing the PYTHONPATH to the just-built code
  • modules are installed in a temporary location (setup.py install) to be later wrapped up in a file.deb we can ship

if you really want to get rid of MATPLOTLIBDATA, i'll just keep it around as a debian patch, no bid deal

anntzer commented May 13, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Can you point to me where these rules are listed?
I don't want to make life exceedingly hard for debian packagers, but having some idea of what are debian's policies would help...

Edit: Or perhaps you can patch get_data_path() to add Debian's path as a last fallback. This way, when setting PYTHONPATH to the just-built Matplotlib, it'll use the "normal" path, but when the end-user is importing Matplotlib, the last-fallback path will be used.

Or as suggested above you can patch doc/conf.py to do something like

import matplotlib; matplotlib.get_data_path = lambda: <the normal data path>

QuLogic commented May 15, 2018

Copy link
Copy Markdown
Member

Tests are run against the installed matplotlib in Fedora during build. The reason that MATPLOTLIBDATA is used is because everything is installed in a self-contained destdir. Only specifying the patched path as the fallback only will not work because of this, as we don't really want to include that build root path in the installed version.

anntzer commented May 15, 2018

Copy link
Copy Markdown
Contributor Author

Would patching to

path = Path(__file__) / "../../(didn't count)/share/data/matplotlib"

work?
(i.e. the relative path of mpl-data relative to matplotlib/__init__.py in the destdir, in whatever way Fedora wants to arrange it)

QuLogic commented May 15, 2018

Copy link
Copy Markdown
Member

Actually, though I haven't looked into the history of it, I think the main reason for separating things is to avoid installing twice for Python 2 and 3, but with Matplotlib 3 only supporting Python 3 now...

anntzer force-pushed the deprecate-MATPLOTLIBDATA branch from 2052e3b to d62a243 Compare May 15, 2018 06:57
tacaswell modified the milestones: v3.0, v3.1 Jul 9, 2018
The MATPLOTLIBDATA environment variable is only relevant to non-standard
installs (e.g., debian) that move the mpl-data directory to a separate
location (in debian's case, /usr/share/matplotlib/mpl-data); but for
whoever is already patching Matplotlib to achieve this, they may as well
also patch `get_data_path` to always return the new correct path (which
debian already does in their `20_matplotlibrc_path_search_fix.patch`),
reproduced here:

    diff --git a/lib/matplotlib/__init__.py b/lib/matplotlib/__init__.py
    index 9339707..563b0a8 100644
    --- a/lib/matplotlib/__init__.py
    +++ b/lib/matplotlib/__init__.py
    @@ -738,10 +738,12 @@ def _get_data_path():
            return path

        _file = _decode_filesystem_path(__file__)
    -    path = os.sep.join([os.path.dirname(_file), 'mpl-data'])
    +    path = '/usr/share/matplotlib/mpl-data'
        if os.path.isdir(path):
            return path

    +    raise RuntimeError('Could not find the matplotlib data files')
    +
        # setuptools' namespace_packages may highjack this init file
        # so need to try something known to be in matplotlib, not basemap
        import matplotlib.afm
    @@ -836,7 +838,7 @@ def matplotlib_fname():
                yield matplotlibrc
                yield os.path.join(matplotlibrc, 'matplotlibrc')
            yield os.path.join(_get_configdir(), 'matplotlibrc')
    -        yield os.path.join(get_data_path(), 'matplotlibrc')
    +        yield '/etc/matplotlibrc'

        for fname in gen_candidates():
            if os.path.exists(fname):
anntzer force-pushed the deprecate-MATPLOTLIBDATA branch from d62a243 to 404ff5d Compare September 6, 2018 13:21

anntzer commented Sep 27, 2018

Copy link
Copy Markdown
Contributor Author

Re-pinging @QuLogic @sandrotosi after #11241 (comment). Thanks in advance for your input.

QuLogic commented Oct 4, 2018

Copy link
Copy Markdown
Member

I think that is reasonable and would work fine; the self-contained destdir is supposed to mirror the final install exactly after all.

Copy link
Copy Markdown
Member

I am -0.5 on this. I don't see much of an upside and see a downside of making the debian packaging a bit more annoying.

anntzer commented Oct 11, 2018

Copy link
Copy Markdown
Contributor Author

The upside is ultimately switching to https://docs.python.org/3/library/importlib.html#module-importlib.resources (Py3.7+) instead of having to bother with _get_data_path at all, and completely remove the rcparam...

anntzer commented Nov 7, 2018

Copy link
Copy Markdown
Contributor Author

FWIW the final patch would make _get_data_path something like (just)

    global _data_path_cm
    _data_path_cm = ExitStack()  # This keeps the contextmanager open.
    return _data_path_cm.enter_context(
        importlib.resources.path('matplotlib', 'mpl-data'))

and not even store it in rcParams (because it doesn't make sense to let the user modify it later anyways -- many things, e.g. the included font paths, are set in stone at import time).

Copy link
Copy Markdown
Contributor

let me just re-state what i said before: i totally understand if you want to get rid of MATPLOTLIBDATA env var to make mpl code easier to maintain. OTOH we kinda like the ability to specify where the mpl-data is at runtime, without patching anything (more than we already do ;) ) or setting a list of "weirdly looking" paths.

i'm just fine to come up with a debian-specific patch to support our needs if you ultimately decide to drop MATPLOTLIBDATA

anntzer commented Dec 29, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@tacaswell Are you willing to revisit this based on the comments so far?

Happy to help with the Debian-specific patch if needed.

jklymak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ummm, seems fine to me. OTOH I think @tacaswell should merge....

Copy link
Copy Markdown
Member

@sandrotosi this is coming for 3.1 (next month-ish).

QuLogic added a commit that referenced this pull request Mar 1, 2019
…241-on-v3.1.x

Backport PR #11241 on branch v3.1.x (Deprecate the MATPLOTLIBDATA environment variable.)
anntzer deleted the deprecate-MATPLOTLIBDATA branch March 1, 2019 09:43

QuLogic commented May 5, 2019

Copy link
Copy Markdown
Member

One minor inconvenience here is that if you're running with a patched path (using Path(__file__).parent ...), then you can't override where an editable install is looking for data. This is something you might do while testing backports, or (in my case), new images for FreeType 2.9.1.

anntzer commented May 6, 2019

Copy link
Copy Markdown
Contributor Author

But the baseline images aren't found relative to the data dir? I'm not sure what the issue is, but would be happy to hear more.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL