Inkscape shell mode. by anntzer · Pull Request #8248 · matplotlib/matplotlib

anntzer · 2017-03-09T05:06:16Z

Using inkscape in shell mode (i.e., having to start it only once) makes the entire test suite run in 624s on my laptop, compared to 787s currently (and 594s using rsvg, which has its own issues mentioned in #8242).

attn @tacaswell @NelleV

Edit: This seems to shave 1-2min per build on Travis, which is a bit less but not inconsistent with my local testing.

NelleV · 2017-03-09T17:01:27Z

That's a significant speed up!

QuLogic · 2017-03-09T23:58:36Z

+        # set to `None`, which make `proc.communicate` or `proc.terminate`
+        # fail.  By relying on `atexit` we ensure the destructor runs before
+        # `None`-setting occurs.
+        atexit.register(self.__del__)


Would __del__ not be called a second time when this object is deleted as well?

Yes, but __del__ can safely be executed twice (a file stream can be closed multiple times).

QuLogic · 2017-03-10T00:32:00Z

+                if window == terminator:
+                    break
+                else:
+                    continue


Code doesn't do anything.

QuLogic · 2017-03-10T00:34:36Z

+            next(itertools.islice(it, i, i), None)  # Advance `it` by `i`.
+        while True:
+            window = tuple(map(next, its))
+            if len(window) == n:


Could be flattened to:

while True: window = tuple(map(next, its)) if len(window) != n or window == terminator: break

or

while True: window = tuple(map(next, its)) if len(window) != n: # Ran out of data. break elif window == terminator: # Found terminator. break

went for the first version

QuLogic · 2017-03-10T00:47:08Z

+        stream = iter(functools.partial(self._proc.stdout.read, 1), b"")
+        terminator = (b"\n", b">")
+        n = len(terminator)
+        its = itertools.tee(stream, n)


If I understand this correctly, can you use the example from the itertools documentation?

def pairwise(iterable): "s -> (s0,s1), (s1,s2), (s2, s3), ..." a, b = tee(iterable) next(b, None) return zip(a, b)

I wrote a slightly more general version that can handle arbitrary long terminators. I would rather not have the implementation tied to the fact that we're looking exactly for a two-character terminator.

QuLogic · 2017-03-10T00:51:58Z

+                break
+
+    def __call__(self, orig, dest):
+        if not self._proc:


I think you also need to handle if inkscape crashed in the background.

QuLogic · 2017-03-10T01:33:39Z

Related to that last comment, what happens when Inkscape fails to convert the image? Do we raise a something like FileNotFoundError, instead of something more specific?

anntzer · 2017-03-10T01:35:05Z

I don't know, but I don't think this PR changes anything about that...

tacaswell · 2017-03-10T20:16:57Z

-    assert expectedImage.ndim == actualImage.ndim
-    assert expectedImage.shape == actualImage.shape
-    absDiffImage = abs(expectedImage - actualImage)
+    assert expectedImage.shape[:2] == actualImage.shape[:2]


why the slices here?

tacaswell · 2017-03-10T20:19:55Z

-    assert expectedImage.shape == actualImage.shape
-    absDiffImage = abs(expectedImage - actualImage)
+    assert expectedImage.shape[:2] == actualImage.shape[:2]
+    absDiffImage = np.abs(expectedImage[:, :, :3] - actualImage[:, :, :3])


and why not check the alpha channel?

tacaswell · 2017-03-10T20:22:14Z

👍 but a bit concerned about not testing the alpha channel.

anntzer · 2017-03-10T20:55:08Z

I was having some issues locally where in some cases the alpha channel would be dropped by the converter (leading to a shape mismatch on comparison) but they seem not to occur anymore. Restored the full comparison.

QuLogic · 2017-03-10T22:34:26Z

I don't know, but I don't think this PR changes anything about that...

Before, with make_external_conversion_command, both stdout and stderr were captured and if the return code was nonzero or the output file didn't exist, it would raise an exception with both of these outputs.

Now, checking for the file occurs much later in the comparison, and any inkscape output is either gobbled up or always printed (but not necessarily at the right place, depends on pytest capture mode, probably.)

anntzer · 2017-03-10T22:53:07Z

Good point. Looks like this will raise FileNotFoundError (or whatever OSError is appropriate) when compare_images calls read_png_int (that function raises the correct exception, I've checked). This is consistent with the behavior of make_external_conversion_command.

In either case, perhaps it would be better to reraise as an ImageComparisonError? (but it's kind of an unrelated issue).

QuLogic · 2017-03-12T01:14:10Z

Yes, it raises the same type of error, but my point is that you've no longer got the output from Inkscape coupled with the exception.

anntzer · 2017-03-12T06:07:36Z

The Appveyor failure seems real.
The way arguments are passed will probably fail if the some path names contain a space (which is always possible via the directory name...). I need to check how inkscape parses this -- including on Windows :/

Edit: inkscape uses g_shell_parse_argv (http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/view/head:/src/main.cpp#L1234) which has a consistent cross-platform behavior (https://developer.gnome.org/glib/stable/glib-Shell-related-Utilities.html#g-shell-parse-argv); it looks like just using shlex.quote is enough.

QuLogic · 2017-03-13T01:08:29Z

There seem to be some timeout problems with the Inkscape process.

anntzer · 2017-03-13T01:12:01Z

Yup, looking into it.

anntzer · 2017-03-13T04:26:54Z

Temporarily closing to avoid spamming the build queue while I try to fix the timeout...

anntzer · 2017-03-14T01:09:24Z

Looks like I got it to work.

Main failure point was that old versions of inkscape (0.48.3, which is used on Travis) would block when stderr was redirected to a pipe and the pipe was not being read from (this is no longer the case on the most recent Inkscape). Instead, I just redirected stderr to a temporary file.

Unless I am mistaken, the Appveyor build is 20min(!!) faster -- perhaps because repeatedly spawning inkscape was much slower on Windows?

NelleV · 2017-03-14T01:14:38Z

It's seems indeed much faster on windows now. Good job! I think this is going to be a real relief in our CI pipeline!

Heavily rewritten PR

anntzer · 2017-03-14T01:30:49Z

PS: Please review carefully, the patch is not completely trivial (and was changed quite a bit since @tacaswell's original review, so I dismissed it).

QuLogic

Holy speedups, Batman!

I think this looks good.

QuLogic · 2017-03-14T22:15:16Z

+                raise OSError("Failed to start Inkscape")
+
+        # Inkscape uses glib's `g_shell_parse_argv`, which has a consistent
+        # behavior across platforms, so we can just use `shlex.quote`.


Isn't this comment related to the next section (L210)?

Yes. Moving it down. Will also clarify the comment regarding \n (inkscape uses fgets so there's no choice).

QuLogic · 2017-03-14T22:22:35Z

+            # right...
+            self._stderr.seek(0)
+            raise ImageComparisonFailure(
+                self._stderr.read().decode(


I'm fine as long as you've tried this out.

In [1]: from matplotlib.testing.compare import _SVGConverter; _SVGConverter()("/tmp/quux.svg", "/tmp/foo.png") --------------------------------------------------------------------------- ImageComparisonFailure Traceback (most recent call last) <ipython-input-2-c896ab68b2ce> in <module>() ----> 1 from matplotlib.testing.compare import _SVGConverter; _SVGConverter()("/tmp/quux.svg", "/tmp/foo.png") /home/antony/src/extern/matplotlib/lib/matplotlib/testing/compare.py in __call__(self, orig, dest) 225 raise ImageComparisonFailure( 226 self._stderr.read().decode( --> 227 sys.getfilesystemencoding(), "replace")) 228 229 def __del__(self): ImageComparisonFailure: ** Message: /dev/null is not a valid directory. ** Message: Inkscape will run with default settings, and new settings will not be saved. ** (inkscape:25211): WARNING **: Could not create extension error log file '/dev/null/extension-errors.log' ** (inkscape:25211): WARNING **: Can't open file: /tmp/quux.svg (doesn't exist) ** (inkscape:25211): WARNING **: Can't open file: /tmp/quux.svg (doesn't exist) ** (inkscape:25211): WARNING **: Specified document /tmp/quux.svg cannot be opened (does not exist or not a valid SVG file)

The first few warnings should be spurious (due to intentionally setting INKSCAPE_PROFILE_DIR to a nonexistent folder) and I could technically crop them out, but I'd rather have the full info available just in case...

tacaswell · 2017-03-15T03:40:56Z

The appveyor failure is conda related, the travis failure is one of the flakey ones. Going to go ahead and merge this as is.

anntzer added the topic: testing label Mar 9, 2017

anntzer force-pushed the inkscape-shell-mode branch 2 times, most recently from 1dee9b7 to f44aaf1 Compare March 9, 2017 05:38

QuLogic reviewed Mar 10, 2017

View reviewed changes

anntzer force-pushed the inkscape-shell-mode branch from f44aaf1 to d67380d Compare March 10, 2017 01:30

tacaswell reviewed Mar 10, 2017

View reviewed changes

tacaswell added this to the 2.1 (next point release) milestone Mar 10, 2017

anntzer force-pushed the inkscape-shell-mode branch from d67380d to c3fab50 Compare March 10, 2017 20:55

tacaswell previously approved these changes Mar 10, 2017

View reviewed changes

anntzer force-pushed the inkscape-shell-mode branch from c3fab50 to 2a25286 Compare March 12, 2017 07:44

NelleV changed the title ~~Inkscape shell mode.~~ [MRG+1] Inkscape shell mode. Mar 12, 2017

anntzer force-pushed the inkscape-shell-mode branch from 2a25286 to 9276eea Compare March 13, 2017 00:38

anntzer force-pushed the inkscape-shell-mode branch from 9276eea to bd7865c Compare March 13, 2017 01:48

anntzer closed this Mar 13, 2017

anntzer reopened this Mar 13, 2017

anntzer force-pushed the inkscape-shell-mode branch 2 times, most recently from d55b6ed to dc3cfd8 Compare March 13, 2017 15:56

anntzer force-pushed the inkscape-shell-mode branch 2 times, most recently from 7dcc42a to 3ab7b71 Compare March 13, 2017 21:35

anntzer changed the title ~~[MRG+1] Inkscape shell mode.~~ [MRG] Inkscape shell mode. Mar 14, 2017

anntzer mentioned this pull request Mar 14, 2017

investigate whether using a single instance of ghostscript for ps->png conversion can speed up the Windows build #8293

Closed

QuLogic approved these changes Mar 14, 2017

View reviewed changes

Inkscape shell mode.

8c66203

anntzer force-pushed the inkscape-shell-mode branch from 3ab7b71 to 8c66203 Compare March 14, 2017 22:39

tacaswell merged commit f5b6246 into matplotlib:master Mar 15, 2017

anntzer deleted the inkscape-shell-mode branch March 15, 2017 03:53

QuLogic changed the title ~~[MRG] Inkscape shell mode.~~ Inkscape shell mode. Apr 30, 2017

This was referenced Oct 17, 2017

Save to .pdf doesn't work in 2.1.0 #9390

Closed

Batch ghostscript converter. #9454

Merged

Conversation

anntzer commented Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NelleV commented Mar 9, 2017

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

Uh oh!

QuLogic commented Mar 10, 2017

Uh oh!

anntzer commented Mar 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Mar 10, 2017

Uh oh!

anntzer commented Mar 10, 2017

Uh oh!

QuLogic commented Mar 10, 2017

Uh oh!

anntzer commented Mar 10, 2017

Uh oh!

QuLogic commented Mar 12, 2017

Uh oh!

anntzer commented Mar 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

QuLogic commented Mar 13, 2017

Uh oh!

anntzer commented Mar 13, 2017

Uh oh!

anntzer commented Mar 13, 2017

Uh oh!

anntzer commented Mar 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NelleV commented Mar 14, 2017

Uh oh!

anntzer commented Mar 14, 2017

Uh oh!

QuLogic 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!

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Mar 15, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

anntzer commented Mar 9, 2017 •

edited

Loading

anntzer commented Mar 12, 2017 •

edited

Loading

anntzer commented Mar 14, 2017 •

edited

Loading