p_iter_fork doesn't flush stdout properly

[sagetrac-johanbosman]

The following code

from sage.parallel.use_fork import p_iter_fork

def f(n):
    print "Entering f with n=%s" % n 
    X = p_iter_fork(1, verbose=True)
    iter_result = X(g, [((n,), {})])
    result = list(iter_result)[0][1]
    print "Leaving f with n=%s" % n
    return result  # Which is just n
    
def g(n):
    print "Inside g with n=%s" % n
    return n
    
def test():
    X = p_iter_fork(2, verbose=True)
    inputs = [((n,), {}) for n in range(5)]
    iter_result = X(f, inputs)
    for n in iter_result:
        pass  

test()

gives this output (perhaps in a different order):

Entering f with n=1
Entering f with n=1
Inside g with n=1
Leaving f with n=1
Entering f with n=0
Entering f with n=0
Inside g with n=0
Leaving f with n=0
Entering f with n=2
Entering f with n=2
Inside g with n=2
Leaving f with n=2
Entering f with n=3
Entering f with n=3
Inside g with n=3
Leaving f with n=3
Entering f with n=4
Entering f with n=4
Inside g with n=4
Leaving f with n=4

The point is that stdout isn't flushed before the process is forked and therefore its contents are copied to the child process as well and thus lives in 2 processes.

---

Apply

1. attachment: 11778_flush_before_fork.patch
2. attachment: trac_11778-flush_output_of_print_stmts.reviewer.patch
   to the Sage library.

Component: misc

Keywords: parallel fork stdout

Author: Johan Bosman

Reviewer: Leif Leonhardy

Merged: sage-4.7.2.alpha3

Issue created by migration from https://trac.sagemath.org/ticket/11778

[sagetrac-johanbosman]

comment:1

The attached patch fixes the problem, but it probably still needs a doctest.

[nexttime]

comment:2

How about stderr?

[nexttime]

Author: Johan Bosman

[nexttime]

Reviewer: Leif Leonhardy

[nexttime]

comment:4

In general I'd move the flush(s) out of the loop.

[sagetrac-johanbosman]

comment:5

Replying to @nexttime:

In general I'd move the flush(s) out of the loop.

Okay. ;)

[sagetrac-johanbosman]

comment:6

Attachment: 11778_flush_before_fork.patch.gz

And now stderr as well.

[nexttime]

comment:7

Replying to @sagetrac-johanbosman:

And now stderr as well.

Sorry, not your problem here, but IMHO the output should also be flushed after every print statement (in verbose mode), e.g. when processes get killed.

[nexttime]

comment:8

Oh, actually not just verbose mode.

[sagetrac-johanbosman]

comment:9

Okay, so you're saying this stderr flush can be left out? Flushing is time consuming so it'd be better to flush as little as possible.

[nexttime]

comment:10

Replying to @sagetrac-johanbosman:

Okay, so you're saying this stderr flush can be left out?

No. The subprocesses shouldn't inherit an unflushed buffer.

I can add a reviewer patch for flushing the output of the print statements, unless you want to change your patch.

[sagetrac-johanbosman]

comment:11

Okay, go ahead; I don't want to change this patch. ;).

[nexttime]

Sage library patch. Also flushs the output of further print statements in the parent process. Apply on top of Johan's patch.

[nexttime]

comment:12

Attachment: trac_11778-flush_output_of_print_stmts.reviewer.patch.gz

Replying to @sagetrac-johanbosman:

Okay, go ahead; I don't want to change this patch. ;).

Gone.

If you're ok with my additional changes, we have a positive review.

[sagetrac-johanbosman]

comment:13

There's a typo (which was already there):
In "Killing subprocess %s with input %s which took too long" there should be a comma between the second %s and "which".

Okay, that was "mierenneuken" (you may want to look up that word); I'll have a more serious look it later on (don't have much time right now).

[nexttime]

comment:14

Replying to @sagetrac-johanbosman:

There's a typo (which was already there):
In "Killing subprocess %s with input %s which took too long" there should be a comma between the second %s and "which".

I'd say a comma there isn't absolutely necessary, but I should have added periods... (I saw that of course^TM after I had committed my changes.)

Okay, that was "mierenneuken" (you may want to look up that word).

"De pagina "mierenneuken" aanmaken op deze wiki."

Guessing nit-picking.

I'll have a more serious look it later on (don't have much time right now).

Go ahead... ;-)

[sagetrac-johanbosman]

comment:15

Replying to @nexttime:

I'd say a comma there isn't absolutely necessary, but I should have added periods... (I saw that of course^TM after I had committed my changes.)

Have a look at http://en.wikipedia.org/wiki/Comma#Separation_of_clauses ;).

[nexttime]

comment:16

Replying to @sagetrac-johanbosman:

Replying to @nexttime:

I'd say a comma there isn't absolutely necessary, but I should have added periods... (I saw that of course^TM after I had committed my changes.)

Have a look at http://en.wikipedia.org/wiki/Comma#Separation_of_clauses ;).

Well, since "which" refers to a single subprocess, it is unambiguous.

(But you're in principle right, omitting the comma might be considered a bit sloppy, as the clause is non-restrictive from a grammatical point of view. I'd say it is restrictive, or constraining, from a logical point of view though, as the subprocess wouldn't have been killed in that situation otherwise, i.e. if it had not taken too long.)

We could also change "which took too long" to "because it took too long"... :)

[sagetrac-johanbosman]

comment:17

Replying to @nexttime:

Replying to @sagetrac-johanbosman:

Okay, go ahead; I don't want to change this patch. ;).

Gone.

If you're ok with my additional changes, we have a positive review.

I'm okay with it. Let's not worry about the comma, which is mierenneuken anyway (the word is explained at http://en.wikipedia.org/wiki/Dutch_profanity) ;).

[nexttime]

Merged: sage-4.7.2.alpha3