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

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

Merged
merged 3 commits into from
Feb 2, 2022

Conversation

lgarrison
Copy link
Collaborator

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
Copy link

pep8speaks commented Dec 21, 2021

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

Corrfunc/utils.py Outdated Show resolved Hide resolved
@manodeep
Copy link
Owner

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
Copy link
Collaborator Author

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 manodeep merged commit 68973d0 into master Feb 2, 2022
@manodeep
Copy link
Owner

manodeep commented Feb 2, 2022

Great - merged!

@manodeep manodeep deleted the fix-std-redir branch May 12, 2022 03:29
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.

3 participants