Skip to content

Provide default progress loggers #21

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

Merged
merged 8 commits into from
Mar 6, 2020
Merged

Provide default progress loggers #21

merged 8 commits into from
Mar 6, 2020

Conversation

devmotion
Copy link
Member

@devmotion devmotion commented Mar 5, 2020

This PR enables default progress loggers that (should) only handle progress logs generated by AbstractMCMC without handling or modifying any other user generated logs. It is based on the following heuristic:

  • If the log level of the current logger is <= -1 (the level of the progress logs), it is used and no custom logger is generated. Of course, this is a rough heuristic and ideally we would like to know if the progress logs can be handled, but it can be checked efficiently and it seems kind of reasonable to assume that a logger with log level <= -1 should be able to handle progress logs (I'm not sure if any other "standard" logging functionality apart from progress logging uses log level -1?).
  • If the current logger seems to be unable to handle progress logs, we instead use a logger that wraps a progress logger and the current logger. Logs are then filtered according to their log level and the module they are generated in (which can be done without actually evaluating the log message), and only logs with the progress log level that are generated in AbstractMCMC are forwarded to the custom progress logger. All other logs are still handled by the original logger.
  • The custom default progress logger is a ConsoleProgressMonitors.ProgressLogger in IJulia and TerminalLoggers.TerminalLogger in all other cases, due to display issues of TerminalLogger in IJulia.

The main disadvantages of this approach are, as far as I can see:

  • It is based on heuristics, so it might break sometimes.
  • It requires additional dependencies ConsoleProgressMonitor, TerminalLoggers, and LoggingExtras.

This PR supersedes #19 and replaces #20.

In my local tests it seems to work as expected inside of Juno, the REPL, and within a Jupyter notebook. I still have to figure out proper CI tests.

@codecov
Copy link

codecov bot commented Mar 5, 2020

Codecov Report

Merging #21 into master will increase coverage by 0.43%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #21      +/-   ##
==========================================
+ Coverage   92.42%   92.85%   +0.43%     
==========================================
  Files           1        1              
  Lines          66       70       +4     
==========================================
+ Hits           61       65       +4     
  Misses          5        5
Impacted Files Coverage Δ
src/AbstractMCMC.jl 92.85% <100%> (+0.43%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c867a65...930708c. Read the comment docs.

@devmotion
Copy link
Member Author

The Travis CI tests show at least that a TerminalLogger is used although it is not set explicitly.

@cpfiffer
Copy link
Member

cpfiffer commented Mar 6, 2020

This seems to be quite an elegant solution, good fix.

@cpfiffer
Copy link
Member

cpfiffer commented Mar 6, 2020

Anything else you want to get into this PR or can I merge it over?

@devmotion
Copy link
Member Author

I just checked that it seems to work as expected when used with downstream packages such as EllipticalSliceSampling (to minimize the possibility that we missed something in the tests). I think this PR can be merged.

However, now that I became a bit more familiar with the logging system and we have a (hopefully) reasonable way of providing default loggers, I started wondering if we should remove the whole if progress block in @ifwithprogresslogger and remove all progress based checks, and instead exploit the logging system a bit more. The main idea would be:

  • If the user specifies progress = true (the default), we apply the heuristic that is added in this PR and just add a default progress logger if the current logger seems to be unable to handle progress messages
  • If the user specifies progress = false, we filter the progress logs generated by AbstractMCMC using an EarlyFilteredLogger that checks the module and the log level of the logs, but only if the minimum log level of the current logger is <= -1 (since then it might handle the progress logs).

If feels like this would clean up the code quite a bit. Moreover, the filtering and dropping of logs happens on the level of the metadata of the logs that can be generated and evaluated efficiently, before the actual log message is evaluated.

@cpfiffer
Copy link
Member

cpfiffer commented Mar 6, 2020

If it'll help things, sure.

@devmotion
Copy link
Member Author

I'll play around with the idea and benchmark it, to make sure it's actually not affecting performance.

@devmotion
Copy link
Member Author

But I think it should be part of a subsequent PR.

@cpfiffer
Copy link
Member

cpfiffer commented Mar 6, 2020

Cool, I'll merge for now. Thanks for this one, @devmotion.

@cpfiffer cpfiffer merged commit b29b38d into master Mar 6, 2020
@delete-merged-branch delete-merged-branch bot deleted the filter branch March 6, 2020 14:54
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.

2 participants