Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Work around missing subprocess members on Google App Engine #1825

Merged
merged 8 commits into from Mar 20, 2013

Conversation

Projects
None yet
6 participants
Contributor

mgiuca-google commented Mar 15, 2013

On Google App Engine, the subprocess module exists, but is empty. This patch introduces a replacement module, subprocess_fixed, which provides a stub for subprocess.Popen which is compatible with existing usage.

I noticed that cbook already has a bit of a hack to work around the fact that subprocess.check_output is missing in Python 2.6, so I've moved that into my subprocess_fixed module as well, so now it has two reasons to exist.

Calls to subprocess_fixed.Popen on Google App Engine always raise OSError, which most of the codebase is already prepared for (since that's what happens when the application is missing). I've added a few missing catches of OSError.

Partial fix for Issue #1823.

@pelson pelson commented on the diff Mar 15, 2013

lib/matplotlib/cbook.py
pid = os.getpid()
if sys.platform == 'sunos5':
- a2 = Popen('ps -p %d -o osz' % pid, shell=True,
- stdout=PIPE).stdout.readlines()
+ try:
+ a2 = Popen('ps -p %d -o osz' % pid, shell=True,
+ stdout=PIPE).stdout.readlines()
+ except OSError:
+ raise NotImplementedError(
@pelson

pelson Mar 15, 2013

Member

I'm tempted to suggest a RuntimeError here. None-the-less, this is an API change which should be documented in docs/api/api_changes.rst

@mgiuca-google

mgiuca-google Mar 18, 2013

Contributor

Done. Note that I am having trouble building the docs locally, so I'm hoping my ReST syntax is correct!

@mgiuca-google

mgiuca-google Mar 18, 2013

Contributor

Also, I agree RuntimeError is more appropriate (since NotImplementedError implies that it is planning to be fixed one day), but I changed this to be compatible with the Windows implementation. Happy to change it if you want. For what it's worth, NotImplementedError is a subclass of RuntimeError.

@pelson pelson commented on the diff Mar 15, 2013

lib/matplotlib/cbook.py
- if 'stdout' in kwargs:
- raise ValueError('stdout argument not allowed, it will be overridden.')
- process = subprocess.Popen(stdout=subprocess.PIPE, *popenargs, **kwargs)
- output, unused_err = process.communicate()
- retcode = process.poll()
- if retcode:
- cmd = kwargs.get("args")
- if cmd is None:
- cmd = popenargs[0]
- raise subprocess.CalledProcessError(retcode, cmd, output=output)
- return output
-
-
-# python2.7's subprocess provides a check_output method
-if hasattr(subprocess, 'check_output'):
- check_output = subprocess.check_output
@pelson

pelson Mar 15, 2013

Member

This will also need to be documented in api_changes.

@mgiuca-google

mgiuca-google Mar 18, 2013

Contributor

The fact that it has been removed from cbook? It was never documented in the first place.

If you think that people are depending on cbook.check_output, perhaps I should just put it back there as an alias (check_output = subprocess_fixed.check_output)?

@pelson

pelson Mar 18, 2013

Member

No, I'm happy for us to move it out - the subprocess compat module makes more sense than cbook. We just need to make sure any public (even if undocumented) API changes are put in the api_changes.rst file.

@pelson pelson and 2 others commented on an outdated diff Mar 15, 2013

lib/matplotlib/subprocess_fixed.py
@@ -0,0 +1,76 @@
+"""
+A replacement wrapper around the subprocess module, with a number of
+work-arounds:
+- Provides the check_output function (which subprocess only provides from Python
+ 2.7 onwards).
+- Provides a stub implementation of subprocess members on Google App Engine
+ (which are missing in subprocess).
@pelson

pelson Mar 15, 2013

Member

I'd suggest an enhancement to core cpython might be desirable here too. Have you looked into doing that?

@pelson

pelson Mar 15, 2013

Member

(that would probably only apply to python3.4 onwards though)

@mdboom

mdboom Mar 15, 2013

Owner

Is there a reason AppEngine has an empty subprocess module rather than one that provides exception-raising stubs?

@mgiuca-google

mgiuca-google Mar 18, 2013

Contributor

@pelson: What enhancement are you suggesting for core cpython? The missing subprocess members is not a cpython problem, it's an App Engine problem. I don't know the rationale (why we don't supply stubs that raise OSError instead of them simply being missing), but this is fairly consistent across App Engine for modules that are unsupported. I've had a discussion with the Python team about changing this and it would be unlikely, because it could break existing users expectations (for example, they may be catching AttributeError). I think for now it would be easier to bring this into matplotlib, if it's not too much trouble. If App Engine ends up changing subprocess to include stubs, then it can be removed from Matplotlib.

@pelson pelson commented on the diff Mar 15, 2013

lib/matplotlib/texmanager.py
@@ -63,8 +63,12 @@
def dvipng_hack_alpha():
- p = Popen('dvipng -version', shell=True, stdin=PIPE, stdout=PIPE,
- stderr=STDOUT, close_fds=(sys.platform != 'win32'))
+ try:
+ p = Popen('dvipng -version', stdin=PIPE, stdout=PIPE, stderr=STDOUT,
+ close_fds=(sys.platform != 'win32'))
+ except OSError:
+ mpl.verbose.report('No dvipng was found', 'helpful')
+ return False
@pelson

pelson Mar 15, 2013

Member

Is this a good idea? Previously an exception would have been raised, now False is returned. What impact does that have?

@mgiuca-google

mgiuca-google Mar 18, 2013

Contributor

I'm happy to back off on this since it doesn't block texmanager from being imported, and it won't work anyway if dvipng is missing.

I don't think dvipng_hack_alpha is a command that should be used externally. It's used within the module to decide whether to change the output from dvipng. It looks like matplotlib detects the presence of dvipng on startup and if it isn't present, it disables the use of the tex module anyway. So this shouldn't really have an effect.

@mgiuca-google

mgiuca-google Mar 18, 2013

Contributor

Did you want me to revert this, or document it, or just leave as-is?

@pelson pelson and 3 others commented on an outdated diff Mar 15, 2013

lib/matplotlib/dviread.py
@@ -23,9 +23,9 @@
import errno
import matplotlib
import matplotlib.cbook as mpl_cbook
+import matplotlib.subprocess_fixed as subprocess
@pelson

pelson Mar 15, 2013

Member

I'm not a huge fan of the name subprocess_fixed. I wonder if it would be worth having matplotlib.compatibility.subprocess.

@mdboom - what are your thoughts on this?

@mdboom

mdboom Mar 15, 2013

Owner

Yes -- I like the idea of having a compat submodule containing stuff like this... we already have compatibility code scattered about, but I've seen the compat module approach in other projects and it's really nice. It makes it easy to clean it out later when the minimum requirements change etc.

@WeatherGod

WeatherGod Mar 15, 2013

Member

On Fri, Mar 15, 2013 at 9:20 AM, Michael Droettboom <
notifications@github.com> wrote:

In lib/matplotlib/dviread.py:

@@ -23,9 +23,9 @@
import errno
import matplotlib
import matplotlib.cbook as mpl_cbook
+import matplotlib.subprocess_fixed as subprocess

Yes -- I like the idea of having a compat submodule containing stuff like
this... we already have compatibility code scattered about, but I've seen
the compat module approach in other projects and it's really nice. It
makes it easy to clean it out later when the minimum requirements change
etc.

I am +1 on the compat submodule as well.

@mgiuca-google

mgiuca-google Mar 18, 2013

Contributor

Moved matplotlib.subprocess_fixed to matplotlib.compat.subprocess.

Member

pelson commented Mar 15, 2013

Again, this is looking good @mgiuca-google - thanks for your hard work.

@pelson pelson was assigned Mar 15, 2013

Contributor

mgiuca-google commented Mar 18, 2013

Hmm ... looks like Travis failed on Python 3 with this error:

$ python ../matplotlib/tests.py
Traceback (most recent call last):
  File "../matplotlib/tests.py", line 8, in <module>
    import matplotlib
  File "/home/travis/virtualenv/python3.2/lib/python3.2/site-packages/matplotlib-1.3.x-py3.2-linux-x86_64.egg/matplotlib/__init__.py", line 136, in <module>
    from matplotlib.compat import subprocess
  File "/home/travis/virtualenv/python3.2/lib/python3.2/site-packages/matplotlib-1.3.x-py3.2-linux-x86_64.egg/matplotlib/compat/subprocess.py", line 18, in <module>
    from . import subprocess
ImportError: cannot import name subprocess

The command "python ../matplotlib/tests.py" exited with 1.

I'm not sure why my code import subprocess was magically changed to from . import subprocess -- can you think of what is doing this and a way around it? In any case, I think the problem is that the name subprocess is now overloaded. From within the compat package, it could refer to either my overriding version, or the official subprocess library. This doesn't sound like a very good idea, but then if we can't call it "subprocess" then we're back to requiring all imports of this module to use "as".

I think this can be worked around using __import__ magic. Does that sound like a good idea, or would you rather just rename the module?

Member

pelson commented Mar 19, 2013

I'm not sure why my code import subprocess was magically changed to from . import subprocess -- can you think of what is doing this and a way around it

Python 2's default is to use relative imports before absolute ones, so import subprocess in python 2 is the same as from . import subprocess in python 3 (assuming you have a submodule called subprocess).

In order to get the behaviour you desire, I think you can just add the line from __future__ import absolute_import into your subprocess compatibility module.

It does look a little like 2to3 is being a little over zealous here...

mgiuca-google added some commits Mar 12, 2013

@mgiuca-google mgiuca-google Added new module, matplotlib.subprocess_fixed, as a replacement for s…
…ubprocess.

cbook: Moved check_output to subprocess_fixed.
backend_pgf: Import check_output from subprocess_fixed instead of cbook.
0755aa6
@mgiuca-google mgiuca-google All modules now use matplotlib.subprocess_fixed instead of subprocess. 52c075f
@mgiuca-google mgiuca-google subprocess_fixed: Gracefully handle platforms without subprocess.Popen.
If subprocess.Popen is missing (for example, on Google App Engine), replaces it
with a dummy version that raises OSError. In these environments, calls to
subprocess will be handled properly as if the called app could not be found,
instead of raising AttributeError.
b29c93c
@mgiuca-google mgiuca-google texmanager: Ignore dvipng if it cannot be found, instead of raising O…
…SError.
a117e19
@mgiuca-google mgiuca-google cbook.report_memory: If ps not found, raises NotImplementedError, not…
… OSError.
ea342b6
@mgiuca-google mgiuca-google Moved matplotlib.subprocess_fixed to matplotlib.compat.subprocess. f41000d
@mgiuca-google mgiuca-google api_changes: Document the API changes.
- The subtle change to cbook.report_memory's exception types.
- Moving subprocess_fixed to compat.subprocess.
888fa20
@mgiuca-google mgiuca-google compat.subprocess: Turn on absolute importing.
This prevents the module from importing itself (as opposed to the global
subprocess module) on Python 3 after 2to3 conversion.
03cddc8
Contributor

mgiuca-google commented Mar 19, 2013

OK thanks for the explanation. What I don't get is if Python 2 has relative imports by default, why does it work? (It only stopped working in Python 3.)

Oh well, I've added the absolute import so we'll see what Travis says. Also rebased.

@pelson pelson added a commit that referenced this pull request Mar 20, 2013

@pelson pelson Merge pull request #1825 from mgiuca-google/subprocess-google-app-engine
Work around missing subprocess members on Google App Engine
8ec9555

@pelson pelson merged commit 8ec9555 into matplotlib:master Mar 20, 2013

1 check passed

default The Travis build passed
Details

@mgiuca-google mgiuca-google deleted the mgiuca-google:subprocess-google-app-engine branch Mar 21, 2013

This Popen call is broken with matplotlib.compat.subprocess, as its Popen (just like subprocess.Popen) does not accept a full command line but rather a list of [command, flags, args...].
So this needs to be changed to Popen('dvipng', '-version'], ...)

Contributor

mgiuca-google replied Apr 16, 2013

What do you mean "broken"? It worked for me last time I checked (from memory?), and as far as I know, it always has. From the Python docs (emphasis mine):

args should be a sequence of program arguments or else a single string. By default, the program to execute is the first item in args if args is a sequence. If args is a string, the interpretation is platform-dependent and described below.

It goes on to say that "it is recommended to pass args as a sequence", which I agree is good advice. But this shouldn't be broken.

It goes further on just into the next paragraph:

On Unix, if args is a string, the string is interpreted as the name or path of the program to execute. However, this can only be done if not passing arguments to the program.

The string is interpreted as a command line only with shell=True set, which is not the case here and also discouraged. Otherwise the Unix interpreter takes "dvipng -version" as the literal name of the executable, which of course does not exist:

matplotlib version 1.3.x
verbose.level helpful
interactive is True
platform is darwin
>>> p = matplotlib.compat.subprocess.Popen('ls')
>>> CHANGELOG       LICENSE         README.rst      agg24           distribute_setup.py lib         setup.cfg.template  src         ttconv
CONTRIBUTING.md     MANIFEST.in     TODO            boilerplate.py      distribute_setup.pyc    license.py      setup.py        tests.py        unit
CXX         Makefile        TODO_TESTS      build           doc         matplotlibrc.template   setupext.py     tools
INSTALL         README.osx      __pycache__     dist            examples        release         setupext.pyc        tox.ini

>>> p = matplotlib.compat.subprocess.Popen('ls -F')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/sw/lib/python2.7/subprocess.py", line 711, in __init__
    errread, errwrite)
  File "/sw/lib/python2.7/subprocess.py", line 1308, in _execute_child
    raise child_exception
OSError: [Errno 2] No such file or directory
>>> p = matplotlib.compat.subprocess.Popen('ls -F'.split())
>>> CHANGELOG       LICENSE/        README.rst      agg24/          distribute_setup.py*    lib/            setup.cfg.template  src/            ttconv/
CONTRIBUTING.md     MANIFEST.in     TODO            boilerplate.py      distribute_setup.pyc    license.py      setup.py        tests.py*       unit/
CXX/            Makefile        TODO_TESTS      build/          doc/            matplotlibrc.template   setupext.py     tools/
INSTALL         README.osx      __pycache__/        dist/           examples/       release/        setupext.pyc        tox.ini

This is on MacOS X, but subprocess.Popen shows exactly the same behaviour on Debian 7, and according to the docs on every Unix system.

Cheers,

Derek

Owner

mdboom replied Apr 16, 2013

Confirmed. I've opened #1911 to track this.

Contributor

mgiuca-google replied Apr 16, 2013

Oh, I see, I removed shell=True in my patch. No idea why I did that or if it was an accident. Oops. I'll fix it (properly). Edit: Thanks for finding this, @dhomeier.

Member

pelson replied Apr 17, 2013

All fixed in master. Thanks everyone.

Contributor

daradib commented Mar 3, 2014

Looks like 70f2296 added the line CalledProcessError = subprocess.CalledProcessError to compat/subprocess.py which causes an exception to raised since the attribute doesn't exist.

Owner

mdboom commented Mar 3, 2014

@daradib: If you're a Google App Engine user, would you mind writing up a PR that works around the issue?

Contributor

daradib commented Mar 3, 2014

@mdboom Ok, sure. I submitted #2860 to the maintenance branch because it's a tiny bugfix. If I should set it to master, let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment