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

Do respect existing loggers #454

Merged
merged 3 commits into from
Mar 10, 2020
Merged

Conversation

devmotion
Copy link
Member

This PR addresses some of the issues raised by @c42f in #450 (comment). It changes the default logger to a TeeLogger such that existing user-defined loggers are respected and the progress logger only handles progress logs. Additionally, instead of checking if Juno is active, it just checks if the current logger can handle logs of log level -1 (which is the case for the JunoProgressLogger).

Moreover, for now I set ConsoleProgressMonitor as the default logger on Windows and for Jupyter notebooks (see @tkf's comment #450 (comment) and the discussion in JuliaLogging/TerminalLoggers.jl#25).

src/utils.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
Copy link
Contributor

@tkf tkf left a comment

Choose a reason for hiding this comment

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

LGTM

@c42f
Copy link

c42f commented Mar 13, 2020

Thanks! So what do we need to do upstream to be able to remove these workarounds eventually?

It seems there's actually quite a few pieces to this puzzle:

  • Get the ProgressLogging.Progress type into Logging once we're satisfied that the design will last.
  • Replace Logging.ConsoleLogger with something more like TerminalLogger which knows how to handle Progress
  • Fix TerminalLogger so that the sticky messages are less terrible on windows (should be doable in the near term I hope Make sticky messages work better on windows JuliaLogging/TerminalLoggers.jl#28)

Backward compat is going to be hard though until Logging itself can be upgraded separately from Base.

Doing all of the above only solves an instance of the general problem though, which is that we might want loggers to ignore certain types of log records by default if they can't process them "sensibly". This feels like a problem of generalizing shouldlog semantics, but there are many options for exactly how to do this. It could be be done based on message type, level, or @tkf has even suggested id...

@ChrisRackauckas
Copy link
Member

And TerminalLogger should include the message information.

@tkf
Copy link
Contributor

tkf commented Mar 13, 2020

@ChrisRackauckas Do you mean the tooltip in Juno?

@ChrisRackauckas
Copy link
Member

Yeah, it should be printing things like dt, umax, etc. It looks like @devmotion got AdvancedMC to do it, but I'm not sure if that's using the same progress bar?

@tkf
Copy link
Contributor

tkf commented Mar 13, 2020

I guess the only cross-platform way to do this, for now, is to put it as a part of the progress bar name. We are tracking the issue at JuliaLogging/ProgressLogging.jl#23

@tkf
Copy link
Contributor

tkf commented Mar 13, 2020

@c42f

It could be be done based on message type, level,

We can't use the message type (e.g., Progress) in shouldlog, right?

or @tkf has even suggested id...

Well, that's just a part of brainstorming :)

I now think doing this with level type is the way to go, provided that we can have something like supportedby(level::LevelType, logger): JuliaLang/julia#33960 (comment)

@c42f
Copy link

c42f commented Mar 13, 2020

We can't use the message type (e.g., Progress) in shouldlog, right?

Ah, you're right of course :-/ The early filtering is necessary for high performance (eventually...) but man it's turning out to be a design annoyance.

@devmotion
Copy link
Member Author

It looks like @devmotion got AdvancedMC to do it, but I'm not sure if that's using the same progress bar?

Unfortunately not, that's why I opened JuliaLogging/ProgressLogging.jl#23.

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.

4 participants