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

make HARK less "verbose" -- introduce logger #701

Closed
wants to merge 8 commits into from

Conversation

sbenthall
Copy link
Contributor

…with variable configuration levels. Fixes #283

The way you can change the verbosity level at the top of a notebook is now, e.g.:

import HARK.logger as logger
logger.verbose()

or

import HARK.logger as logger
logger.quiet()

I've shown this at the top of one of the example notebooks to demonstrate.

This PR so far only introduces the logger's functionality for the PerfForesightConsumerType and IndShockConsumerType, which have implemented checkConditions() methods.
If the design passes review, I can change other print statements to logging.info statements.

@sbenthall sbenthall requested a review from llorracc May 26, 2020 19:54
@MridulS
Copy link
Member

MridulS commented May 26, 2020

I probably missed this conversation regarding this but shouldn't

import HARK.logger as logger

be used in ConsIndShockModel.py?

Users should use something like

IndShockConsumerType(verbose=True)

instead of creating a "global" loggers while creating an example/notebook?

@sbenthall
Copy link
Contributor Author

@MridulS No, I don't think so.

In this, PR, ConsIndShockModel.py imports logging, and then uses logging.info, logging.warning, or logging.debug instead of print statements.

This effectively puts the logging module, which is a Python standard library, as an intermediary between these statements and STERR. (With a little more sophisticated tuning, it could pipe these messages to STOUT, but this is the simplest implementation currently.)

This is done "globally" because that's the simplest implementation. If we needed something more complex, we could do it. But that would require more expressing logging configuration.

I think it's not smart to have verbosity be a model parameter.

Logging configuration should be done at the global level because really it's a configuration of the Python environment's I/O. Specifically, the O. Trying to patch it through the program logic creates a lot of messy interaction between model, controller, and view logic. It's harder to maintain and makes it difficult to give the user the ability to turn verbosity on or off.

@sbenthall
Copy link
Contributor Author

Oh, hmmm. @MridulS you might be right about something actually.

Really, it depends on what the default behavior. This is disputed. See #699

@sbenthall sbenthall mentioned this pull request May 28, 2020
@sbenthall
Copy link
Contributor Author

I'll add the logger to the init file as per this PR from Mridul

https://github.com/pymc-devs/pymc3/pull/1416/files

@sbenthall
Copy link
Contributor Author

In the latest commit to this PR, the logging configuration is moved to HARK.__init__
Now this code is executed when HARK packages are imported.

For library code, the HARK logger needs to be invoked explicitly. I.e.:

from HARK import _log

_log.info("An informative message")

The HARK user does not need to do anything extra to run this code! This is good.

However: in the current PR:

  • Most (but not all) of the checkConditions messages are at the INFO level
  • This INFO level is logged to the user by default for HARK.
  • checkConditions is run automatically before solve() is called.

This means that for the current PR, the user will automatically see the checkConditions() messages.
This I believe satisfies the requirements of #283 but contradicts #699

This looks like a difference of opinion between @mnwhite and @llorracc and I defer to their consensus on the final implementation.

@mnwhite
Copy link
Contributor

mnwhite commented Jun 4, 2020

I need to check the boolean flags for verbosity the default dictionaries; there is no disagreement otherwise.

@sbenthall sbenthall removed the request for review from llorracc June 11, 2020 14:30
@llorracc
Copy link
Collaborator

This is superseded by #714

@llorracc llorracc closed this Jun 18, 2020
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.

replace print with logging, with 'quiet' and 'verbose' logging configuration options.
4 participants