-
Notifications
You must be signed in to change notification settings - Fork 157
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 Logging #875
Add Logging #875
Conversation
pyAMG prints diagnostics on its own, so can't be logged
mpilogging doesn't seem to be in pypi yet. Is it supposed to be? |
No. Bootstrapping issue. I'd like somebody else's impression before I transfer it to |
>>> logfile.setLevel(logging.DEBUG) | ||
>>> log.addHandler(logfile) | ||
|
||
>>> log.setLevel(logging.DEBUG) |
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 it necessary to set the level twice?
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's a debatable point. One establishes what logging messages are captured by a particular handler; the other establishes what messages get generated at all.
Would is make sense to include the version number of the dependencies at the start of the log file in debug mode(like for |
Do you not get something like
logged at the top of a run? |
Funny that you asked, but yes I do! I thought it was a good idea, but didn't think you would also have this good idea. |
|
||
_log = logging.getLogger(__name__) | ||
|
||
from ._version import get_versions |
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.
Maybe at top of file
FYI, there's a stray |
from fipy.tools.logging import package_info | ||
|
||
_log.info(package_info()) | ||
del package_info |
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 see you are deleting these modules hence why you're doing the import close by
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.
Yes, but I guess it would be cleaner overall to do all the imports together and just make sure the namespace is clean at the end.
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've collected most of the imports at top, but these are a little trickier. Any logging configuration should be in effect before doing anything that might cause logging.
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 JSON is misdefined, but otherwise this LGTM.
See also a tiny new
mpilogging
package.