New progress logging interface?

[tkf]

Edit: Originally it was about using gensym for _id. But it turned out there are many aspects that may require re-design.

---

IMHO using gemsym dynamically is not really the best practice as it leaks memory. It also is slow compared to (say) UUID

julia> @btime uuid4();
  11.958 ns (0 allocations: 0 bytes)

julia> @btime gensym();
  1.082 μs (0 allocations: 0 bytes)

These are not very critical problems but it's nice if we can avoid it. I think we can easily avoid it by introducing a new key value (say) progressid such that

@logmsg ProgressLevel name progress=0.1 progressid=id

is used instead of

@logmsg ProgressLevel name progress=0.1 _id=id

Then id can be of any type. The easiest thing to use may be uuid4().

[pfitzseb]

Agreed.
We should still specify that the Logger falls back to _id if progressid isn't used, so that

for i in 0:0.1:1
   sleep(0.5)
   @logmsg -1 "foo" progress=i
end

continues to work.

[tkf]

Thinking about this and #8, I started wondering if it makes sense to use a single key like this

@logmsg ProgressLevel name progress=Progress(0.1, id, parentid)

where

Base.@kwdef struct Progress
    progress::Float64
    id::UUID
    parentid::UUID = UUID(0)  # not nested by default
end

This has some benefits:

• The top level keys are not cluttered.
• It is easy to construct progress events programmatically.
• Adding new API or extending the old one is easier (e.g., iterators with unknown length support unknown length iterators in progress API JunoLab/Juno.jl#222)
• Changes in API is easy to propagate.
  • Backends (log handlers) using _id-based old specification are likely to check if progress is a Real or "done". They would just ignore the new Progress type in this case.
  • For backward-compatible changes, we can just append optional arguments to Progress constructor.
  • For backward-incompatible changes (after transitioned to Progress), we can introduce a new struct (or bump ProgressLogging.jl's major version number if we absolutely have to).

Maybe it makes sense to put above struct in ProgressBase.jl, since this is what both frontend and backend need while backends do not need @progress macro etc. It can be done anytime later, though.

[pfitzseb]

I like it.
Would still be nice to have a way to use the @logmsg API without worrying about ids at all, so we maybe should add a macro-constructor so you can do

for i in 0:0.1:1
   sleep(0.5)
   @logmsg -1 "foo" progress=@Progress(0.1) # This sets id to something based on __source__.
end

IMHO we should also keep all other kwargs supplied to Progress somewhere, so log handlers can have specialized fields (Juno for example supports right_text and message).

[tkf]

Instead of @Progress, maybe we can use @logprogress? ATM we have

@withprogress begin
    for i = 1:10
        sleep(0.5)
        @logprogress "iterating" progress=i/10
    end
end

I think @logprogress "iterating" progress=0.1 can be easily lowered to @logmsg ProgressLevel "iterating" progress=Progress(0.1, id).

Thinking this a bit more, I wonder if we should only expose a smaller @logprogress API like this

@logprogress progress [message=value] [right_text=value]

which would be used as

@withprogress name="iterating" begin
    for i = 1:10
        sleep(0.5)
        if i == 10
            @logprogress i/10 message="this is the last one"
        else
            @logprogress i/10
        end
    end
end

(where name is passed to @logmsg inside the for loop body via a gensym'ed local variable)

This way, we can be more flexible about the upgrade path to Progress struct.

[tkf]

IMHO we should also keep all other kwargs supplied to Progress somewhere, so log handlers can have specialized fields (Juno for example supports right_text and message).

Since @logmsg already allows arbitrary keyword arguments, I wonder if it is enough to use them for the extension to the core progress API.

[pfitzseb]

Yeah, @logprogress seems like the better API.

Since @logmsg already allows arbitrary keyword arguments, I wonder if it is enough to use them for the extension to the core progress API.

Mh, fair enough, I suppose.

[tkf]

Do you think @logprogress progress [key1=val1 [key2=val2 ...]] is fine? i.e., no name argument and no progress=. Do you have usecases to change the progress bar name? Is it supported by Juno?

[pfitzseb]

Currently,

for i in 1:100
  @logmsg -1 "$(i)/100" progress=i/100 message="fooing: at element $i"
  sleep(1)
end

produces

Imho it's very useful to be able to change the progress bar's name.

I'd also kinda like it if the @logmsg API would still be supported even if we go with @logprogress, mostly because it doesn't make this package mandatory for everyone trying to display progress bars.

[tkf]

Thanks for the demo. I see. It makes sense to support changing the name. So, how about this syntax?

@logprogress [name] progress [key1=val1 [key2=val2 ...]]

i.e., make name optional and progress positional.

I'd also kinda like it if the @logmsg API would still be supported

I think I understand the benefit but I'm a bit ambivalent about this direction. But we are going to have a phase supporting both specs anyway if we are to switch to Progress type-based API. I think it would then be reasonable to provide utility functions for logger monitor packages to "merge" those two APIs. If this compatibility layer is simple enough to maintain (it probably is), I think I'd be happy with this choice.

[c42f]

I agree that dynamic gensym() should be avoided. I do think a macro-based version of this would make sense with a statically generated id.

Hmm, concurrent tasks would get the same id when running a given loop which is annoying. That could be dealt with by adding a task id and using (_id,task_id) to update progress bars. Or generating a UUID I guess.

The _id doesn't need to be a symbol, by the way. It could be a UUID in principle.

[tkf]

Hmm, concurrent tasks would get the same id when running a given loop which is annoying.

I had the same thought. But actually you only need a recursive function to break macro-based _id:

foreachprod(f) = f(())
function foreachprod(f, it, itrs...)
    for (i, x) in enumerate(it)
        @info "foreachprod" progress = i / length(it)
        foreachprod(itrs...) do y
            f((x, y...))
        end
    end
end

This function requires nested bars but produces only one _id.

Another example from https://tkf.github.io/Transducers.jl/dev/manual/#Transducers.withprogress:

xf = MapCat() do x
    withprogress(1:x; interval=1e-3)  # nested progress
end |> Map() do x
    sleep(0.5)
    x
end

foldl(+, xf, withprogress(1:10; interval=1e-3))

The _id doesn't need to be a symbol, by the way.

Logging.handle_message says

id is an arbitrary unique Symbol to be used as a key to identify the log statement when filtering.

So I've been assuming that it has to be a symbol. Maybe we should fix the doc, if this is not indented?

[c42f]

But actually you only need a recursive function to break macro-based _id:

Oh! You're completely right about that.

Maybe we should fix the doc, if this is not indented

Yes we should; this must be a leftover from earlier versions. As of julia-1.0, depwarns use an _id which definitely is not a Symbol. It's unfortunate though: it would be preferable if we could use a concrete type of some kind or other. In hindsight a UUID may have been a better choice and I think we could still go in that direction for the default generated values of _id as it's currently basically an Any for logging backends.

[tkf]

Quoting @c42f's comment

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)

--- JuliaLogging/TerminalLoggers.jl#4 (comment)