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

automatic cross platform progress monitoring #450

Merged
merged 2 commits into from
Mar 9, 2020
Merged

Conversation

ChrisRackauckas
Copy link
Member

@ChrisRackauckas ChrisRackauckas commented Mar 9, 2020

@pfitzseb @tkf @devmotion @c42f any issues you see with doing this kind of thing?

@ChrisRackauckas
Copy link
Member Author

No noticable overhead:

using OrdinaryDiffEq
using BenchmarkTools

@btime solve(
    ODEProblem((u, p, t) -> -u, 1.0, nothing),
    Euler();
    dt = 0.5,
    tspan = (0.0, 0.1),
    #progress = true,
    #progress_steps = 1,
)

# Before
44.600 μs (334 allocations: 26.61 KiB)
45.900 μs (334 allocations: 26.61 KiB)

# After
45.000 μs (338 allocations: 26.75 KiB)
46.500 μs (338 allocations: 26.75 KiB)

@ChrisRackauckas
Copy link
Member Author

progress

@ChrisRackauckas
Copy link
Member Author

@davidanthoff is there a logger that needs to be set with VSCode?

@davidanthoff
Copy link
Contributor

We don't have any progress UI support in VS Code.

@c42f
Copy link

c42f commented Mar 9, 2020

I don't think this is a good solution.

Setting logger sinks should be controlled at the top application level and never in libraries. This allows libraries to compose together while giving the application control of the log stream. What if people want to send informational messages from DiffEq to a file? (Or from a user defined function DiffEq calls?) Overriding the global logger sink prevents this.

It also forces all users of DiffEqBase to install Atom and Juno which seems far from ideal.

In the case of interactive work, the "application level" for the purposes of user code is the REPL in use. So it's appropriate for

  • Juno to provide a default logger (and a way to configure a custom one)
  • The standard REPL to provide a default logger, and allow users to override it in their startup.jl
  • VSCode to provide a default logger (and a way to configure it)

In summary, if you're having trouble with Juno + TerminalLoggers, we need to figure out what's going wrong and sort it out at the application level. Not in the libraries which the user may happen to have loaded.

@ChrisRackauckas
Copy link
Member Author

It also forces all users of DiffEqBase to install Atom and Juno which seems far from ideal.

No, it definitely does not do that...

Setting logger sinks should be controlled at the top application level and never in libraries. This allows libraries to compose together while giving the application control of the log stream. What if people want to send informational messages from DiffEq to a file? (Or from a user defined function DiffEq calls?) Overriding the global logger sink prevents this.

What about having the logger as a keyword argument? I'm looking for a solution for today, and a discussion for tomorrow.

@@ -255,3 +255,6 @@ prob2dtmin(tspan, onet, ::Any) = onet*1//10^(10)
timedepentdtmin(integrator::DEIntegrator) = timedepentdtmin(integrator.t, integrator.opts.dtmin)
timedepentdtmin(t::AbstractFloat, dtmin) = abs(max(eps(t), dtmin))
timedepentdtmin(::Any, dtmin) = abs(dtmin)

maybe_with_logger(f, logger) = logger === nothing ? f() : with_logger(f, logger)
default_logger() = Juno.isactive() ? nothing : TerminalLogger()
Copy link
Contributor

@tkf tkf Mar 9, 2020

Choose a reason for hiding this comment

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

Maybe
Suggested change
default_logger() = Juno.isactive() ? nothing : TerminalLogger()
default_logger() = Juno.isactive() || current_logger() isa TerminalLogger ? nothing : TerminalLogger()

or, even more hacky "solution" that does not require Juno:

Suggested change
default_logger() = Juno.isactive() ? nothing : TerminalLogger()
default_logger() =
Logging.min_enabled_level(current_logger()) == LogLevel(-1) ? nothing : TerminalLogger()

Copy link
Member Author

Choose a reason for hiding this comment

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

how does the latter work with Juno?

Copy link
Contributor

@tkf tkf Mar 9, 2020

Choose a reason for hiding this comment

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

Actually, I think @devmotion's solution using LoggingExtras.TeeLogger is much better SciML/DifferentialEquations.jl#424 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

how does the latter work with Juno?

The log level of the JunoProgressLogger is guaranteed to be <= -1: https://github.com/JunoLab/Atom.jl/blob/016327893e4c2a22688446758debd1a4d47ed721/src/progress.jl#L75-L76

@tkf
Copy link
Contributor

tkf commented Mar 9, 2020

I agree with all what @c42f said. But, for a short term solution, I don't think it's a horrible idea.

What if people want to send informational messages from DiffEq to a file?

One workaround is to use LoggingExtras.TeeLogger like @devmotion is doing:

https://github.com/TuringLang/AbstractMCMC.jl/blob/4bfe617eca199852251f6314627d7eba242e6294/src/AbstractMCMC.jl#L38-L48

@tkf
Copy link
Contributor

tkf commented Mar 9, 2020

BTW, if you want progress bars to be nicely shown in Windows consoles, https://github.com/tkf/ConsoleProgressMonitor.jl may be better. It just uses ProgressMeter.jl behind the scene. Though I consider ConsoleProgressMonitor.jl more or less dead as I only use TerminalLoggers.jl.

@c42f
Copy link

c42f commented Mar 9, 2020

It also forces all users of DiffEqBase to install Atom and Juno which seems far from ideal.

No, it definitely does not do that...

Ok. But I'm then confused that Juno seems to be included in the Project.toml here. Is it just the Juno backend which is relatively lightweight?

@ChrisRackauckas
Copy link
Member Author

Ok. But I'm then confused that Juno seems to be included in the Project.toml here. Is it just the Juno backend which is relatively lightweight?

Yeah, it's just some functions for interacting with Juno, but doesn't include Atom.jl or the big dependencies.

@tkf
Copy link
Contributor

tkf commented Mar 9, 2020

Actually, I think a better solution is to just check progress keyword argument and print some helpful message:

if get(NamedTuple(kwargs), :progress, false) === true &&
   Logging.min_enabled_level(current_logger()) > LogLevel(-1)
    @warn """
    `progress = true` is passed but your logger does not support progress bars.
    Please see the documentation for how to set it up:
    https://...
    """
end

@tkf
Copy link
Contributor

tkf commented Mar 9, 2020

Maybe we can add a function (say) ProgressLogging.enabled_for(logger) so that we don't need a hack like Logging.min_enabled_level(current_logger()) > LogLevel(-1).

@c42f
Copy link

c42f commented Mar 9, 2020

Actually, I think a better solution is to just check progress keyword argument and print some helpful message:

That would also work (add the maxlog=1 keyword to that @warn, I guess).

I think the long term direction for progress should be to remove the progress keyword from APIs like solve and have a neat generic way to configure progress filters in the logging backends.

Maybe we can add a function (say) ProgressLogging.enabled_for(logger) so that we don't need a hack like Logging.min_enabled_level(current_logger()) > LogLevel(-1).

This seems like a good thing to try out. I feel like it's roughly the job of shouldlog(logger, level, ...), but we don't have enough sophistication in the semantics of shouldlog to express this right now. So having a ProgressLogging.enabled_for could do the trick for now.

Btw, I was thinking something kind of related — that a filter for progress logs could sensibly live in ProgressLogging itself, keeping all the Progress related stuff more self-contained. This is somewhat the thing that ProgressLogging.asprogress does right now.

@tkf
Copy link
Contributor

tkf commented Mar 9, 2020

add the maxlog=1 keyword to that @warn

Good idea 👍

remove the progress keyword from APIs like solve

Agree in general, but for DiffEq API, I think it's reasonable to have progress argument. It is a common situation where you want every bit of instructions to be used for solving the problem and not wasting time for creating progress events.

Btw, I was thinking something kind of related — that a filter for progress logs could sensibly live in ProgressLogging itself, keeping all the Progress related stuff more self-contained. This is somewhat the thing that ProgressLogging.asprogress does right now.

Agree. Let's track that it in JuliaLogging/ProgressLogging.jl#9

@ChrisRackauckas
Copy link
Member Author

remove the progress keyword from APIs like solve

I would like this too: everything across Julia should have a common way to turn on/off all logging, like warnings, progress bars, etc. Having a different setup for every package, and not being able to turn it off in many, is just confusing and doesn't compose well.

But for now this seems like the best solution, so I'll merge but I'm glad there's an ongoing discussion to be had 😄

@ChrisRackauckas ChrisRackauckas merged commit 0d2741a into master Mar 9, 2020
@devmotion
Copy link
Member

I don't think this is a good solution.

Setting logger sinks should be controlled at the top application level and never in libraries. This allows libraries to compose together while giving the application control of the log stream. What if people want to send informational messages from DiffEq to a file? (Or from a user defined function DiffEq calls?) Overriding the global logger sink prevents this.

I completely agree, and so for quite some time I was arguing that we should not provide a default progress logger in AbstractMCMC but only display a warning if the current logger seems unable to handle progress logs (based on the min_enabled_level hack). However, I still think it is nice if displaying progress logs just works even without setting any specific progress logger (the main argument being that new users might expect such a functionality from a PPL) if it does not interfere with any custom loggers a user might have defined. Therefore the current approach in AbstractMCMC uses a LoggingExtras.TeeLogger and two LoggingExtras.EarlyFilteredLoggers to only forward progress logs (or more precisely, logs with log level -1) generated by the current module to the progress logger and forward all other logs to the current logger. The idea was that this allows us to get the best of both worlds - keep the composability of the logging system and provide a default progress logger if it was not set.

@pfitzseb
Copy link

pfitzseb commented Mar 9, 2020

For the record: JunoLab/Atom.jl#298 fixes the problems with Juno + TerminalLoggers.jl (or any other non-Base global logger).

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.

6 participants