Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

forward stdout from forked processes #2791

Merged
merged 18 commits into from Apr 11, 2013
Merged

Conversation

minrk
Copy link
Member

@minrk minrk commented Jan 15, 2013

third attempt, in collaboration with @piti118

uses zmq instead of multiprocessing, because mp has too many issues.

  • messages are sent via PUSH/PULL from subprocesses
  • messages are sent at flush time, not at write time
  • subprocess messages
  • no threads, no sync events, etc.

some basic tests are included

todo:

  • test on Windows
  • resolve issues with inprocess kernels

@minrk
Copy link
Member Author

minrk commented Jan 15, 2013

supercedes #2734

@minrk
Copy link
Member Author

minrk commented Jan 18, 2013

Come to think of it, I don't think this can work on Windows, due to the lack of a real fork.
I will confirm that (and fix the new failures due to the merge of inprocess) tomorrow.

@minrk
Copy link
Member Author

minrk commented Jan 18, 2013

last failure was a travis timeout - it actually passes now.

@piti118
Copy link
Contributor

piti118 commented Feb 4, 2013

multiprocessing.Pool.map seems to do something funny with stdout

import multiprocessing as mp
def f(x):
    print x
    return x
pool = mp.Pool()
pool.map(f, range(200))

running on terminal will print the numbers but not in this patch.

May be we miss a flush on join or something along that line?

@minrk
Copy link
Member Author

minrk commented Feb 4, 2013

Must be a missed flush.

@minrk
Copy link
Member Author

minrk commented Feb 4, 2013

adding an explicit flush results in expected output

@piti118
Copy link
Contributor

piti118 commented Feb 4, 2013

I know explicit flush will fix the issue. And this is probably second order issue.

@minrk
Copy link
Member Author

minrk commented Feb 4, 2013

It should be a bit better behaved now. I made \n imply flush from the subprocesses, so at least every complete line should arrive in tact.

@piti118
Copy link
Contributor

piti118 commented Feb 6, 2013

This may not has to do with this PR but I notice that sometimes python warning(written to stderr) isn't flushed at the end of code cell and it leaks to the next cell execution instead.

@minrk
Copy link
Member Author

minrk commented Feb 6, 2013

That would not be related to this. It would be exceedingly peculiar, since sys.stderr is flushed at the end of all execute requests. Does this warning perhaps come from a thread?

@ellisonbg
Copy link
Member

@minrk I would like to help review this but it looks like pretty complex code in some critical parts of the code base. I would like to understand the motivation/design better. Maybe we could Skype later to go over the design?

@satra
Copy link
Contributor

satra commented Mar 25, 2013

works for me!

@Carreau
Copy link
Member

Carreau commented Mar 27, 2013

@minrk @ellisonbg did you had a chance to Skype to discuss that, is this ok to merge ?

@minrk
Copy link
Member Author

minrk commented Mar 27, 2013

We haven't had a chance - I think it's ready, but I need someone else's eyes on it since it's my code.

@satra
Copy link
Contributor

satra commented Mar 27, 2013

and i didn't really look through the code closely - just tested it empirically on a problem i was having.

@ellisonbg
Copy link
Member

OK @minrk I am plugging back into code reviews and will be one HipChat most of the week and available on skype all days but Tuesday 4/9.

@minrk
Copy link
Member Author

minrk commented Apr 9, 2013

Ok, hopefully we can get this in tomorrow. See you on hip chat.

@ellisonbg
Copy link
Member

I am guessing that my best times will be 1-3 and 7-9. Cheers, Brian

"""flush any messages waiting on the queue"""
for channel in (KM.shell_channel, KM.iopub_channel):
for channel in (km.shell_channel, km.iopub_channel):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this will have to change after the kernel manager PR is merged.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there will need to be some rewriting - that PR (and the two that depend on it) change so much, that I am running out of stuff I can reasonably do before they are merged.

@ellisonbg
Copy link
Member

I left a few minor comments, but the code looks really good. I am +1 for merging this after you look at my mostly trivial comments.

minrk added a commit that referenced this pull request Apr 11, 2013
forward stdout from forked processes

uses zmq instead of multiprocessing, because mp has too many issues.

- messages are sent via PUSH/PULL from subprocesses
- messages are sent at flush time, not at write time
- subprocess messages
- no threads, no sync events, etc.

some basic tests are included

closes #2438
@minrk minrk merged commit ad7b12c into ipython:master Apr 11, 2013
@minrk minrk deleted the forksafe_nomp branch March 31, 2014 23:36
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
forward stdout from forked processes

uses zmq instead of multiprocessing, because mp has too many issues.

- messages are sent via PUSH/PULL from subprocesses
- messages are sent at flush time, not at write time
- subprocess messages
- no threads, no sync events, etc.

some basic tests are included

closes ipython#2438
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants