-
Notifications
You must be signed in to change notification settings - Fork 529
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
Ensure capture_output
does not output to captured file descriptors
#3537
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment/question.
pyomo/common/tee.py
Outdated
# if not hasattr(stream, 'write'): | ||
# raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this debugging code that should be removed or history that could use context? If it can be removed then you could combine this except
with the one below for OSError
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can be removed. At one point I was using that to better ensure that the stream was an actual stream (I think I was accidentally getting stream to be a tuple of streams, but that is handled elsewhere now.
Fixes #3520
Summary/Motivation:
This further improves
capture_output
whencapture_fd=True
. The core issue was that we were creating output loops when capturing the file descriptor ifcapture_output
was capturing a file descriptor shared (directly or indirectly through a logger) with the stream that it was sending output to. While this solution is not bulletproof (I am not sure that we can ever be bullet proof against things like user-defined classes), this is significantly more robust than the previous implementations.This primarily resolves issues with solvers where
tee
is implemented withcapture_fd=True
(cyipopt, highs).Thanks to @jlinderoth for finding the issue and providing the MRE.
Changes proposed in this PR:
LoggingIntercept
capture_output
to better guarantee all contexts are unwound correctlyLegal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: