-
Notifications
You must be signed in to change notification settings - Fork 24
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
Use standard logger in log function #158
Conversation
8cc9f58
to
974f4aa
Compare
core/logging.py
Outdated
self._logger.addHandler(self._stdout_handler) | ||
|
||
# Update the stdout handler level if required | ||
if stdout_level != self._stdout_handler.level: |
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 would sugget this correction:
if stdout_level != 28 - self._stdout_handler.level:
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.
Thanks, good spot - updated
974f4aa
to
6073be1
Compare
bb30972
to
195f611
Compare
3f63ebe
to
831c851
Compare
831c851
to
919ec0d
Compare
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 looks good to me.
Description
We are currently considering how to improve the PLAMS logging for a better user experience.
This includes having logging of job summaries etc. as in #156
As a first step though, a refactor of the existing
log
method in PLAMS. This currently had it's own logging implementation to write to stdout and a file, including handling the thread safety etc.This change maintains the current behaviour exactly, but instead uses the standard python logger and formatter etc. This is more a best-practice change at the moment, but also provides a pattern for adding other logger types.
Implementation
A wrapper class
Logger
has been added toscm.plams.core.logging
. It is a thin wrapper over the standard library logger, which has an interface optimised for the current usage in PLAMS. It has aconfigure
method and alog
method which are both used by the generallog
function.Note that the configure methods are called very frequently (every time log is called), but they only update the handlers if there is an actual change to the config. This is required as the current behaviour is to log to the global config default job manger, which can be changed dynamically in the script. So this can't be a one-time-setup type of configuration.
Instead of instantiating the
Logger
directly, aLogManager
static class has also been added. This allows the loggers to be accessed by name and makes sure they are singletons. This is exposed to the user via aget_logger
helper method.Example
An example usage is as follows:
Testing
Logging tests were added for
log
function, which passed before and after this change.Specific tests have also been added for the
Logger
class.In addition, the scripting tests for PLAMS were run, which should flag any issues as the diffs would be incorrect
Future Extensions
It would now be possible to add a
get_csv_logger
method which could return a csv logger based on a similar pattern to the current logger. There could also be an (abstract) base class for the different logger types.