-
-
Notifications
You must be signed in to change notification settings - Fork 404
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
Bokeh server support #959
Bokeh server support #959
Conversation
6cba793
to
2d80908
Compare
2d80908
to
c3151cb
Compare
Nice to have the examples! That said, I feel they belong in |
I do like the idea of contrib, but until it's been integrated into our doc building process and referenced throughout our documentation it just seems like no one will ever find it. Edit: I do think it's the right place for these examples. |
Given that we want to split out the bokeh backend eventually and this is definitely bokeh related, it should be in |
I did think |
I think it would be nice to keep all the examples together in For this reason, I think we should move these examples to |
0cd9548
to
16a6338
Compare
@jlstevens, @jbednar I've rebased this PR with the latest events PR and it's working as I want it to. The question is now about scope, I'll happily move the current examples to contrib, but my main question is about bokeh widgets. As of right now this PR ships with a small class that emulates our regular Dimension based widgets, and you can easily connect streams to other widgets and use the regular bokeh API to compose your new set of widgets into a layout with the plot(s). Should we ship a basic widget class which generates widgets for HoloMap/DynamicMap key dimensions? You can see what that looks like in the gif example above. The implementation is fairly straightforward (~130 LOC) and means you can directly deploy standard HoloMaps/DynamicMaps you used and defined in a notebook without manually having to hook up widgets for key dimensions. |
That all sounds good, but is there a reason not to just use Bokeh widgets all the time when we are using the Bokeh backend, even in the notebook? That seems like the way to have seamless interoperability with bokeh in its various possible usages. |
If we had already done the widget refactor and implemented a general widget/comm manager as we are planning to do I'd say yes. However for the time being I'd have to come up with some ad hoc solution which I'm strongly against. So my answer is that once we have that we can easily retrofit this widget implementation to support it. In the long run we should find an easy and general way to map both dimensions and streams to specific widgets. |
Waiting on bokeh/bokeh#6062 so that the I've also implemented an event queue on the python end which works fairly well. Once the PR has been merged I'll have to extend that implementation to handle different event types as I did on the JS end. |
8ad43db
to
0f4e00f
Compare
@jlstevens This is now ready for review. All streams now work for both bokeh server and in the notebook. I'm happy to move the example apps elsewhere though. |
holoviews/plotting/bokeh/widgets.py
Outdated
if self.plot.renderer.mode == 'default': | ||
self.attach_callbacks() | ||
self.state = self.init_layout() | ||
self._event_queue = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would still rename this from _event_queue
to just queue
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this is for the widgets now.. so it might be ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although maybe better for consistency? I'll rename.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in widgets now so my original comment was addressed. Might be worth renaming here too - up to you.
def _init_plot_handles(self): | ||
""" | ||
Find all requested plotting handles and cache them along | ||
with the IDs of the models callbacks will be attached to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either an an extra word here or words missing. Either '... along with the IDs the model callbacks will be attached to' or '...along with the IDs of the model callbacks that they will be attached to'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neither, it's missing a the
:
along with the IDs of the models the callbacks will be attached to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works too. I knew something was wrong!
def process_on_change(self): | ||
if not self._queue: | ||
return | ||
self._queue = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come the _queue
isn't used for process_on_change
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because process_on_change just looks up the model values directly, that avoids having separate callbacks for each model change, which would be required otherwise.
the data sent by the callback before it is passed to the streams. | ||
""" | ||
|
||
def initialize(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better! :-)
holoviews/plotting/bokeh/renderer.py
Outdated
|
||
mode = param.ObjectSelector(default='default', | ||
objects=['default', 'server'], doc=""" | ||
Whether to render the DynamicMap in regular or server mode. """) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it only rendering DynamicMaps? Might want a bit more to say what server mode is about ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, not sure why I wrote that. It handles anything.
|
||
|
||
@classmethod | ||
def create_widget(self, dim, holomap=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much nicer as a classmethod! Good for testing...
Fantastic! Other than one docstring fix I am now very happy with this PR. The functionality is great and the code is now structured in a much more sensible way. I'll merge once that is done. |
All done! Merging. |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Adds support for deploying any holoviews object as a bokeh server app.
To do:
cb_data
andcb_obj
work correctly.Optional/Future:
Example app: