progress bar integration

[pfitzseb]

ToDo:

• REPL integration.
• Potentially upstream a few ProgressLogging.jl changes (that aren't in this PR, actually): Ducktyping JuliaLogging/ProgressLogging.jl#34
• Support for multiple (and/or nested) progress bars.

[pfitzseb]

Alright, this should be in a fairly good spot now. Reviews would be appreciated :)

[davidanthoff]

Awesome!!

I think my main question is whether we can do this without shipping ProgressLogging.jl...

[c42f]

Suggested change

	module ProgessBase
	module ProgressBase

[c42f]

Very cool to see this functionality close to working!

The vendoring of most of ProgressLogging concerns me, somewhat. As I noted above, the intent of ProgressLogging.Progress was to be a fairly definitive type for representing progress (with asprogress as a backward compatibility hack).

I'm sure you want to avoid a ProgressLogging dependency for reason though, so I'd like to understand why that is.

The logging system needs some way to connect message semantics emitted by logging frontends like @progress to a semantically relevant action taken by the logging backend (displaying progress bars in the UI). Currently we were considering that this would be based on matching of message types, hence the ProgressLogging.Progress type. But it's also possible this is the wrong direction to take. Thoughts?

[davidanthoff]

We generally ship all the code that is needed for the VS Code extension in a vendored form, so that things a) always work on a user system without any additional installs, and b) we never run into problems that the version of some package that we need and that we ship might conflict with a different version of that package that the user might want to use in their project/environment. Essentially one of the requirements for us is that everything works, even if the user is using Julia 1.0 with package versions from that era.

So using specific types from packages for interfacing user code with our VS Code is very tricky in that model, because we can always run into version compatibilities when we do that.

So I think one way around this is the duck typing approach that @pfitzseb started here, which I actually kind of like :) Another would approach would be to put the type that we use for progress reporting into a package that is absolutely, 100% guaranteed to never ever get a 2.0 release (that is essentially how we handle the table integration with TableTraits.jl). I think that would only work if such a package is way, way more minimal than the current ProgressLogging.jl, it might really just have that one type defined in it or something like that. There are still some really tricky design questions with this approach, though, how we would ship that package in the extension then and make sure any other environment that uses ProgressLogging.jl also gets the version we ship etc. So I think generally I'm not too keen on this approach. The final idea I have of course would be to put this into Base or the stdlib :) I know folks want to not move stuff there, but this seems like such a fundamental part that it might be warranted, actually?

Also open to hear other ideas!

[c42f]

Right, the duck typing / pattern matching approach makes some sense:

• On the downside, we're replacing an explicit type and package version dependency with an implicit structural dependency. So when/if ProgressLogging.Progress is updated by the user it may break the julia-vscode logging backend. But assuming users always install the latest julia-vscode this can always be mitigated by updating julia-vscode to pattern match against multiple versions of ProgressLogging.Progress. So it's not too bad.
• On the upside, it removes any dependence between the user's installed package versions and the vscode implementation which seems extremely important.

Another would approach would be to put the type that we use for progress reporting into a package that is absolutely, 100% guaranteed to never ever get a 2.0 release

I don't think we've reached that level of stability in the logging backends. So I agree that this is not the way to go for now.

The final idea I have of course would be to put this into Base or the stdlib :) I know folks want to not move stuff there, but this seems like such a fundamental part that it might be warranted, actually?

I really want to do this. But I feel like this might be a strictly worse version of depending on a "ProgressBase" package because it gives us even less flexibility to improve the design and ties the release schedule to Base.

Anyway, thinking through your options I've come around to the idea that duck typing is the way to go for now. Even though it's quite ugly, it leaves a lot more flexibility to improve the system in the future.

@tkf do you have any thoughts on this?

[pfitzseb]

Would be cool if we could get consensus on whether we want to go with the duck typing approach.

On the downside, we're replacing an explicit type and package version dependency with an implicit structural dependency. So when/if ProgressLogging.Progress is updated by the user it may break the julia-vscode logging backend. But assuming users always install the latest julia-vscode this can always be mitigated by updating julia-vscode to pattern match against multiple versions of ProgressLogging.Progress. So it's not too bad.

That doesn't seem like a downside to me :) Or at least having a structural dependency doesn't. We could (should?) probably define a minimal set of required fields and pass all others along to the frontend.

[davidanthoff]

I've understood the discussion here as everyone agreeing that for now the duck typing approach is the way to go and we can merge this?

[c42f]

I've understood the discussion here as everyone agreeing that for now the duck typing approach is the way to go and we can merge this?

Agreed! It seems there's unfinished business here, but this is a nice flexible option for now.

That doesn't seem like a downside to me :) Or at least having a structural dependency doesn't.

To explain why I think structural dependency is a little strange:

• Julia has nominal types, not structural types
• Interfaces are defined in terms of generic functions rather than pattern matching

In a language with structural types and pervasive pattern matching, a structural dependency would make complete sense but in Julia I think it's a bit unusual. Having said that, logging has some particular needs which make structural typing more compelling:

• Log data has a stronger need for serialization and persistence than most data structures.
• We desire backward compatibility for serialized logs.

Perhaps this suggests we "should have" a stdlib Logging interface to map from Julia's nominal type system into structural types for use in log serialization. This make sense for, eg, JSON log serialization, but it's not clear this really helps all logging backends. For example TerminalLogger will probably still want show(io, MIME"text/plain"(), logged_value) to map into a textural form.

[tkf]

I'm sorry I'm late to the party. (@c42f thanks for the ping!) I need to catch up with the discussion and think about this. But my gut feeling is that some packages are not adequate for vendoring because, for example, you sometimes really need nominal typing or you need specific constants (like ProgressLogging._id_var). I don't think it's just ProgressLogging.jl. For example, to provide table UI, don't you need to actually load Tables.jl/TableTraits.jl? Shouldn't we be relying on SemVer to actually interop well with the user code?

[davidanthoff]

For example, to provide table UI, don't you need to actually load Tables.jl/TableTraits.jl?

We jump through some hoops to be able to integrate with tabular data sources while not loading any package ourselves and thus constraining users to specific versions of some package. You can see the code for that here. That works well because a) TableTraits.jl is super, super minimal (really just two functions) and b) I control it so I can guarantee that this will never ever break :). The idea is essentially that we let the user load the package, and when it is loaded our integration starts to kick in.

Shouldn't we be relying on SemVer to actually interop well with the user code?

I think in theory yes, but I think we would need to use it in a very different way than how it is typically used in the package manager right now. The core requirement we have for the extension is to always work with any packages and any package versions that a user might have. So we could never use the "I restrict things to compatible versions" aspect of SemVer from our extension side of things: we never want to restrict what versions a user can use.

I think it would be a different story if we had something like the existing pkgload structure that then was able to handle incompatible versions of packages, for example it could test whether TableTraits v1 or TableTraits v2 (which will never exist, I hope) was loaded and then make sure things work with either version.

So I guess that whole structure we use for TableTraits.jl and DataValues.jl right now could be extended to handle ProgressLogging.jl as well... But maybe we start with this PR as it is? I don't think we are locking ourselves into any design with this, and then we can still reconsider down the road if we run into problems?

[tkf]

It's not clear to me if this PR handles ProgressString and in particular nested progress bars. Supporting it with duck typing could be possible but I think it becomes trickier.

Isn't TableTraits-like solution easier to implement? I imagine it'd be something like

const progresslogging_pkgid = Base.PkgId(
    Base.UUID("33c8b6b6-d38a-422a-b730-caa89a2f386c"),
    "ProgressLogging"
)

"""
    try_process_progress(f, args...; kwargs...) -> nothing or Some(ans)

Try to process logging record by a function `f(::Progress) -> ans`.  Arguments
`args` and `kwargs` are the ones passed to ```Logging.handle_message`.  Return
`Some(ans)` if it is a progress record.
"""
function try_process_progress(f, args...; kwargs...)
    m = get(Base.loaded_modules, progresslogging_pkgid, nothing)
    m === nothing && return nothing
    # TODO: maybe check the version of ProgressLogging here.
    Base.invokelatest() do
        progress = m.asprogress(args...; kwargs...)
        progress === nothing && return nothing
        return Some(f(progress))
    end
end

and then use it as

try_process_progress(level, message, _module, group, id, file, line; kwargs...) do progress
    JSONRPC.send_notification(conn_endpoint[], "repl/updateProgress", progress)
end isa Some && return nothing

? Or maybe even

progress = try_process_progress(identity, level, message, _module, group, id, file, line; kwargs...)
if progress !== nothing
    JSONRPC.send_notification(conn_endpoint[], "repl/updateProgress", something(progress))
else
    ...
end

?

Is the latter (i.e., using progress::Progress outside invokelatest) OK as long as we don't define getproperty etc. in ProgressLogging.jl?

If the above snippet is enough, it sounds like much easier solution. Or are there some aspects that I'm missing?

[pfitzseb]

Fair point. I was originally hesitant to use that approach because it means the non-ProgressLogging way of doing things (constructing logging messages manually) doesn't work. On second thought that doesn't seem like too much of a loss though.