Closed Carreau closed 7 months ago
Thanks for raising the issue and also providing a quick fix in the linked ipython issue.
This context manager is designed to suppress the output of Fortran and C code called from HoloPy, but they operate at a low level so it's not easy to do from within python, hence the current system. It's been iterated a few times with previous versions causing rare crashes or not always fully suppressing output. HoloPy needs to be able to run across a few different platforms, such as jupyter notebooks, local python scripts, and distributed HPC jobs and it's tricky to get output suppression working consistently.
Holopy causing ipython to crash is a problem that should be fixed, but I would recommend anyone handle this issue carefully since a lot of effort went into getting the current behaviour to work as consistently as it does.
We have similar logic in ipykernel to actually capture fortran stdout that avoid Python's sys.stdout, and end up in the terminal instead of notebooks, so yes that kind of things is indeed quite brittle.
If there are any changes on IPython side to make things easier let me know.
Hi @Carreau
Thanks for the help with this! I believe I'm the person to blame for this code snippet. I use ipython frequently so I did run into this problem before on my machine.
My recollection is that the issue is closing stdout, which ipython wants open (as you say). As @barkls says part of the issue is that there are many different ways this is called (jupyter notebooks, native python, ipython, and the python CI on travis), all of which behave slightly differently. We did some iteration to make this work on 3 of the above 4, but not on ipython.
Can you elaborate on your proposed solution? If I recall commenting out sys.stdout.close()
fixes the ipython crash but causes other problems on other systems.
I'm not too sure what problem not closing sys.stdout
creates, and I understand that it may, though without knowing what problems it creates, I'm not sure my solution is the right either for the full ecosystem.
I know that intercepting compiled code output to stdout is complicated as most of the time, it will directly open fd=1 and write to it, unless the stream is closed, which is why I guess you closed it here. So my guess again, is that if you don't close it there are some cases where the output is not properly redirected to devnull. I might be wrong though.
At the same time the close() call seem to directly come from the original example in https://eli.thegreenplace.net/2015/redirecting-all-kinds-of-stdout-in-python/ (according to git blame), so maybe it's not actually necessary ?
I did try removing the sys.stdout.close()
, but it meant that output rarely wasn't suppressed (0.1%-1% occurence). See resolved comment thread on #335. I never did figure out what was going wrong in those rare cases.
There is probably an alternate solution that doesn't break ipython, but unfortunately both @briandleahy and myself have moved on to other projects so it's unlikely to be fixed anytime soon.
See https://github.com/ipython/ipython/issues/13223
Smallest reproductible example:
My understanding is that in
It's ok to replace it, but you should restore it after the context manager. and in general you should not close an object you did not open as someone else might have references to ti.
Storing sys.stdout in enter, and restoring it in exit seem to work.