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

Check proper verbosity of mbar #379

Open
mrshirts opened this issue Mar 28, 2020 · 12 comments · May be fixed by #391
Open

Check proper verbosity of mbar #379

mrshirts opened this issue Mar 28, 2020 · 12 comments · May be fixed by #391
Assignees
Labels

Comments

@mrshirts
Copy link
Collaborator

Doesn't appear to be printing out much anymore when verbose=True is set.

@mrshirts
Copy link
Collaborator Author

OK, the issue is that setting verbose sends the desired text to logger.info, but doesn't actually print it. Presumably, if one sets verbose, one wants that info to be printed out in some way, but right now, it's getting lost. @jaimergp , suggestions for sending that information to where the user can see it?

@jaimergp
Copy link
Member

jaimergp commented Mar 29, 2020

The logging module works on the assumption that emission of messages is decoupled of delivery. In this decoupling, libraries are only emitters, and it's up to the applications to configure how to deliver the message. So the user needs to configure logging to print stuff if they want to (usually, logging.basicConfig() is enough).

Since pymbar is a library, does it make sense for us to configure the verbosity? Do we have an CLI entry point? We can configure logging globally with some settings (e.g. with a singleton like pymbar.settings.verbose = True, but in principle we shouldn't.

@jaimergp
Copy link
Member

jaimergp commented Mar 29, 2020

That's why all these if verbose: checks do not make much sense in our case. We can (and should) leave all the logger.{info,debug,warn,error} calls unchecked and, if anything, configure the verbosity at a global level.

@mrshirts
Copy link
Collaborator Author

So what would your suggestion be to have the code perform how people would expect, that if they give a verbosity argument, it increases the amount of information out? What hierarchy of flags to use? Where and when to send the output out (right now, most of the output occurs near the data that is being generated).

@jaimergp
Copy link
Member

Ideally, there would be no verbose flag at the function/class level. Users are expected to set the logging level in their scripts if desired.

import logging
logging.basicConfig(level=logging.DEBUG)

# proceed with your pymbar calling code

To transition from the current approach, last version of pymbar 3 should include a deprecation warning for verbose=True:

>>> some_result = some_function(verbose=True)
DeprecationWarning("Pymbar v4.0+ will use `logging` for debug, informative reports and warnings. To increase output verbosity in the future, please use `logging.basicConfig(level=logging.<DESIREDLEVEL>)` in your scripts")

IIRC, there's a context manager you can use for temporary verbosity increase. The main idea is that emission/delivery are uncoupled in terms of responsibility, but it still happens synchronously if you want to. There's more info here (check the "Using handlers" section).

@mrshirts
Copy link
Collaborator Author

I'm OK with that. However, note that a lot of people (including me) are operating in the "UNIX command" mentality, where you trigger a flag to increase verbosity. Adding functionality doesn't always improve user experience as one would like if people are not used to using the functionality.

So it will have to really be well documented that one does this, and the default level should probably be verbose on, with maybe a command itself telling people how to adjust the verbosity (i.e., we set the default level, and tell them how to change it).

@jaimergp
Copy link
Member

Yes, I totally agree. Those "app-level" flags can still be set up internally with the context manager if necessary. That's why I have been added all those "ideally". I understand it's a big change for the end user who just wants some stuff printed to stdout.

I'll try to come up with a design that addresses all of our concerns. Now that we have logging, all the configuration possibilities are there. We just need to agree on the best default config for our use case (which should be as close to the pymbar3 behavior as possible).

@mrshirts
Copy link
Collaborator Author

Now that the PMF reformatting is in, we are getting closer to getting something that can be pymbar 4.0. @jaimergp, do you have a suggested design here for the context manager in terms of app-level flags to print expected output from the logger?

@jaimergp
Copy link
Member

I have been thinking about this, and we might want to implement a package-level Settings instance that can configure the logging for all pymbar to a sensible default. Changing the value on the Settings instance will propagate that to the logger automatically.

With respect to the context manager for the verbose flag, we could provide something like this.

@jaimergp
Copy link
Member

I can try to implement this next week, if that's ok?

@mrshirts
Copy link
Collaborator Author

I'll defer to you on what makes the most sense and is most maintainable going forward.

@jaimergp jaimergp linked a pull request Apr 27, 2020 that will close this issue
@jaimergp jaimergp linked a pull request Apr 27, 2020 that will close this issue
@jaimergp
Copy link
Member

Check #391 !

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 a pull request may close this issue.

2 participants