Add PEP 519 support by aragilar · Pull Request #6788 · matplotlib/matplotlib

aragilar · 2016-07-18T07:10:27Z

Closes #5968 and replaces #6772. This should allow pathlib.Path or other PEP 519 objects to be used with matplotlib.

aragilar · 2016-07-18T09:10:35Z

I'm not sure why this is failing on windows, suggestions?

jenshnielsen · 2016-07-18T10:14:59Z

Looks like it's a permission error where it tries to write the files IOError: [Errno 13] Permission denied: u'c:\\users\\appveyor\\appdata\\local\\temp\\1\\tmpo9j2ke.svg'

@JanSchulz Do you know anything about the permission model on appveyor

jankatins · 2016-07-18T12:36:04Z

Appveyor builds run as admin, so I don't think this is the problem. In my experience, such problem arise (on windows) from doing creation/deletions too fast: one process still has the file opened/locked and the next call is not allowed to do something with it.

jankatins · 2016-07-18T12:41:21Z

lib/matplotlib/tests/test_animation.py

+        return line,
+
+    # Use NamedTemporaryFile: will be automatically deleted
+    F = tempfile.NamedTemporaryFile(suffix='.' + extension)


This works on windows to get a named temp file name...

aragilar · 2016-09-04T13:22:33Z

Updated and rebased on top of current master. I did have to add matplotlib.testing.nose and matplotlib.testing.nose.plugins to the included modules to build locally though, is that expected?

aragilar · 2016-09-04T14:48:39Z

Also, is it worth creating a separate pull request with @JanSchulz's closed_tempfile context manager?

tacaswell · 2016-09-04T16:39:40Z

lib/matplotlib/tests/test_backend_pdf.py

+    try:
+        from pathlib import Path
+    except ImportError:
+        raise SkipTest("pathlib not installed")


@Kojoley What is the correct way to handle this now?

It is OK to use raise SkipTest as pytest supports it, but skip (without raise, just function call) (imported from from .testing import skip) is preferable as it does not use nose when you are using pytest.

tacaswell · 2016-09-04T19:24:15Z

We are in the process of converting from nose to pytest (and the whole test suite will currently run under both) with almost all of the work being done by @Kojoley .

Can you do the context manager change in this PR?

aragilar · 2016-09-13T01:17:43Z

This should be all good now.

NelleV

There is a lot of code duplication in the tests. I think they need to be refactored to have less code duplication.

NelleV · 2016-09-30T20:04:18Z

lib/matplotlib/cbook.py

                    pass
+
+
+try:


I think we should consider making cbook a python module with different submodule, and have all the backward compatibility fixes in a file where we cleary can keep track of it.
That would require more work (creating the cbook folder, __init__.py, a fixes.py module with those, updating the setup.py), but would allow us to split this huge file into more manageable size files.

What do you think?

Sure, I can do that.

So I'm not sure what can actually be moved from matplotlib.cbook to matplotlib.cbook.fixes, as the fspath stuff is the only compatibility things I can see, and there's already matplotlib.compat...

NelleV · 2016-09-30T21:42:12Z

lib/matplotlib/testing/__init__.py

+    """
+    with tempfile.NamedTemporaryFile(
+        'w+t', suffix=suffix, delete=False
+    ) as test_file:


That parenthesis here is weird. I am surprised pep8 allows this. Is there anyway to break the line in another way?

with tempfile.NamedTemporaryFile('w+t', suffix=suffix, delete=False) as test_file:

fits within 79 chars, and looks reasonable, I think.

NelleV · 2016-09-30T21:46:26Z

lib/matplotlib/testing/__init__.py

+@contextmanager
+def closed_tempfile(suffix='', text=None):
+    """
+    Context manager which yields the path to a closed temporary file with the


Can you please rewrite this docstring with the numpydoc format? I find the docstring relatively unclear on what exactly the function does. If I understand correctly, it creates, closes and returns a path to a file and deletes it on exiting the context. It is also unclear where the file is created. A note on how it relates and differs from tempfile.NamedTemporaryFile would help clarify what the function does.

Also, this is publicly available - do we want this publicly available? (if not, then the docstring comment can be ignored.)

Does publicly available mean should be used outside of matplotlib, or is the recommended option for other parts of matplotlib? I don't see much use outside matplotlib, unless you were already using matplotlib's testing utils.

NelleV · 2016-09-30T21:58:34Z

lib/matplotlib/tests/test_animation.py

+
+    if not animation.writers.is_available(writer):
+        skip("writer '%s' not available on this system"
+                               % writer)


I am, again, surprised pep8 allows that.

It doesn't for me; I don't know why this is passing either.

NelleV · 2016-09-30T22:03:10Z

lib/matplotlib/cbook.py

+            raise TypeError("expected str, bytes or os.PathLike object, not "
+                            + path_type.__name__)
+
+def fspath_no_except(path):


I think this should be a private function.
If it is not a private function, it should be documented.

I also don't really like this name, but I am not coming up with anything better for now…

I seem to recall something (Sphinx? NumPyDoc?) using a safe_ prefix; so something like safe_fspath (or even more explicitly safe_fspath_to_str)?

That'd be a much better name.

I'm not a fan of safe_fspath, as it's not really safe, since the reason it exists to to pass through anything that's not a PEP 519 path, and validating anything else doesn't happen. Also, there's no guarantee that it'll return a string, as __fspath__ can return either bytes or a string (I've special-cased pathlib here as there exist versions of pathlib without PEP 519 support), so safe_fspath_to_str is even worse. How about fspath_passthrough?

NelleV · 2016-09-30T22:04:17Z

lib/matplotlib/cbook.py

+                    return six.text_type(path)
+
+            raise TypeError("expected str, bytes or os.PathLike object, not "
+                            + path_type.__name__)


That is not pep8 compliant. Please break the line after the +.
I would also prefer using string formatting instead of concatenation.

PEP8 does not prescribe either way, but has a slight preference for the code as it's written.

So change or leave?

NelleV · 2016-09-30T22:07:06Z

lib/matplotlib/tests/test_animation.py

+
+
+@cleanup
+def check_save_animation_pep_519(writer, extension='mp4'):


I would avoid referencing to peps in function name. It is unclear what it does.

What about check_save_animation_PathLike?

check_save_animation_path_like?

NelleV · 2016-09-30T22:09:24Z

lib/matplotlib/tests/test_animation.py

+
+
+@cleanup
+def check_save_animation_pathlib(writer, extension='mp4'):


This test and the previous one looks like they could be refactoring to loop other Path and fspath compatible object to minimize the code duplication.

NelleV · 2016-09-30T22:30:15Z

lib/matplotlib/tests/test_backend_pdf.py

+
+
+@cleanup
+def test_pdfpages_accept_pep_519():


Those two tests look like they could be refactored into one.

The test_pdfpages_accept_pep_519 and test_savefig_accept_pathlib? I'm not sure it's possible to skip part of a test and have that appear in nose/pytest.

mmh… good question.
@Kojoley any idea on that?

NelleV · 2016-09-30T22:36:14Z

lib/matplotlib/tests/test_testing.py

+    text = "This is a test"
+    with closed_tempfile(".txt", text=text) as f:
+        with open(f, "rt") as g:
+            assert text == g.read()


Can you add the approriate code to be able to run the tests in this file independantly from the rest?

if __name__=='__main__': nose.runmodule(argv=['-s','--with-doctest'], exit=False)

I'll add this

NelleV · 2016-09-30T22:37:46Z

This patch will be useful for the future! Thanks for tackling this work.

mruffalo · 2016-12-07T00:05:06Z

I see that this is tagged with the "2.1 (next point release)" milestone -- and I don't follow matplotlib development very closely, so I'm not sure when to expect 2.0 to be released. Is there any chance of this (or a stripped-down version of this) functionality being included in a 1.5.x release relatively soon? I assume not, but I don't see too much harm in asking.

I started using Python 3.6 for my analysis code a week or so ago; it's been a while since the last beta, rc1 is due tonight (as I understand it), and the final release is scheduled for 2016-12-16. PEP 519 is the main thing motivating me to install 3.6 on my machines, since I use pathlib extensively and it's a pain to wrap every Path object in str() when using the convenience I/O functions in things like Pandas, matplotlib, and NetworkX (among others). I enjoyed dropping all of these extra calls to str(), but found that matplotlib was still rejecting Path objects instead of simply passing them to open as I expected. This isn't a big deal but is kind of an eyesore now that I can pass Path objects to things like pd.read_pickle.

More generally, is there a good justification for the current behavior of throwing an error from matplotlib if e.g. savefig is given an unknown type? That doesn't seem to really add anything to the simpler behavior of "if it's a file-like object, use it, else pass it to open, and let any exceptions thrown there propagate to the caller" -- and that simpler behavior would have entirely avoided the need for this pull request.

tacaswell · 2016-12-14T21:49:24Z

There will be no more 1.5.x releases, 2.0 has an rc1 out final hopefully in late December / early January . 2.1 should be something like late Q1/early Q2 2017.

tacaswell · 2016-12-14T21:54:42Z

lib/matplotlib/cbook.py

    results = []

-    for dirname, dirs, files in os.walk(root):
+    for dirname, dirs, files in os.walk(fspath_no_except(root)):


Shouldn't the standard library be able to handle this on it's own?

mangecoeur · 2017-04-12T15:29:36Z

Is this likely to land soon?

tacaswell · 2017-04-14T02:11:05Z

@aragilar I have done the rebase and taken over this PR. Please move discussion to #8481

mdboom added the status: needs review label Jul 18, 2016

aragilar mentioned this pull request Jul 18, 2016

Add PEP 519 support #6772

Closed

QuLogic added this to the 2.1 (next point release) milestone Jul 18, 2016

aragilar force-pushed the add_pep_519 branch from 6563aa3 to 2a54821 Compare July 18, 2016 07:32

jankatins reviewed Jul 18, 2016
View reviewed changes

tacaswell added status: needs revision and removed status: needs review labels Jul 26, 2016

aragilar added 2 commits September 4, 2016 23:17

Added tests for PEP 519/pathlib, these should fail

4e46319

Add PEP 519 support

1414c28

aragilar force-pushed the add_pep_519 branch from 2a54821 to 1414c28 Compare September 4, 2016 13:19

tacaswell reviewed Sep 4, 2016
View reviewed changes

Use skip instead of SkipTest for joint nose/pytest support

3cdf590

aragilar force-pushed the add_pep_519 branch 5 times, most recently from 2e04700 to 39cdd81 Compare September 12, 2016 10:45

Added closed_tempfile for testing filename handling

76bb608

aragilar force-pushed the add_pep_519 branch from 39cdd81 to 76bb608 Compare September 12, 2016 11:21

NelleV added status: needs review and removed status: needs revision labels Sep 30, 2016

NelleV requested changes Sep 30, 2016

View reviewed changes

NelleV reviewed Sep 30, 2016

View reviewed changes

NelleV added status: needs revision and removed status: needs review labels Sep 30, 2016

tacaswell reviewed Dec 14, 2016

View reviewed changes

tacaswell mentioned this pull request Apr 14, 2017

Add pep 519 #8481

Closed

6 tasks

tacaswell closed this Apr 14, 2017

QuLogic modified the milestones: v2.1, v2.2 Jan 21, 2018

QuLogic modified the milestones: needs sorting, v2.2.0 Feb 13, 2018

QuLogic added the status: duplicate label Feb 13, 2018



		@cleanup
		def check_save_animation_pep_519(writer, extension='mp4'):



		@cleanup
		def check_save_animation_pathlib(writer, extension='mp4'):

Uh oh!

Conversation

aragilar commented Jul 18, 2016

Uh oh!

aragilar commented Jul 18, 2016

Uh oh!

jenshnielsen commented Jul 18, 2016

Uh oh!

jankatins commented Jul 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aragilar commented Sep 4, 2016

Uh oh!

aragilar commented Sep 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kojoley Sep 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Sep 4, 2016

Uh oh!

aragilar commented Sep 13, 2016

Uh oh!

NelleV left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NelleV Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NelleV Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aragilar Oct 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NelleV Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Kojoley Sep 4, 2016 •

edited

Loading

NelleV Sep 30, 2016 •

edited

Loading

NelleV Sep 30, 2016 •

edited

Loading

aragilar Oct 1, 2016 •

edited

Loading

NelleV Sep 30, 2016 •

edited

Loading