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

Configure logging with support for verbose flags #391

Open
wants to merge 5 commits into
base: pymbar4
Choose a base branch
from

Conversation

jaimergp
Copy link
Member

Addresses #379

@jaimergp jaimergp linked an issue Apr 27, 2020 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Apr 27, 2020

Codecov Report

Merging #391 into pymbar4 will increase coverage by 0.37%.
The diff coverage is 66.66%.

Copy link
Collaborator

@mrshirts mrshirts left a comment

Choose a reason for hiding this comment

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

Is there a way we can make this a bit more concise? As it is, it doubles the number of lines to add a comment, which adds significant code length with just duplicate lines? Maybe there can be a wrapper function in utils that wraps this, so one can just give the comment and the level? (maybe that gets in the way of the use of name)?

@jaimergp
Copy link
Member Author

jaimergp commented Apr 27, 2020

That's a good point. I think I can come up with something...

The level of the message is independent of the verbose flag. In other words, verbose does not control when a messages gets logged, but when the message is emitted (in our case, to stdout by default). So what we are doing now is temporarily changing the emission threshold down from warning to info. That's something we can't workaround because that's the backend for the logging mechanism.

However, I think we could wrap up this logic in a nicer way. Some quick ideas, let me know which one you like more:

# A
with emit_if_verbose(__name__, verbose=verbose):
    logger.info("Something something")

# B
logger.log("Something something", level=logging.INFO, force_emit=verbose)

I tend to prefer A because it's less hacky and more aligned with the logging design principles.

@mrshirts
Copy link
Collaborator

I think I prefer the second?

I think the second part is to document somewhere how people ask for emission of the other levels.

@jaimergp
Copy link
Member Author

Let me know what you think about it. It's a bit hacky, but hopefully not too much :)

My only concern is what happens when pymbar is used in another project that is already using logging without our custom subclass. Maybe that extra keyword can cause problems if it's not consumed before hand... but maybe the logging machinery is isolated enough? We'll see during the rc stage I guess.

@mrshirts
Copy link
Collaborator

mrshirts commented May 5, 2020

Anything else needed for this?

@jaimergp
Copy link
Member Author

jaimergp commented May 6, 2020

Ideally, some manual testing, to see if the logging machinery behaves the way a user would expect. I did a couple of quick ones with the examples and was successful, but I'd like more feedback if possible.

@jaimergp
Copy link
Member Author

@mrshirts anything else you want to add here?

@jaimergp
Copy link
Member Author

This is needs some more testing in real world conditions, ideally in a notebook or somewhere where you are using other libraries at the same time. Does anybody have a good example?

@Lnaden
Copy link
Contributor

Lnaden commented Nov 12, 2020

My one question with this, is there a reason you cannot add a different level for verbosity? Since INFO, DEBUG, etc are all just int, can we not make a level for VERBOSE and then if verbose we just set the logger level to that? I think that would be much simpler than carrying around the force_emit=verbose in all the calls.

@mrshirts
Copy link
Collaborator

The one thing that I want to make sure of is it's very clear to users in the documentation how to make a given function call verbose.

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

Successfully merging this pull request may close these issues.

Check proper verbosity of mbar
4 participants