manodeep / Corrfunc

⚡️⚡️⚡️Blazing fast correlation functions on the CPU.
https://corrfunc.readthedocs.io
MIT License
163 stars 50 forks source link

Add additional check to tell if it's safe to redirect stdout/err #270

Closed lgarrison closed 2 years ago

lgarrison commented 2 years ago

From #269, invoking a Corrfunc script as python -u script.py &> out.txt causes the script to hang. Something about the combination of redirection and unbuffered output was causing trouble. This change adds an additional check, sys.stdout is sys.__stdout__, to see if Python's sys.stdout has been redirected elsewhere, like a Jupyter cell, in which case we want to follow that redirect. Otherwise, we don't touch anything.

I don't exactly understand why unbuffered output causes a problem when buffered output did not, but I'm pretty confident that this new check more accurately reflects the scenarios in which we actually want redirection. This new check probably supersedes the sys.stdout.isatty() check, but to be conservative, we'll keep both.

Minimal reproducer, note it is completely unrelated to Corrfunc:

import sys

import wurlitzer

with wurlitzer.pipes(stderr=sys.stderr):
    print('hello, there!', file=sys.stderr)
$ python repro.py 2> eout.txt  # fine
$ python -u repro.py 2> eout.txt  # hangs

This probably reflects my ignorance around pipes and fd's more than anything else!

pep8speaks commented 2 years ago

Hello @lgarrison! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 1032:80: E501 line too long (84 > 79 characters) Line 1033:80: E501 line too long (83 > 79 characters) Line 1034:80: E501 line too long (86 > 79 characters) Line 1035:80: E501 line too long (84 > 79 characters) Line 1036:80: E501 line too long (82 > 79 characters) Line 1037:80: E501 line too long (83 > 79 characters) Line 1049:1: W293 blank line contains whitespace

Comment last updated at 2022-01-31 15:09:40 UTC
manodeep commented 2 years ago

I still do not understand why this works (well, really why there is a bug in the first place). @lgarrison Do you think this is okay to merge?

lgarrison commented 2 years ago

Interestingly, both of the following commands now hang for me, while previously only the second one did:

$ python repro.py 2> eout.txt
$ python -u repro.py 2> eout.txt

Something about my environment has probably changed since I first looked into this issue (I did check that PYTHONUNBUFFERED is not set).

But in any case, the fix remains valid, if only for the reason that it avoids invoking Wurlitzer in a scenario Wurlitzer tells us it cannot handle. So this is ready for merge.

manodeep commented 2 years ago

Great - merged!