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

Merging Turkie with TuringCallbacks #16

Open
theogf opened this issue Nov 27, 2020 · 4 comments
Open

Merging Turkie with TuringCallbacks #16

theogf opened this issue Nov 27, 2020 · 4 comments

Comments

@theogf
Copy link
Member

theogf commented Nov 27, 2020

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
Copy link
Member

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:

for (ksym, val) in zip(Turing.Inference._params_to_array([transition])...)
and
names, vals = Turing.Inference.get_transition_extras([transition])
. 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
Copy link
Member Author

theogf commented Dec 28, 2020

Regarding

for (ksym, val) in zip(Turing.Inference._params_to_array([transition])...)

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
Copy link
Member

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
Copy link
Member Author

theogf commented Feb 7, 2021

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.

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

No branches or pull requests

2 participants