mila-iqia / blocks-extras

A collection of extensions to the Blocks framework
MIT License
27 stars 40 forks source link

Status of Plot mega-ticket #22

Closed dwf closed 8 years ago

dwf commented 9 years ago

Looking into adding an extension for plotting filters and/or samples from image models I've noticed that things have gotten complicated with the Plot extension. At least two pull requests with potentially useful changes got tossed on the floor (I believe that was a mistake, even if the code was to be eventually moved here) and have since rotted ( mila-udem/blocks#447, mila-udem/blocks#569 ). Since half of our open issues on this repo concern Plot, maybe we should try and organize efforts to finally bring it up to working order.

fvisin commented 9 years ago

I don't remember much by now, but if I am not wrong mila-udem/blocks#569 should partially work in case the training is resumed from a checkpoint (i.e., it shouldn't preventing from resuming the training): it should open a new Plot filling it with the new data, but you would loose the data you had in the previous Plot. I am not sure though, it should be tested.

If I remember correctly, the Session was a reference to a single Plot object on a server (somewhat in the middle between a socket and a Plot object) and many inner behaviors of Bokeh relied on a "default session" object. This was the origin of many problems because whenever I created a new Plot it would create a new session, which would become the "default" one, making it impossible to write on the previous one(s).

My 2 cents: working with Bokeh has been a terrible experience. I hope they improved the documentation and changed the way session is handled, because when I used it last time it was really hard to understand what was going on behind the curtains and some of the inner elements of the framework were simply not accessible and not documented. On top of that, half of the documentation is built around some examples, which makes it very hard to find the information you are looking for. If they didn't improve these points, I suggest you don't waste your time on it and drop the support to Bokeh until it's usable.

This is all I remember, a more accurate description of the specific problem I had and of the solution we came up with can be found in a discussion on their forum: https://groups.google.com/a/continuum.io/forum/#!searchin/bokeh/fvisin/bokeh/snm3qxAIomI/YKloSfBsSTAJ

dwf commented 9 years ago

Okay, useful. To summarize, sounds like the plotting interface for Bokeh is stateful (the session, the document, etc.), and manipulating that state correctly was an issue?

fvisin commented 9 years ago

Yes, exactly. At least, this is the best I can get from my foggy memory!

rizar commented 9 years ago

Honestly, I know nothing about Bokeh at all and don't know if I can help. One thing I know for sure is that with Bokeh 0.9.1 our plotting does not work.

fvisin commented 9 years ago

Bokeh's changelog is not very informative on that behalf. After a quick overview I can tell that they improved the documentation, but the usage of Server and Session is still not clear. My guess/hope is that they are refactoring that part (or plan to), so the documentation is still partial.

I confirm my previous opinion: as far as the server interface and the plotting interface are not completely separated, the server connection is stateful and their interaction is not well documented, I wouldn't spend time on supporting Bokeh.

dwf commented 9 years ago

I haven't been able to get even a toy example working with 0.9.3 which works perfectly with 0.8.0, so I've filed a ticket at bokeh/bokeh#2787.

dwf commented 9 years ago

On the plus side I think I have kind of a handle on what is going on with respect to the stateful plotting backend so once this is fixed on Bokeh's end I think we can probably clean house quite a bit.

fvisin commented 9 years ago

Apparently you found the bug in Bokeh, good job! It would be nice to have it integrated in Blocks, but last time I checked it was really badly engineered and difficult to understand. Do you think it's good enough now to keep the support in Blocks?

dwf commented 9 years ago

So my fix for that bug (bokeh/bokeh#2792) as well as another fix for a bizarre palette bug (bokeh/bokeh#2804) have both been merged, and the latest devel release.

I wouldn't call Bokeh badly engineered, in fact the JavaScript library is quite impressive. The precise way that the Python library interacts with the server when you're using the module-level state machine is a bit annoying but you can use it in a fully explicit manner without the module-level global side effects. It took some fiddling to figure out:

rizar commented 9 years ago

Awesome that you fixed the bug, thanks!

I think we should consider not using module-level machinery, because it is hard to read and hard to debug.

dwf commented 9 years ago

I wholeheartedly agree. It also becomes all but untenable the moment we have more than one extension interacting with Bokeh (which we soon will). The only sane option is explicit document management.

I will send a PR today converting Plot to use this approach.

fvisin commented 9 years ago

Great job! Thank you for reporting how to use the Session/Document in an explicit way. They refactored that part quite a lot, as when I worked on that it was not possible to decouple Session and Document and I don't know how many things I tried to find ways around that! Accessing Session and Document in the explicit way seems reasonably simple.

Ciao Francesco

On Mon, Sep 7, 2015 at 1:30 PM, David Warde-Farley <notifications@github.com

wrote:

I wholeheartedly agree. It also becomes all but untenable the moment we have more than one extension interacting with Bokeh (which we soon will). The only sane option is explicit document management.

I will send a PR today converting Plot to use this approach.

— Reply to this email directly or view it on GitHub https://github.com/mila-udem/blocks-extras/issues/22#issuecomment-138346266 .