Shouldn't use pexpect for subprocesses in in-process terminal frontend #297

Closed
minrk opened this Issue Mar 23, 2011 · 17 comments

Comments

Projects
None yet
3 participants
@minrk
Member

minrk commented Mar 23, 2011

Doing things like:

In [1]: !nano testfile.py

Will do crazy and wrong things in the terminal, after the move to pexpect for all subprocesses.

This must be fixed.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Apr 8, 2011

Member

Yes, I'll work on this.

Member

fperez commented Apr 8, 2011

Yes, I'll work on this.

@ghost ghost assigned fperez Apr 10, 2011

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver May 3, 2011

Member

@fperez: Have you had a chance to look at this? Is it worth blocking 0.11 on it?

Member

takluyver commented May 3, 2011

@fperez: Have you had a chance to look at this? Is it worth blocking 0.11 on it?

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk May 3, 2011

Member

This is a pretty big deal for people who use shell escapes to terminal programs that accept input (less/more, vi, emacs, etc.), which is now completely broken.

Though it's possible we can bump it to 0.12 if we want to hasten the 0.11 release as a tech preview. It depends on how many people rely on this, and would be super angry if it broke in a released version of IPython.

Member

minrk commented May 3, 2011

This is a pretty big deal for people who use shell escapes to terminal programs that accept input (less/more, vi, emacs, etc.), which is now completely broken.

Though it's possible we can bump it to 0.12 if we want to hasten the 0.11 release as a tech preview. It depends on how many people rely on this, and would be super angry if it broke in a released version of IPython.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver May 3, 2011

Member

Do we have a feel for how difficult it's likely to be to fix it?

Member

takluyver commented May 3, 2011

Do we have a feel for how difficult it's likely to be to fix it?

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk May 19, 2011

Member

It's super easy to fix. There just needs to be a configurable that selects the call made by InteractiveShell.system, and points it to os.system instead of the pexpecty utils.process.system (or the configurable updates utils.process.system itself).

Member

minrk commented May 19, 2011

It's super easy to fix. There just needs to be a configurable that selects the call made by InteractiveShell.system, and points it to os.system instead of the pexpecty utils.process.system (or the configurable updates utils.process.system itself).

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver May 19, 2011

Member

So what's the reason we're using pexpect for this in the first place? What can we do with it that we can't with os.system?

Member

takluyver commented May 19, 2011

So what's the reason we're using pexpect for this in the first place? What can we do with it that we can't with os.system?

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk May 19, 2011

Member

forwarding stdout/err over the wire is the main reason, I think.

For example, do os.system('ls') in the qt console. The output will be in the launching terminal window. You need to use Popen/pexpect or some such in order to get redirection of stdout. Of course, in a single-process terminal IPython redirection is irrelevant, so os.system is fine.

Member

minrk commented May 19, 2011

forwarding stdout/err over the wire is the main reason, I think.

For example, do os.system('ls') in the qt console. The output will be in the launching terminal window. You need to use Popen/pexpect or some such in order to get redirection of stdout. Of course, in a single-process terminal IPython redirection is irrelevant, so os.system is fine.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver May 19, 2011

Member

Ah, I see. So would it make sense to have the method in the core interactiveshell simply call os.system, and then override it in zmqshell with the pexpect/Popen method we need there?

Member

takluyver commented May 19, 2011

Ah, I see. So would it make sense to have the method in the core interactiveshell simply call os.system, and then override it in zmqshell with the pexpect/Popen method we need there?

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk May 19, 2011

Member

I don't think there's a reason to override the method, because then changing behavior depends on subclassing. A simple configurable would cover it just fine. That way code that embeds an InteractiveShell can decide whether it should use os.system or utils.process.system without needing a subclass.

Member

minrk commented May 19, 2011

I don't think there's a reason to override the method, because then changing behavior depends on subclassing. A simple configurable would cover it just fine. That way code that embeds an InteractiveShell can decide whether it should use os.system or utils.process.system without needing a subclass.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver May 19, 2011

Member

But it's not really a configuration issue, is it? Where we're running in-process, os.system is more capable. Where we're talking to a kernel, we need pexpect/Popen to redirect the output. I think making it configurable is just adding complexity.

Member

takluyver commented May 19, 2011

But it's not really a configuration issue, is it? Where we're running in-process, os.system is more capable. Where we're talking to a kernel, we need pexpect/Popen to redirect the output. I think making it configurable is just adding complexity.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk May 19, 2011

Member

Except it's less complex than having to subclass if you want different behavior...

For instance, what if you have an InteractiveShell, and you want to capture all output (e.g. for testing, or embedded use)? os.system can't work for that, so you would have to subclass.

It's also true that the utils.process.system on Windows does some extra things, so it would likely be a preferable default there. We certainly shouldn't be using different classes on Windows, just because their defaults might differ, right?

Member

minrk commented May 19, 2011

Except it's less complex than having to subclass if you want different behavior...

For instance, what if you have an InteractiveShell, and you want to capture all output (e.g. for testing, or embedded use)? os.system can't work for that, so you would have to subclass.

It's also true that the utils.process.system on Windows does some extra things, so it would likely be a preferable default there. We certainly shouldn't be using different classes on Windows, just because their defaults might differ, right?

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver May 19, 2011

Member

If I'm writing an application to embed IPython, I think I'd find it easier to subclass (which works as any Python programmer would expect) than to work out how to interface with our config system. You don't even need to subclass, you can just reassign the method: myip.system = myip.system_piped (hypothetically). To me, the question for config is: would the user (the one typing commands in) ever want to change this?

As for Windows: it would be easy enough to set things up so that the command was run inside that config manager either way.

Member

takluyver commented May 19, 2011

If I'm writing an application to embed IPython, I think I'd find it easier to subclass (which works as any Python programmer would expect) than to work out how to interface with our config system. You don't even need to subclass, you can just reassign the method: myip.system = myip.system_piped (hypothetically). To me, the question for config is: would the user (the one typing commands in) ever want to change this?

As for Windows: it would be easy enough to set things up so that the command was run inside that config manager either way.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk May 19, 2011

Member

'interfacing with our config system' is a matter of setting an attribute value. 'myshell.use_os_system=True' is not exactly complex, and certainly less complex than subclassing. You don't have to understand how configurables work (or even know that they exist) to use it.

I see it another way - if one object is trivially configurable to cover multiple cases (as is the case here), why add the artificially cumbersome requirement of subclassing to change behavior?

If we do go the subclass route, it's actually going to be the case that the current behavior should be the default, and the single-process terminal should override (only on posix), and select os.system. Because right now we have a variety of ways to launch kernels, and every single one requires that stdout be captured and/or forwarded except for the single-process terminal.

Note that simply overriding shell.system=os.system is not exactly the same, because the IPython shell escape also does an expand_vars on the command, allowing variables, etc.

Member

minrk commented May 19, 2011

'interfacing with our config system' is a matter of setting an attribute value. 'myshell.use_os_system=True' is not exactly complex, and certainly less complex than subclassing. You don't have to understand how configurables work (or even know that they exist) to use it.

I see it another way - if one object is trivially configurable to cover multiple cases (as is the case here), why add the artificially cumbersome requirement of subclassing to change behavior?

If we do go the subclass route, it's actually going to be the case that the current behavior should be the default, and the single-process terminal should override (only on posix), and select os.system. Because right now we have a variety of ways to launch kernels, and every single one requires that stdout be captured and/or forwarded except for the single-process terminal.

Note that simply overriding shell.system=os.system is not exactly the same, because the IPython shell escape also does an expand_vars on the command, allowing variables, etc.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver May 19, 2011

Member

But why set an attribute to tell it to change another attribute, when you can simply change the attribute directly?

I'm not suggesting simply replacing myshell.system=os.system. I'm suggesting that we have methods for "piped system" and "raw system", which can be assigned to myshell.system. We don't need a configurable variable to control toggling them if we can just switch the method directly.

We have a variety of ways to launch kernels, but shouldn't they all be using the IPython.zmq.zmqshell.ZMQInteractiveShell class?

Member

takluyver commented May 19, 2011

But why set an attribute to tell it to change another attribute, when you can simply change the attribute directly?

I'm not suggesting simply replacing myshell.system=os.system. I'm suggesting that we have methods for "piped system" and "raw system", which can be assigned to myshell.system. We don't need a configurable variable to control toggling them if we can just switch the method directly.

We have a variety of ways to launch kernels, but shouldn't they all be using the IPython.zmq.zmqshell.ZMQInteractiveShell class?

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk May 19, 2011

Member

Ah, fair enough. So we should implement it just like having a configurable (the system method, in this case), but don't actually have it be a configurable.

That seems clean enough to me, I'll make a PR following your proposal.

Member

minrk commented May 19, 2011

Ah, fair enough. So we should implement it just like having a configurable (the system method, in this case), but don't actually have it be a configurable.

That seems clean enough to me, I'll make a PR following your proposal.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver May 20, 2011

Member

OK, thanks. I look forward to reviewing it.

Member

takluyver commented May 20, 2011

OK, thanks. I look forward to reviewing it.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver May 26, 2011

Member

(See Min's PR #459)

Member

takluyver commented May 26, 2011

(See Min's PR #459)

@minrk minrk closed this in bdb012c May 27, 2011

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this issue Nov 3, 2014

split shell.system into shell.system_raw/system_piped
system_raw calls os.system, system_piped uses pexpect/utils.platform magic

use system_raw in Terminal except on Windows and in tests
use system_piped elsewhere

closes gh-297
closes gh-457
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment