TuringLang / AbstractMCMC.jl

Abstract types and interfaces for Markov chain Monte Carlo methods
https://turinglang.org/AbstractMCMC.jl
MIT License
87 stars 18 forks source link

ProgressLogging default environments #18

Closed cpfiffer closed 4 years ago

cpfiffer commented 4 years ago

I just noticed that the new ProgressLogging.jl backend does not actually have any display functionality by default, and that downstream users do not get any automatic progress bar updates.

Is there a way to detect which environment someone is in (either Juno or a terminal) and to use the appropriate progress viewer automatically? I wasn't able to find a good way to do so.

devmotion commented 4 years ago

Actually, IMO detecting the environment and using a specific logging viewer is against the purpose of ProgressLogging. The purpose of ProgressLogging is to provide an easy, backend-agnostic way of defining progress logging with the help of the Julia logging system. Users then can decide if they, e.g., want to drop the logging messages, archive them to files, or display them in the terminal.

That being said, Juno will display a native progress bar automatically. For displaying progress bars in the REPL, users can install an appropriate logger such as TerminalLogger globally

using TerminalLoggers: TerminalLogger
using Logging: global_logger

global_logger(TerminalLogger())

or only locally when calling sample

using TerminalLoggers: TerminalLogger
using Logging: with_logger

with_logger(TerminalLogger()) do
    sample(....)
end

If TerminalLogger should be enabled by default, it can be loaded in the startup file ~/.julia/config/startup.jl.

In principle, this also works with Jupyter notebooks, however, the output of TerminalLogger seems to fill up the the notebook since it is not cleared. As a workaround, one can use ConsoleProgressMonitor.jl which uses ProgressMeter for printing progress bars (but is superseded by TerminalLoggers). Again, ConsoloeProgressMonitor could be loaded globally for every Jupyter notebook by modifying the startup file ~/.julia/config/startup_ijulia.jl.

devmotion commented 4 years ago

See also https://github.com/c42f/TerminalLoggers.jl/issues/25

cpfiffer commented 4 years ago

One of the big reasons we provided ProgressMeter in the first place was to show sampling as it occurred by default. As much as I like hands-off options, Turing (and others) have progress bars by default because many people would rather Turing "just worked" rather than specify their own terminal logger. I think we should consider enforcing a default so that there is literally any kind of visual representation of progress, rather than just a blank screen for seconds or minutes.

cpfiffer commented 4 years ago

We could have something like

function sample(. . .; logger=TerminalLogger(right_justify=120))
    . . . 
end

And then there's a nice default for those who don't care enough to set this moderately arcane setting in their startup file, and those who want special loggers can just define what they want.

devmotion commented 4 years ago

I think the correct approach would be to increase the log level such that the progress is always displayed as a regular log message as a fallback, in case this is desired.

The advantage of using the logging system is that it supports multiple backends and environments (such as Juno and REPL), but by providing a default logger we interfere with user-defined setups in a way that removes this advantage completely. I'm not even sure how you would have to define the logger if you are using Juno, and in general anyone who does not want to use TerminalLogger would have to provide their own logger (even if it is just the standard logger for catching other error messages!).

cpfiffer commented 4 years ago

We could also just check whether the current logger is the default logger, so we don't ignore people who've got their own setup:

if Loggers.global_logger() isa Loggers.ConsoleLogger
    local_logger = TerminalLogger()
end

I think it's worth spending time on figuring how Juno should fit into this. I think showing progress bars is pretty damn close to a requirement for something like Turing -- it's used in basically every other PPL with zero work on the part of users. Having a progress bar is a strong quality signal -- spitting out log messages is a very tacky thing to do for what is essentially the only responsive UI element anybody who uses Turing is exposed to.

devmotion commented 4 years ago

We could also just check whether the current logger is the default logger, so we don't ignore people who've got their own setup:

This is prone to errors and almost guaranteed to break (e.g., if Juno is used; someone has changed some settings such as the log level of the standard logger on purpose and hence actually wants to use it; and more importantly someone has set a local logger such as a ConsoleProgressMonitor in a Jupyter notebook that should not be replaced with a TerminalLogger), and hence IMO there is no good reason for trying to be clever at this point. The most natural and intended setup is to let the user pick whatever backend they want to use (which is even set up automatically if they are using Juno).

I think showing progress bars is pretty damn close to a requirement for something like Turing -- it's used in basically every other PPL with zero work on the part of users. Having a progress bar is a strong quality signal -- spitting out log messages is a very tacky thing to do for what is essentially the only responsive UI element anybody who uses Turing is exposed to.

First of all, I think in AbstractMCMC the focus should be on defining generally applicable sensible defaults without concentrating on Turing only. To me, changing the logger is definitely not a sensible default setting, and hence should be done at most in Turing itself. However, as explained above, IMO not even Turing should mess around with the loggers in whatever more or less clever way since IMO this would be unintended and unexpected behaviour when calling sample.

In my opinion, it is not unreasonable to expect from users to use and load custom loggers if they do not use Juno and want to print a progress bar in the REPL - as mentioned above, this can be done even automatically by modifying the Julia config file. Turing requires you to implement your probabilistic models anyway, so why should it be problematic if a user has to add two more lines that load ConsoleProgressMonitor or TerminalLoggers, as explained in the documentation? JuliaDiffEq and Flux use the same approach, so why should it be different in Turing?

If the default output should be more verbose on the REPL to provide visual feedback also to users that do not set a specific logger (I'm not sure if that's actually a good idea), then increasing the loglevel would e.g. result in output such as

┌ Info: Sampling
└   progress = 0.94
┌ Info: Sampling
└   progress = 0.95
┌ Info: Sampling
└   progress = 0.96
┌ Info: Sampling
└   progress = 0.97
┌ Info: Sampling
└   progress = 0.98
┌ Info: Sampling
└   progress = 0.99
┌ Info: Sampling
└   progress = 1.0
┌ Info: Sampling
└   progress = "done"
cpfiffer commented 4 years ago

This is prone to errors and almost guaranteed to break (e.g., if Juno is used; someone has changed some settings such as the log level of the standard logger on purpose and hence actually wants to use it; and more importantly someone has set a local logger such as a ConsoleProgressMonitor in a Jupyter notebook that should not be replaced with a TerminalLogger), and hence IMO there is no good reason for trying to be clever at this point. The most natural and intended setup is to let the user pick whatever backend they want to use (which is even set up automatically if they are using Juno).

I want to be quite clear here -- I agree with you that what I am proposing is not ideal from an engineering standpoint. A perfect system filled with sophisticated software people would just require all of the downstream users to set their own loggers. That would be great, and it's a fine idea if that is who all of our users were.

But that is not the case. What you are proposing here is essentially a regressive tax on our most inexperienced users, who are often new to Julia. None of these users are likely to touch their startup.jl file for a long time, the same way that many new vim users do not touch their .vimrc file. I certainly did not look at my startup.jl file until months after I had started seriously using Julia. Each of these new users will be confronted with a black screen while their models spin away for minutes -- this makes Turing and all other packages that inherit AbstractMCMC seem slow! More importantly, many of these people are the marginal users who will make future decisions about continuing to use Turing (and other packages) based on an interaction that takes perhaps several minutes at best. The people who already highly familiar with both Julia and Turing may be the only ones who every actually see a progress bar, and those are the very same people who don't really care that much.

We do not offer just another package. If that were the case, sure, whatever, set your own logger. Turing is a comprehensive suite of statistical analysis tools, and it should behave as such! That means we should make an effort (what I view as a very small effort) to provide incredibly rudimentary features like progress bars or telling people when they have numerical errors. We want the statisticians who are not really the best programmers as much as we want the 1% of stats people who also happen to be Julia experts. If that means we have to pull away from the Flux/DiffEq world a bit, I am more than happy to do so.

If I'm being honest, I think this is one thing that the ecosystem has wrong -- progress meters are essentially the most basic way to signal quality. If this were Python or R (or anything else that faces users), we would likely not ever have this discussion -- everyone would just agree that the package should write a very small amount of code to say "Hey, I know you asked me to do that thing, and I am now doing it. Hold tight".

I'm fine with ProgressLogging if it just dispatches to the appropriate logging environment. If we're in the REPL, it should go to TerminalLoggers. If we're in Jupyer, it should go to ConsoleProgressMonitor. If we're in Juno, it's fine. If someone has anything other than the default ConsoleLogger, we should give them the most useful way to show progress.

But it doesn't do that! It just throws all these messages into a blank pit by default, and we assume that users will "just know" that they have a way to get those messages out of the pit. I don't know about you, but when I first learned programming, and even when I learn a new language, I do not know that there is a pit, that all my messages go into that pit, and that there's a way to get stuff out of the pit.

Sure, we could just stick Logging.global_logger(ConsoleLogger()) at the top of every tutorial, but that's really not a great fix. It's boilerplate code that we can just try to remove everywhere with a few lines of code. If it doesn't work, fine. But I think writing off this concept in total is foolish and alienating to inexperienced users who are most in need of a responsive system.

First of all, I think in AbstractMCMC the focus should be on defining generally applicable sensible defaults without concentrating on Turing only. To me, changing the logger is definitely not a sensible default setting, and hence should be done at most in Turing itself. However, as explained above, IMO not even Turing should mess around with the loggers in whatever more or less clever way since IMO this would be unintended and unexpected behaviour when calling sample.

I think progress meters are a generally applicable default in this setting, particularly for all MCMC methods. That's why progress monitoring was included in the first place -- everyone wants to know how far along their sampling is and they may not have the know-how to add the additional line to their startup file, or to every single script they write. At the very least, this should be a default for Turing, if not for AbstractMCMC.

I'm fine with backing off here if I am strongly in the minority, but I think we should keep an eye on the newest members of Julia. Every time we make something more engineer-friendly without providing a useful default for the less sophisticated, we introduce yet another friction to someone who is brand new to Turing and to Julia. We do not have to be some ultra-sleek DIY package that is completely free of defaults that make the lives of the uninitiated easy. The whole point of all of this is to make things easy! We wouldn't write an interface package, or Turing, or AdvancedHMC, or any of the other tools we provided if everyone just rolled their own stuff. If it's low cost and trivial it should just be a part of the system.

devmotion commented 4 years ago

I completely agree that we should help inexperienced Julia users and provide them with a good user experience. Sorry if that was not clear from my comments above.

I'm still wondering how we could get the best of both worlds - providing a reasonable default without any configuration and not interfering with the logging system too much.

As far as I see, we could check the combination of the user environment and the current logger, and if they seem unsuitable for progress logging

  1. display a warning with detailed instructions for how to fix/load a proper progress logging frontend for the current user environment,

  2. or display a warning and switch the current logger to a suitable progress logging frontend during sampling.

The first approach

The second approach

Somewhat in between these two solutions would be the approach to

  1. display a warning with detailed instructions for how to fix/load a proper progress logging frontend for the current user environment, and display the progress in form of simple log messages (with the new ProgressLogging API that would change to [ Info: Sampling: 91% etc BTW)

This approach

Even though it is not perfect, I think the third approach could provide a reasonable trade-off between providing every user with some feedback about the progress of sampling and avoiding any potentially unintended hacks.

devmotion commented 4 years ago

More concretely, without any additional dependencies we use something along the lines of

function isprogresslogfiltered()
    # assume Juno works fine
    isdefined(Main, :Atom) && return false

    # otherwise check if progress logs are filtered
    logger = Logging.current_logger()
    Logging.min_enabled_level(logger) < ProgressLogging.ProgressLevel && return false

    # display warning and add instructions for how to install a backend
    if isdefined(Main, :IJulia)
        @warn "Progress bars can not be displayed. Please install a progress logging frontend such as [ConsoleProgressMonitor.jl](https://github.com/tkf/ConsoleProgressMonitor.jl)."
    else
        @warn "Progress bars can not be displayed. Please install a progress logging frontend such as [TerminalLoggers.jl](https://github.com/c42f/TerminalLoggers.jl)."
    end

    return true
end

The return value could be used to update progess such that progress logs are not generated if they are filtered by the current logger.

devmotion commented 4 years ago

OK, as I expected initially, such a simple check won't work.

The problem with the code above is that neither the current logger nor the global logger need to be able to display the progress logs, it would be sufficient if any logger in the dynamical stack of loggers is suitable - but that's something we can't (?) check. According to the documentation we also shouldn't try to do it, since

logging statements make no mention of where log events go or how they are processed. This is a key design feature that makes the system composable and natural for concurrent use.

So another idea would be to abuse Requires and set a flag if a combination of packages is loaded that seems to work. I'll see if I can get something more reliable.

cpfiffer commented 4 years ago

Couldn't we just have

function sample(. . .;
    logger=global_logger() == Logging.ConsoleLogger(stderr) ? 
         TerminalLogger : 
         global_logger()
)
    with_logger(logger) do
        # sampling stuff
    end
end

In this case we check if the global logger is anything other than the default (set here). If someone's set it to something else, we use that. If not, we add TerminalLogger as a dependency and just use that instead. Then, we never overwrite the global logger, and only use a task-specific logger. A user who really cares about this kind of thing can just pass in their own logger.

devmotion commented 4 years ago

That test is not reliable since it is not unlikely that one uses TerminalLogger or some other custom logger as a task-specific logger (and also Juno does not change it). I think there is no better way to check the availability of loggers than checking if the packages were loaded (assuming that an inexperienced user might not load them).

cpfiffer commented 4 years ago

Well, current_logger is an alternative, since it returns either the current task logger or the global logger if there is no task logger.

devmotion commented 4 years ago

No, unfortunately it's not an alternative, it does not work for the very same reason.

The logging system by design allows an extreme amount of composability, so that you can run some parts of your code with a TerminalLogger in place but still handle, e.g., @debug statements inside of it with a nested standard ConsoleLogger. When people start specializing or requiring certain loggers in their packages, these stacks of loggers are created quite naturally. So both checking the current and the global logger will miss out any logger in between those two that could handle progress messages.

I still believe, if our main concern are inexperienced users that do not know about Julia's logging system, our best guess is to assume that these users have problems since they do not load ConsoleProgressMonitor or TerminalLoggers. I think that's a quite reasonable assumption, and hence at the moment I'm tending towards just checking if one of these packages was loaded, and presenting a warning otherwise. However, maybe there are some issues with this approach as well that I don't see right now :woman_shrugging:

All in all, I'm very much in favour of presenting a warning/explanation/help/fix to new users but I strongly believe that in a package such as AbstractMCMC that is supposed to be reused and extended by many other packages that has to be done in a way that does not break the code of people that use the logging system correctly in mysterious and unexpected ways (such as, e.g., a keyword argument would).

cpfiffer commented 4 years ago

I think there is just some very large point here that I am not seeing, and I think I should drop this whole issue. I don't have a clear picture in my head of why any of this was implemented, though obviously there are many reasons which you have been kind enough to outline in this thread. I appreciate that you've taken the time to explain this all to me in great detail.

Can you add a section about how to get progress bars to show up to the Turing docs?

devmotion commented 4 years ago

There's a rudimentary section but I guess we should extend that and maybe provide some examples?

cpfiffer commented 4 years ago

Yeah, it should have a block to show how to use it in Jupyter and the terminal separately, so people can just copy the code.

devmotion commented 4 years ago

I think we can close this for now, hopefully we came up with a reasonable solution.

cpfiffer commented 4 years ago

Yeah, I think what you've got is quite elegant.