Skip to content
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

Improve log messages #96

Merged
merged 2 commits into from Feb 15, 2022
Merged

Improve log messages #96

merged 2 commits into from Feb 15, 2022

Conversation

pluegh
Copy link
Contributor

@pluegh pluegh commented Jan 24, 2022

This pull request changes the way printing status messages is handled.

  1. All print functionality is replaced by the logging standard library.
    Messages that were wrapped with if(verbose) are logged as information, or as warnings otherwise.
  2. The new function util.log_to_stdout sets up a basic logging handler for easy standalone usage of pypmc.
    This function is called in __init__.py as a drop-in replacement of the current behaviour.

Comments

  • Every submodule uses its own logger such that the level of granularity is still high.
    E.g. to only log warnings and errors from PMC, use logging.getLogger('pypmc.mix_adapt.pmc').setLevel(logging.WARNING).
  • Note that the default behaviour of logging when no log handler is specified, is to print warnings and errors to stderr.
  • Importing pypmc is setting up a log handler (to stdout), which must be removed explicity to use the module as a library.

WIP

  • Adding the default StreamHandler creates uncaptured output in the nosetests, as warnings log to stderr. To comply with the previous code, we specify to log only to stdout.
  • Emit deprecation notice for optional verbose argument instead of removing the option (breaking change).
  • Document logging behaviour and how to use pypmc as a library.
    The wiki page is a good place to document logging after the proposed changes are approved.

Solves #95

@fredRos
Copy link
Collaborator

fredRos commented Jan 27, 2022

@pluegh First of thanks a lot for starting this pull request. I hope I will find some time over the weekend to have a more detailed look but I support the idea of replacing prints with logging.

Out of curiosity: how would this help with eos?

@dvandyk
Copy link
Collaborator

dvandyk commented Jan 27, 2022

Out of curiosity: how would this help with eos?

@fredRos EOS now pipes all log messages into a logging.logger when used from within IPython or a Jupyter notebook.
We can easily turn off the most intrusive log messages, but switching pypmc's (very useful) diagnostic messages, e.g. in the PMC update steps, cannot be handled in the same way yet. I've asked @pluegh to fix that, and he delivered!

pypmc/util.py Outdated Show resolved Hide resolved
@pluegh pluegh changed the title (WIP) Improve log messages Improve log messages Feb 6, 2022
@pluegh pluegh marked this pull request as ready for review February 6, 2022 13:25
@@ -5,6 +5,9 @@
import numpy as np
import scipy.linalg as linalg

import logging
logger = logging.getLogger(__name__)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This creates sub loggers, with a name of pypmc.mix_adapt.hierarchical, correct?
Is there an easy way to get to all loggers that belong to the pypmc root logger?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.
Checking the logging documentation, I don't think it's designed such that you can get a list of all child loggers. Instead, you are supposed to configure the parent logger (pypmc or pypmc.mix_adapt). This has the advantage that child loggers, which are instantiated later also adhere to the parent.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird, but apparently the recommended approach.

Copy link
Collaborator

@dvandyk dvandyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve, but I would like to have also @fredRos to approve, if possible.

- Add a deprecation notice for the optional `verbose` argument
- Create `tools/util.py` to contain technical helpers such as
  the deprecation warning used in various submodules.
@dvandyk
Copy link
Collaborator

dvandyk commented Feb 15, 2022

@fredRos merging now. Please holler if you find something untoward a-posterior 😄

@dvandyk dvandyk merged commit 331b5b9 into pypmc:master Feb 15, 2022
@fredRos
Copy link
Collaborator

fredRos commented Feb 15, 2022

@dvandyk Sorry, I was overwhelmed with work and family. I know git revert but I'm pretty sure we won't need that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants