run() - unified high-level interface for subprocess

[takluyver]

BPO	23342
Nosy	@warsaw, @gpshead, @ncoghlan, @bitdancer, @ethanfurman, @takluyver, @berkerpeksag, @vadmium
Files	subprocess_run.patch subprocess_run2.patch subprocess_run3.patch subprocess_run4.patch process.py subprocess_run5.patch subprocess_run6a.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/gpshead'
closed_at = <Date 2015-04-26.05:13:16.179>
created_at = <Date 2015-01-28.22:13:38.931>
labels = ['type-feature', 'library']
title = 'run() - unified high-level interface for subprocess'
updated_at = <Date 2016-05-18.05:14:24.030>
user = 'https://github.com/takluyver'

bugs.python.org fields:

activity = <Date 2016-05-18.05:14:24.030>
actor = 'ncoghlan'
assignee = 'gregory.p.smith'
closed = True
closed_date = <Date 2015-04-26.05:13:16.179>
closer = 'gregory.p.smith'
components = ['Library (Lib)']
creation = <Date 2015-01-28.22:13:38.931>
creator = 'takluyver'
dependencies = []
files = ['37897', '37899', '37991', '38072', '38075', '38574', '38997']
hgrepos = []
issue_num = 23342
keywords = ['patch']
message_count = 29.0
messages = ['234918', '234922', '234923', '234924', '234925', '234927', '235093', '235133', '235134', '235300', '235653', '235654', '235656', '235659', '235663', '235726', '236189', '236319', '237079', '238589', '238591', '240274', '240276', '240277', '240961', '241053', '241054', '242032', '265807']
nosy_count = 11.0
nosy_names = ['barry', 'gregory.p.smith', 'ncoghlan', 'r.david.murray', 'cvrebert', 'ethan.furman', 'python-dev', 'takluyver', 'berker.peksag', 'martin.panter', 'Jeff.Hammel']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue23342'
versions = ['Python 3.5']

[takluyver]

This follows on from the python-ideas thread starting here: https://mail.python.org/pipermail/python-ideas/2015-January/031479.html

subprocess gains:

• A CompletedProcess class representing a process that has finished, with attributes args, returncode, stdout and stderr
• A run() function which runs a process to completion and returns a CompletedProcess instance, aiming to unify the functionality of call, check_call and check_output
• CalledProcessError and TimeoutExceeded now have a stderr attribute, to avoid throwing away potentially relevant information.

Things I'm not sure about:

1. Should run() capture stdout/stderr by default? I opted not to, for consistency with Popen and with shells.
2. I gave run() a check_returncode parameter, but it feels quite a long name for a parameter. Is 'check' clear enough to use as the parameter name?
3. Popen has an 'args' attribute, while CalledProcessError and TimeoutExpired have 'cmd'. CompletedProcess sits between those cases, so which name should it use? For now, it's args.

[takluyver]

Another question: With this patch, CalledProcessError and TimeoutExceeded exceptions now have attributes called output and stderr. It would seem less surprising for output to be called stdout, but we can't break existing code that relies on the output attribute.

Using properties, either stdout or output could be made an alias for the other, so both names work. Is this desirable?

[gpshead]

A 1) Opting not to capture by default is good. Let people explicitly request that.

A 2) "check" seems like a reasonable parameter name for the "should i raise if rc != 0" bool. I don't have any other good bikeshed name suggestions.

A 3) Calling it args the same way Popen does is consistent. That the attribute on the exceptions is 'cmd' is a bit of an old wart but seems reasonable. Neither the name 'args' or 'cmd' is actually good for any use in subprocess as it is already an unfortunately multi-typed parameter. It can either be a string or it can be a sequence of strings. The documentation is not clear about what type(s) 'cmd' may be.

A Another) Now that they gain a stderr attribute, having a corresponding stdout one would make sense. Implement it as a property and document it with a versionadded 3.5 as usual.

[ethanfurman]

I haven't checked the code, but does check_output and friends combine stdout and stderr when ouput=PIPE?

[takluyver]

Updated patch following Gregory's suggestions:

• The check_returncode parameter is now called check. The method on CompletedProcess is still check_returncode, though.
• Clarified the docs about args
• CalledProcessError and TimeoutExceeded gain a stdout property as an alias of output

Ethan: to combine stdout and stderr in check_output, you need to pass stderr=subprocess.STDOUT - it doesn't assume you want that.

I did consider having a simplified interface so you could pass e.g. capture='combine', or capture='stdout', but I don't think the brevity is worth the loss of flexibility.

[gpshead]

Ethan: check_output combines them when stdout=subprocess.STDOUT is passed (
https://docs.python.org/3.5/library/subprocess.html#subprocess.STDOUT).
Never pass stdout=PIPE or stderr= PIPE to call() or check*() methods as
that will lead to a deadlock when a pipe buffer fills up. check_output()
won't even allow you pass in stdout as it needs to set that to PIPE
internally, but you could still do the wrong thing and pass stderr=PIPE
without it warning you.

the documentation tells people not to do this. i don't recall why we
haven't made it warn or raise when someone tries. (but that should be a
separate issue/change)

On Wed Jan 28 2015 at 3:30:59 PM Ethan Furman <report@bugs.python.org>
wrote:

Ethan Furman added the comment:

I haven't checked the code, but does check_output and friends combine
stdout and stderr when ouput=PIPE?

----------

---

Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue23342\>

---

[vadmium]

Maybe you don’t want to touch the implementation of the “older high-level API” for fear of subtly breaking something, but for clarification, and perhaps documentation, would the old functions now be equivalent to this?

def call(***):
    # Verify PIPE not in (stdout, stderr) if needed
    return run(***).returncode
def check_call(***):
    # Verify PIPE not in (stdout, stderr) if needed
    run(***, check=True)
def check_output(***):
    # Verify stderr != PIPE if needed
    return run(***, check=True, stdout=PIPE)

If they are largely equivalent, perhaps simplify the documentation of them in terms of run(), and move them closer to the run() documentation.

Is it worth making the CalledProcessError exception a subclass of CompletedProcess? They seem to be basically storing the same information.

[takluyver]

Yep, they are pretty much equivalent to those, except:

• check_call has a 'return 0' if it succeeds
• add '.stdout' to the end of the expression for check_output

I'll work on documenting the trio in those terms.

If people want, some/all of the trio could also be implemented on top of run(). check_output() would be the most likely candidate for this, since I copied that code to create run(). I'd probably leave call and check_call as separate implementations to avoid subtle bugs, though.

Sharing inheritance between CalledProcessError and CompletedProcess: That would mean that either CompletedProcess is an exception class, even though it's not used as such, or CalledProcessError uses multiple inheritance. I think duplicating a few attributes is preferable to having to think about multiple inheritance, especially since the names aren't all the same (cmd vs args, output vs stdout).

[vadmium]

It’s okay to leave them as independent classes, if you don’t want multiple inheritance. I was just putting the idea out there. It is a similar pattern to the HTTPError exception and HTTPResponse return value for urlopen().

[takluyver]

Third version of the patch (subprocess_run3):

• Simplifies the documentation of the trio (call, check_call, check_output) to describe them in terms of the equivalent run() call.
• Remove a warning about using PIPE with check_output - I believe this was already incorrect, since check_output uses .communicate() internally, it shouldn't have deadlock issues.
• Replace the implementation of check_output() with a call to run().

I didn't reimplement call or check_call - as previously discussed, they are more different from the code in run(), so subtly breaking things is more possible. They are also simpler.

[takluyver]

Would anyone like to do further review of this - or commit it ;-) ?

I don't think anyone has objected to the concept since I brought it up on python-ideas, but if anyone is -1, please say so.

[vadmium]

Have you seen the code review comments on the Rietveld, <https://bugs.python.org/review/23342\>? (Maybe check spam emails.) Many of the comments from the earlier patches still stand. In particular, I would like to see the “input” default value addressed, at least for the new run() function, if not the old check_output() function.