Overhaul external process calls #7572

tomspur · 2016-12-05T23:21:24Z

This pull request starts to implement the move from os.system to the subprocess module and captures stderr and includes it in the exceptions.

For more information see #7490.

QuLogic · 2016-12-07T03:01:09Z

I wonder if we should have been using our matplotlib.compat.subprocess for compatibility with GAE, or whether it makes no difference because you'd never have LaTeX installed there (the other instances of subprocess you've changed here don't really matter for this, I think.)

codecov-io · 2016-12-11T12:19:02Z

Current coverage is 62.03% (diff: 32.94%)

Merging #7572 into master will decrease coverage by 4.54%

@@             master      #7572   diff @@
==========================================
  Files           109        174     +65   
  Lines         46633      56048   +9415   
  Methods           0          0           
  Messages          0          0           
  Branches          0          0           
==========================================
+ Hits          31049      34770   +3721   
- Misses        15584      21278   +5694   
  Partials          0          0

Powered by Codecov. Last update c4caab8...10adcfc

tomspur · 2016-12-11T12:20:55Z

Thanks for the pointer to matplotlib.compat.subprocess. I think using it for consistency is also fine and I'll change it to it.

Just as a reference of the benefit of this:
With the failing outputs of TexManager, I see now in a more detail why some tests failed

ERROR: test suite for <class 'matplotlib.sphinxext.tests.test_tinypages.TestTinyPages'>
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tomspur/.virtualenvs/default_python3/lib/python3.5/site-packages/nose/suite.py", line 210, in run
    self.setUp()
  File "/home/tomspur/.virtualenvs/default_python3/lib/python3.5/site-packages/nose/suite.py", line 293, in setUp
    self.setupContext(ancestor)
  File "/home/tomspur/.virtualenvs/default_python3/lib/python3.5/site-packages/nose/suite.py", line 316, in setupContext
    try_run(context, names)
  File "/home/tomspur/.virtualenvs/default_python3/lib/python3.5/site-packages/nose/util.py", line 471, in try_run
    return func()
  File "/home/tomspur/src/matplotlib.git/build/lib.linux-x86_64-3.5/matplotlib/sphinxext/tests/test_tinypages.py", line 56, in setup_class
    raise e
  File "/home/tomspur/src/matplotlib.git/build/lib.linux-x86_64-3.5/matplotlib/sphinxext/tests/test_tinypages.py", line 53, in setup_class
    '{0}\nstderr:\n{1}\n'.format(out, err))
RuntimeError: sphinx-build failed with stdout:
b'Running Sphinx v1.4.8\nmaking output directory...\n'
stderr:
b'\nException occurred:\n  File "/home/tomspur/src/matplotlib.git/build/lib.linux-x86_64-3.5/matplotlib/rcsetup.py", line 847, in validate_cycler\n    cycler_inst.change_key(prop, norm_prop)\nAttributeError: \'Cycler\' object has no attribute \'change_key\'\nThe full traceback has been saved in /tmp/sphinx-err-0zxJFz.log, if you want to report the issue to the developers.\nPlease also report this if it was a user error, so that a better error message can be provided next time.\nA bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!\n'

tomspur · 2016-12-11T13:10:12Z

Having access to the stderr seems to work fine:

ERROR: matplotlib.tests.test_backend_pgf.test_rcupdate
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tomspur/.virtualenvs/default_python3/lib/python3.5/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/tomspur/src/matplotlib.git/build/lib.linux-x86_64-3.5/matplotlib/testing/decorators.py", line 150, in wrapped_callable
    func(*args, **kwargs)
  File "/home/tomspur/src/matplotlib.git/build/lib.linux-x86_64-3.5/matplotlib/testing/decorators.py", line 479, in backend_switcher
    result = func(*args, **kwargs)
  File "/home/tomspur/src/matplotlib.git/build/lib.linux-x86_64-3.5/matplotlib/tests/test_backend_pgf.py", line 144, in test_rcupdate
    compare_figure('pgf_rcupdate%d.pdf' % (i + 1), tol=tol[i])
  File "/home/tomspur/src/matplotlib.git/build/lib.linux-x86_64-3.5/matplotlib/tests/test_backend_pgf.py", line 43, in compare_figure
    plt.savefig(actual, **savefig_kwargs)
  File "/home/tomspur/src/matplotlib.git/build/lib.linux-x86_64-3.5/matplotlib/pyplot.py", line 696, in savefig
    res = fig.savefig(*args, **kwargs)
  File "/home/tomspur/src/matplotlib.git/build/lib.linux-x86_64-3.5/matplotlib/figure.py", line 1694, in savefig
    self.canvas.print_figure(*args, **kwargs)
  File "/home/tomspur/src/matplotlib.git/build/lib.linux-x86_64-3.5/matplotlib/backend_bases.py", line 2230, in print_figure
    **kwargs)
  File "/home/tomspur/src/matplotlib.git/build/lib.linux-x86_64-3.5/matplotlib/backends/backend_pgf.py", line 929, in print_pdf
    self._print_pdf_to_fh(fh, *args, **kwargs)
  File "/home/tomspur/src/matplotlib.git/build/lib.linux-x86_64-3.5/matplotlib/backends/backend_pgf.py", line 883, in _print_pdf_to_fh
    self.print_pgf(fname_pgf, *args, **kwargs)
  File "/home/tomspur/src/matplotlib.git/build/lib.linux-x86_64-3.5/matplotlib/backends/backend_pgf.py", line 865, in print_pgf
    self._print_pgf_to_fh(fh, *args, **kwargs)
  File "/home/tomspur/src/matplotlib.git/build/lib.linux-x86_64-3.5/matplotlib/backends/backend_pgf.py", line 844, in _print_pgf_to_fh
    RendererPgf(self.figure, fh),
  File "/home/tomspur/src/matplotlib.git/build/lib.linux-x86_64-3.5/matplotlib/backends/backend_pgf.py", line 422, in __init__
    self.latexManager = LatexManagerFactory.get_latex_manager()
  File "/home/tomspur/src/matplotlib.git/build/lib.linux-x86_64-3.5/matplotlib/backends/backend_pgf.py", line 234, in get_latex_manager
    new_inst = LatexManager()
  File "/home/tomspur/src/matplotlib.git/build/lib.linux-x86_64-3.5/matplotlib/backends/backend_pgf.py", line 318, in __init__
    raise LatexError("LaTeX returned an error, probably missing font or error in preamble:\n%s" % stdout)
matplotlib.backends.backend_pgf.LatexError: LaTeX returned an error, probably missing font or error in preamble:
b"This is pdfTeX, Version 3.14159265-2.6-1.40.17 (TeX Live 2016) (preloaded format=pdflatex)\n restricted \\write18 enabled.\n**entering extended mode\nLaTeX2e <2016/03/31>\nBabel <3.9r> and hyphenation patterns for 6 language(s) loaded.\n\n*(/usr/share/texlive/texmf-dist/tex/latex/base/minimal.cls\nDocument Class: minimal 2001/05/25 Standard LaTeX minimal class\n)\n*(/usr/share/texlive/texmf-dist/tex/latex/base/inputenc.sty\n(/usr/share/texlive/texmf-dist/tex/latex/ucs/utf8x.def))\n(/usr/share/texlive/texmf-dist/tex/latex/ucs/ucs.sty\n(/usr/share/texlive/texmf-dist/tex/latex/ucs/data/uni-global.def))\n*(/usr/share/texlive/texmf-dist/tex/latex/base/fontenc.sty\n(/usr/share/texlive/texmf-dist/tex/latex/base/t1enc.def))\n*\n! LaTeX Error: File `sfmath.sty' not found.\n\nType X to quit or <RETURN> to proceed,\nor enter new name. (Default extension: sty)\n\nEnter file name: ! Undefined control sequence.\n\\filename@simple ...#2\\\\}\\fi \\edef \\filename@base \n                                                  {#1}\n<*> \n    \n!  ==> Fatal error occurred, no output PDF file produced!\nTranscript written on texput.log.\n"

Which points to LaTeX Error: File sfmath.sty not found.

tomspur · 2016-12-11T13:12:02Z

The failure seems to be related to an upgrade of freetype on travis...

QuLogic · 2016-12-11T21:56:13Z

No, it's just a flaky test; we build in a specific freetype version on CI.

QuLogic · 2017-01-17T05:26:49Z

Can you rebase and remove the use of shell=True?

tomspur · 2017-02-04T20:55:46Z

Removing shell=True turns out to require a rewrite of most of TexManager as there are chained commands run in the subprocess. So this will likely still take a while.

QuLogic · 2017-02-04T21:18:34Z

It's mostly just cd which can be replaced by passing cwd= and outout redirection which can be fixed using the right stdout=.

tomspur · 2017-02-04T21:39:24Z

Thanks for the pointer. I didn't think about the stdout= parameter and was trying to do it more complicated...

tomspur · 2017-02-04T23:43:37Z

There are now only two shell=True left, which are windows specific and I cannot test them properly here on Linux...

tacaswell · 2017-02-05T00:08:12Z

You should always import from our compat shim, https://github.com/matplotlib/matplotlib/blob/1a9b4eecff4409b4d9e5502df2ab12b4b16ac1f8/lib/matplotlib/compat/subprocess.py due to issues on early micro versions of 2.7 on mac (see #5314 )

tacaswell · 2017-02-05T00:09:09Z

examples/tests/backend_driver.py

@@ -350,7 +350,7 @@ def run(arglist):
            return ret
 except ImportError:
    def run(arglist):
-        os.system(' '.join(arglist))
+        os.system(arglist)


Can just drop this whole section, this is <2.7 compat code.

Removed this part of the backend driver and used the compat shim also there.

tacaswell · 2017-02-05T00:10:05Z

Ah, disregard my comment, I see you are using the shim internally but not in the examples.

Carry on 🐑

tomspur · 2017-02-05T10:31:32Z

I don't know how to fix the python 2.7 failure. It looks quite similar to #7715, which @tacaswell has tracked down. Any pointers how to fix it here?

jkseppan · 2017-02-05T15:34:38Z

lib/matplotlib/backends/backend_ps.py

-                "%s" > "%s"'% (gs_exe, dpi, device_name,
-                               paper_option, psfile, tmpfile, outfile)
-
+    command = [gs_exe, "-dBATCH", "-dNOPAUSE", "-r%d" % dpi,


This probably needs to be str(gs_exe)

jkseppan · 2017-02-05T15:34:58Z

The fix to #7715 is #7804, and it added a non-ASCII directory on the path to catch this kind of bugs. The fix was to wrap the arguments to subprocess.check_output in str(...) so probably you need to do something similar here.

QuLogic · 2017-03-27T07:17:23Z

Ping @tomspur? You can ignore the build errors; they're just flaky tests.

tomspur · 2017-03-28T06:42:35Z

I just rebased it to master and cleaned up some parts. Let's see how it looks like now.

Things not implemented yet:

converting of a string with precmd to a list: I don't have a windows to test this on and the example of _get_shell_cmd in TexManager is never used actually in the repository.
use of check_output in try except: When the command fails, nothing is written to output and we would have no information what went wrong in the subprocess.

This commits starts with moving from `os.system` to the `subprocess` module and captures stderr and includes it in the exceptions. For more information see matplotlibgh-7490.

The full traceback leads to here: lib/matplotlib/backends/backend_ps.py:1537: in gs_distill if ps_backend_helper.supports_ps2write: # gs version >= 9 lib/matplotlib/backends/backend_ps.py:106: in supports_ps2write return self.gs_version[0] >= 9 lib/matplotlib/backends/backend_ps.py:87: in gs_version s = Popen([self.gs_exe, "--version"], stdout=PIPE) venv/lib/python2.7/site-packages/subprocess32.py:825: in __init__ restore_signals, start_new_session) venv/lib/python2.7/site-packages/subprocess32.py:1387: in _execute_child for exe in executable_list) venv/lib/python2.7/site-packages/subprocess32.py:1386: in <genexpr> executable_list = tuple(fs_encode(exe) venv/lib/python2.7/site-packages/subprocess32.py:1385: in <genexpr> for dir in path_list)

… information

tomspur · 2017-04-09T15:57:25Z

All comments should be implemented - the failing test in test_invisible_Line_rendering seems a false positive.

QuLogic

LGTM, modulo a few minor questions. Low coverage is probably to be expected given that we probably don't test what happens when subprocess calls fail. If it interests you, you can take a stab at writing some tests for that, but I don't think it needs to hold up this PR.

QuLogic · 2017-04-11T23:22:09Z

lib/matplotlib/backends/backend_ps.py

@@ -83,8 +84,7 @@ def gs_version(self):
            pass

        from matplotlib.compat.subprocess import Popen, PIPE
-        s = Popen(self.gs_exe + " --version",
-                     shell=True, stdout=PIPE)
+        s = Popen([str(self.gs_exe), "--version"], stdout=PIPE)


One missed change?

QuLogic · 2017-04-11T23:30:36Z

lib/matplotlib/backends/backend_ps.py

    gs_exe = ps_backend_helper.gs_exe
-    command = '%s -dBATCH -dNOPAUSE -sDEVICE=bbox "%s"' %\
-                (gs_exe, tmpfile)
+    command = '%s -dBATCH -dNOPAUSE -sDEVICE=bbox "%s"' % (gs_exe, tmpfile)
    verbose.report(command, 'debug')
    stdin, stdout, stderr = os.popen3(command)


This one could use subprocess.Popen, probably.

I missed that... Thanks

QuLogic · 2017-04-11T23:36:20Z

lib/matplotlib/texmanager.py

-                mpl.verbose.report(report, 'debug')
+                     'string:\n%s\n\n'
+                     'Here is the full report generated by LaTeX:\n%s '
+                     '\n\n' % (repr(tex.encode('unicode_escape')),


Did something require adding the unicode escaping? Windows?

Good question. This was like this in one of the calls the current master and I took that over in all new subprocess.check_outputs

QuLogic · 2017-04-11T23:37:58Z

lib/matplotlib/texmanager.py

+                    ('dvipng was not able to process the following '
+                     'string:\n%s\n\n'
+                     'Here is the full report generated by dvipng:\n%s '
+                     '\n\n' % (repr(tex.encode('unicode_escape')),


Same question?

QuLogic · 2017-04-11T23:38:40Z

lib/matplotlib/texmanager.py

+                    ('dvips was not able to process the following '
+                     'string:\n%s\n\n'
+                     'Here is the full report generated by dvips:\n%s '
+                     '\n\n' % (repr(tex.encode('unicode_escape')),


Same question.

tomspur · 2017-04-13T10:13:52Z

Thanks for the review.

The failing test is a flaky test.
The unicode escaping was there before, so I left it in as is.
I have a test around, but don't know how to use the image_comparison decorator and some pytest decorator to indicate an expected exception at the same time. I would prefer to continue looking into this, after this has been merged...

tacaswell · 2017-04-14T02:26:10Z

Thanks for doing this tedious work!

Looking forward to the added test coverage.

tacaswell added this to the 2.1 (next point release) milestone Dec 7, 2016

tomspur force-pushed the subprocesses branch from 6501dd9 to ffc993c Compare December 11, 2016 11:17

tomspur force-pushed the subprocesses branch from 68a6cc7 to 5208d74 Compare December 11, 2016 12:34

tomspur force-pushed the subprocesses branch from 0e50192 to 10adcfc Compare December 23, 2016 11:06

QuLogic mentioned this pull request Feb 4, 2017

overhaul external process calls in TexManager #7490

Closed

2 tasks

tomspur force-pushed the subprocesses branch from 10adcfc to 41ea14f Compare February 4, 2017 20:55

tomspur force-pushed the subprocesses branch 2 times, most recently from c4dac47 to 9fac96b Compare February 4, 2017 23:40

tomspur force-pushed the subprocesses branch 3 times, most recently from 2aa5cf1 to 0a338f0 Compare February 4, 2017 23:56

tacaswell reviewed Feb 5, 2017

View reviewed changes

tomspur force-pushed the subprocesses branch 2 times, most recently from 36b4b44 to d89c25a Compare February 5, 2017 10:02

jkseppan reviewed Feb 5, 2017

View reviewed changes

tomspur force-pushed the subprocesses branch 2 times, most recently from 7a0a653 to d85947a Compare March 28, 2017 06:33

tomspur force-pushed the subprocesses branch 3 times, most recently from d81ea42 to bc00092 Compare April 3, 2017 21:54

tomspur added 13 commits April 9, 2017 17:38

Overhaul external process calls

7e8882a

This commits starts with moving from `os.system` to the `subprocess` module and captures stderr and includes it in the exceptions. For more information see matplotlibgh-7490.

TexManager: Use subprocess instead of os.system

de4acea

Use subprocess instead os.system also in the examples

b09e79c

tests: Remove < 2.7 compat code

772fbc4

BUG: fix check_output with non-ascii in paths similar to matplotlib#7715

742cf62

doc/make.py: Use cwd in check_output instead of os.chdir

9bf01c6

Remove precmd from backend_ps and use subprocess

6d189b3

backend_ps: Print strings explicitely when printing error report

982ade1

subprocess: Use check_output instead of check_call

24ce833

backend_ps: remove uneeded imports and fix some pep8 errors

01491c0

TexManager: transform bytes-object to string before searching for box…

dbb0115

… information

TexManager: Remove unneeded _get_shell_cmd

f56dd13

tomspur force-pushed the subprocesses branch from 2ab7a36 to f56dd13 Compare April 9, 2017 15:38

QuLogic approved these changes Apr 11, 2017

View reviewed changes

tomspur added 3 commits April 13, 2017 10:14

Fix typo: 'determin'

c3b481b

backend_ps: replace os.popen3 -> subprocess.Popen

79d71cc

backend_ps: remove one not needed str casting

d579266

tacaswell merged commit 269dabc into matplotlib:master Apr 14, 2017

tomspur mentioned this pull request Apr 17, 2017

Add tests for the external process calls #8504

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhaul external process calls #7572

Overhaul external process calls #7572

tomspur commented Dec 5, 2016

QuLogic commented Dec 7, 2016

codecov-io commented Dec 11, 2016 •

edited

tomspur commented Dec 11, 2016

tomspur commented Dec 11, 2016

tomspur commented Dec 11, 2016

QuLogic commented Dec 11, 2016

QuLogic commented Jan 17, 2017

tomspur commented Feb 4, 2017

QuLogic commented Feb 4, 2017

tomspur commented Feb 4, 2017 •

edited

tomspur commented Feb 4, 2017

tacaswell commented Feb 5, 2017

tacaswell Feb 5, 2017

tomspur Feb 5, 2017

tacaswell commented Feb 5, 2017

tomspur commented Feb 5, 2017

jkseppan Feb 5, 2017

jkseppan commented Feb 5, 2017

QuLogic commented Mar 27, 2017

tomspur commented Mar 28, 2017

tomspur commented Apr 9, 2017

QuLogic left a comment •

edited

QuLogic Apr 11, 2017

QuLogic Apr 11, 2017

tomspur Apr 13, 2017

QuLogic Apr 11, 2017

tomspur Apr 13, 2017

QuLogic Apr 11, 2017

QuLogic Apr 11, 2017

tomspur commented Apr 13, 2017

tacaswell commented Apr 14, 2017

Overhaul external process calls #7572

Overhaul external process calls #7572

Conversation

tomspur commented Dec 5, 2016

QuLogic commented Dec 7, 2016

codecov-io commented Dec 11, 2016 • edited

Current coverage is 62.03% (diff: 32.94%)

tomspur commented Dec 11, 2016

tomspur commented Dec 11, 2016

tomspur commented Dec 11, 2016

QuLogic commented Dec 11, 2016

QuLogic commented Jan 17, 2017

tomspur commented Feb 4, 2017

QuLogic commented Feb 4, 2017

tomspur commented Feb 4, 2017 • edited

tomspur commented Feb 4, 2017

tacaswell commented Feb 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Feb 5, 2017

tomspur commented Feb 5, 2017

Choose a reason for hiding this comment

jkseppan commented Feb 5, 2017

QuLogic commented Mar 27, 2017

tomspur commented Mar 28, 2017

tomspur commented Apr 9, 2017

QuLogic left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomspur commented Apr 13, 2017

tacaswell commented Apr 14, 2017

codecov-io commented Dec 11, 2016 •

edited

tomspur commented Feb 4, 2017 •

edited

QuLogic left a comment •

edited