Improve progress bar (ETA and Unicode-based rendering) #4
Conversation
| export TerminalLogger | ||
|
|
||
| const ProgressLevel = LogLevel(-1) |
There was a problem hiding this comment.
This is the level that has been used for a while in Juno https://junolab.org/ProgressLogging.jl/dev/#ProgressLogging.progress-Tuple{Any}
There was a problem hiding this comment.
I've just been thinking more about this and about the best way represent progress.
I think it does make sense to have a default urgency associated with a @progress frontend, but I also think there's sense in having progress messages at Debug level, or progress messages at Info level. So I think progress reporting is not inherently tied to the level.
Progress is currently detected via the _id and progress key. But perhaps progress "should" be determined by the type of the message?
@info Progress("msg", fraction)I think you suggested something similar elsewhere?
There was a problem hiding this comment.
Rather than type of message, why not type of value?
@info "msg" fraction=Progress(fraction)
TensorBoardLogger is all abut the type of the value,
since it logs them differently
There was a problem hiding this comment.
But perhaps progress "should" be determined by the type of the message?
@info Progress("msg", fraction)
This is an interesting approach. It's nice that all the information for "dispatch" is available as the mandatory positional argument. We can also avoid awkward empty message for logging progress.
Rather than type of message, why not type of value?
@info "msg" fraction=Progress(fraction)
That's more or less what I suggested here JuliaLogging/ProgressLogging.jl#7 (comment)
But I think it's important to support the spec that already exists. Apparently this has been used by Juno users for a while.
For the new progress logging API, let's discuss it in https://github.com/JunoLab/ProgressLogging.jl. At some point it would be nice to have similar package for documenting table logging specification and also providing some utility functions.
There was a problem hiding this comment.
But I think it's important to support the spec that already exists.
Agreed completely. This PR just sparked the thought, so I wrote it down!
There was a problem hiding this comment.
Thanks for writing it down! I just quoted your comment in JuliaLogging/ProgressLogging.jl#7 (comment)
Codecov Report
@@ Coverage Diff @@
## master #4 +/- ##
==========================================
- Coverage 97.69% 96.37% -1.32%
==========================================
Files 3 4 +1
Lines 130 193 +63
==========================================
+ Hits 127 186 +59
- Misses 3 7 +4
Continue to review full report at Codecov.
|
|
The progress meter code is simple enough there is no reason really to use ProgressMetersfor it. I vendored it before in https://github.com/oxinabox/OhMyLog.jl/blob/master/src/progress.jl Aside: |
| @@ -225,4 +225,10 @@ import TerminalLoggers.default_metafmt | |||
| \e[36m\e[1m│ \e[22m\e[39mline2 | |||
| \e[36m\e[1m└ \e[22m\e[39m\e[90mSUFFIX\e[39m | |||
| """ | |||
|
|
|||
| # Using infix operator so that `@test` prints lhs and rhs when failed: | |||
There was a problem hiding this comment.
Ha sure! Test really needs some work to fix this kind of thing :-/
|
Nice, this is pretty cool. I kind of agree that it would make sense to copy some code snippets to avoid relying on the ProgressMeter internals. Also because terminal layout is very non-composable and I think we're likely to want to modify the printing / layout code. |
tkf
changed the title
|
ProgressMeter.jl is vendored now. I think it's good to go? |
|
Thank you guys for reviewing this! @c42f How about registering it soon-ish? I find TerminalLoggers.j pretty useful. |
|
For sure. I reckon we register as soon as we've got some basic docs working. |
This PR implements ETA for progress bars. close #3.
It is a slightly hacky solution to use ProgressMeter.jl for generating a "smoother" progress bar and also let it compute and format ETA. At some point, we may want to vendor ProgressMeter.jl or write our own functions to do this. But I think this is a good start for now.