-
Notifications
You must be signed in to change notification settings - Fork 526
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
Overhaul Pyomo logging configuration #1797
Conversation
The reworks the Pyomo logger to more closely adhere to the python logging conventions while maintaining backwards compatibility. It moves much of the logic out of the LogHandler class and into the WrappingFormatter. It better bahaves with other systems by suppressing pyomo log output if a root log handler is defined.
Codecov Report
@@ Coverage Diff @@
## master #1797 +/- ##
==========================================
- Coverage 75.14% 74.71% -0.44%
==========================================
Files 638 638
Lines 91904 92030 +126
==========================================
- Hits 69065 68760 -305
- Misses 22839 23270 +431
Continue to review full report at Codecov.
|
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 found a couple typos but otherwise I think this looks fine
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.
Resolved @blnicho comments.
@@ -727,33 +727,24 @@ def test_inf_bounds_on_expr(self): | |||
self.assertEqual(lb, None) | |||
self.assertEqual(ub, None) | |||
|
|||
@unittest.skip('This test passes locally, but not on travis or appveyor. I will add an issue.') |
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'm guessing this is a sneaky bug fix?
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.
It is actually a fix to the implementation of the test (leveraging the LoggingIntercept
context manager resolves portability problems)
Fixes #1760
Summary/Motivation:
The original Pyomo logging configuration was heavily geared around the
pyomo
script and did not conform to the general behavior expected by users. For example, callinglogging.basicConfig()
was effectively ignored by the Pyomo loggers. This PR does an overhaul of the Pyomo logger configuration to more closely follow the accepted (and expected) Python logging behavior. In particular, the bulk of the logic is now implemented through Formatter and Filter objects (allowing users to reuse the formatting in their own Handlers), and the Pyomo default logging will defers to a root logger if the user / calling environment created one.This also creates a standardized
is_debug_set
function (inpyomo.common.log
) to replace our use ofif __debug__ and logger.isEnabledFor(logging.DEBUG)
. The old idiom would generate debugging log records when the root logger was left in "NOTSET" (which is the case not what the core Pyomo log configuration does not explicitly set a level). The new function preserves the old behavior (of only emitting DEBUG log records when the user explicitly set a debugging level) by only returningTrue
if the effective level is inNOTSET < level <= DEBUG
.This should be fully-backwards compatible with the existing logging setup (in that existing scripts that log correctly should continue to log in the same manner).
Changes proposed in this PR:
is_debug_set
function (and propagate throughout code base)LogHandler
class andconfigure_loggers
functionLegal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: