TuringLang / TuringCallbacks.jl

http://turinglang.org/TuringCallbacks.jl/
MIT License
13 stars 8 forks source link

Merging Turkie with TuringCallbacks #16

Open theogf opened 3 years ago

theogf commented 3 years ago

Ok time to get this done! As we talked about, it would be nice to merge Turkie with TuringCallbacks. The question is how? Regardless of the UI, which I think can be unified, the problem is with dependencies:

Turkie relies on AbstractPlotting.jl which is pretty heavy, similarly TuringCB relies on TensorboardLogger, and therefore tensorboard which is also heavy in a way. Easiest solution would be to use Requires, but it has its issues too. Maybe it would be nice to have some kind of Plots.jl approach where the user can decide what he wants with a function turkie() or tensorboard() or something like this.

Also I saw you have a dependency on Turing, if just rely on AbstractMCMC / DynamicPPL, this dependency is not necessary and the package could be used for other PPL no?

torfjelde commented 3 years ago

The question is how?

This depends on how we want to do things. One approach is to simply implemented a callable struct a la TensorBoardCallback https://github.com/TuringLang/TuringCallbacks.jl/blob/master/src/callbacks/tensorboard.jl#L129. Then we can at least share implementations of different statistic implementations, e.g. https://github.com/TuringLang/TuringCallbacks.jl/blob/master/src/stats.jl.

Ideally we'd have a shared main function to call, but it's a bit unclear to me how we'd do that at the moment. Therefore I think what I mention above is the best way to get started, and then we can figure out how to share more code between the methods later.

Turkie relies on AbstractPlotting.jl which is pretty heavy, similarly TuringCB relies on TensorboardLogger, and therefore tensorboard which is also heavy in a way.

TensorBoardLogger doesn't require tensorboard to be installed to use it. tensorboard is only needed if you want to visualize the outputs. This means that you can for example run code using TuringCallbacks.jl on a server which does not have tensorboard while still producing logs which you can look at later.

I agree though, AbstractPlotting.jl is quite a dependency if you for example only want to use the tensorboard logging.

How does the Plots.jl approach actually work? It also seems like Requires.jl would be a reasonable choice in this case since the main code will fairly light-weight and so the compile-times shouldn't be very long.

Also I saw you have a dependency on Turing, if just rely on AbstractMCMC / DynamicPPL, this dependency is not necessary and the package could be used for other PPL no?

We currently depend on Turing.jl for the following methods: https://github.com/TuringLang/TuringCallbacks.jl/blob/1a4bbc396b295833c6dc4eaa285f5c6d79b35a13/src/callbacks/tensorboard.jl#L110 and https://github.com/TuringLang/TuringCallbacks.jl/blob/1a4bbc396b295833c6dc4eaa285f5c6d79b35a13/src/callbacks/tensorboard.jl#L129. So the dependency is deliberate (this method is not available in DynamicPPL.jl or AbstractMCMC.jl).

But I agree that ideally we wouldn't need this method, so open to suggestions on how to improve this!

theogf commented 3 years ago

Regarding https://github.com/TuringLang/TuringCallbacks.jl/blob/1a4bbc396b295833c6dc4eaa285f5c6d79b35a13/src/callbacks/tensorboard.jl#L110

I don't know if it works but I got this working in Turkie :

https://github.com/theogf/Turkie.jl/blob/3990deb6f18198648cf34a595c975d2512ed792d/src/Turkie.jl#L93-L99

For the transitions extras maybe we could require the user to load Turing for it?

torfjelde commented 3 years ago

The issue with that approach is that all AbstractTransition will not have the θ field, which is why the _params_to_array is used in the current impl. We could indeed replace _params_to_array somehow, but it will still rely on tonamedtuple which requires Turing.jl (or one of the other packages under the Turing-umbrella; I forget which package the "base-version" of tonamedtuple(::AbstractTransition) is implemented in).

theogf commented 3 years ago

Hey so I was having a second look at the merging and it looks more and more that that merging Turkie to TuringCallbacks is not viable. As we talked about AbstractPlotting.jl is too heavy of a dependency to have by default. But the problem by using Requires.jl is that there is no version control, and this is a large issue with the ever changing API of Makie.

What I would propose is to go down the road of having an AbstractSamplingCallbacks package containing all the necessary methods from which TuringCallbacks and Turkie would both be depending on.