Add support for bokeh event callbacks #1148

philippjfr · 2017-02-24T04:20:43Z

Adds support for the upcoming event callbacks in bokeh, currently being developed here.

Summary of how it works

Check if any of the skip_conditions apply, if so, skip
Grab all the requested attributes from the models and accumulate them in data
Check if there is a Comm instantiated otherwise open it and store it in a global registry
Check if there is a comm_state for the Comm otherwise make one defining a event_buffer, a blocked flag and a time
Check whether the comm_state is blocked or has timed out (i.e. Date.now()>comm_state.time+timeout):

a) if it's blocked put data along with cb_obj.event.event_name on top of the event_buffer

b) if it's not blocked also prepend to event_buffer but also call process_event with small timeout. The small timeout acts as a debouncing mechanism as new events can be pushed ahead of the original event in the event queue. After process_events is called set the comm_state to blocked
process_events will iterate over the event_buffer and filter out older duplicated events. This ensures only the latest event of each type is processed (e.g. to ensure panstart, pan and panend are all processed). (Can probably be simplified)
In python everything does its thing and sends back a message either acknowledging execution or failure. This message also contains the comm_id, with which the comm_state is looked up. If there is something on the event_buffer process it and keep the Comm blocked, otherwise unblock.

ToDo:

Update docstring
Finalize naming for Callback attributes
Add tests to ensure callbacks reference correct handles/models
Add documentation about defining custom Stream/Callbacks

jlstevens · 2017-03-21T12:24:01Z

holoviews/plotting/bokeh/callbacks.py

@@ -92,19 +104,41 @@ class Callback(object):
    attributes = {}

    js_callback = """
+        function unique_events(events) {{


As far as I can tell this is a self-contained function that could be a utility. Would be good to have a docstring i.e a comment line saying that it does and the same for process_events below. At some point we will probably want the concept of Javascript 'utilities' that you can just include where needed.

That said, I just noticed comm_state is referenced in this function, can it not be made into an explicit function argument?

This is not ready to review. I've got outstanding changes locally.

Independent of the details of this function a set of JS utilities definitely makes sense. The issue then is that we will really want to have a proper JS package, because polluting the main namespace isn't ideal. All things to consider once we get started writing the widget manager.

Ok, the signature now looks fine and making JS utilities is for some other day.

jlstevens · 2017-03-21T12:27:17Z

holoviews/plotting/bokeh/callbacks.py

+        }}
+
+        function process_events(comm_state) {{
+            var events = unique_events(comm_state.events);


So this is at least one place where unique_events is used and comm_state is available. You could definitely pass comm_state to unique_events here though unique_events(comm_state.events, comm_state) is a bit redundant (just pass comm_state). The fact that you can access comm_state at all in unique_event without explicitly declaring it makes me uncomfortable (but then again, this is Javascript!)

As I said above, I would not review this yet.

Fair enough - I was just trying to understand the code out of my own curiosity and thought I might as well make some comments while I'm at it.

Looking is fine, but there's no point reviewing things I'm still actively working on.

jlstevens · 2017-03-21T12:28:45Z

holoviews/plotting/bokeh/element.py

+            'fixed', 'scale_width', 'scale_height',
+            'scale_both', 'stretch_both'
+        ], doc="""Defines how the plot scales when resized.""")
+


Is this relevant to hooking up to events?

Please stop reviewing this PR.

Why not just work on a branch if it is totally WIP? Once you make a PR you make the proposed changes public in a way that invites comment even if the status is WIP.

I would recommend working on a branch and only turning it into a PR when you require some sort of feedback.

I would recommend working on a branch and only turning it into a PR when you require some sort of feedback.

I disagree, I like being able to easily see an overview of my changes on Github and that's exactly what the WIP and ready tags are for.

But if it's really not ready for someone to look at, in addition to tagging it WIP you should put a sentence in the description saying what you're up to so that other people know what the status is. Lots of things marked WIP are ready to look at, just not ready to merge.

Or WIP and WIP-pre-review...

Then WIP allows for discussion. :-)

That said, I believe you can rename GitHub tags so all existing PRs and issues get renamed at once. Personally, I like Jim's suggestion more but I am happy to be overruled.

Also, I personally prefer that a WIP PR includes an explicit checklist in the description, stating what the author intends to do before it will be ready, and marking those off as they are done. That way it's fully ready for review about some things, and other things are clearly not ready for review. This PR doesn't have such a checklist, which is one reason it's hard for other people to know what to do with it.

By the way, GitHub supports PR templates which is where this information could go. I would keep it short as a long list of requirements/restrictions/demands would put people off contributing.

Asking for a checklist of items would be a very reasonable thing to do though.

I agree, I definitely think we should add a PR and Issue template.

philippjfr · 2017-03-23T00:42:42Z

Ready for review.

jlstevens · 2017-03-23T16:02:29Z

holoviews/plotting/bokeh/element.py

-        return bokeh.plotting.Figure(x_axis_type=x_axis_type,
-                                     y_axis_type=y_axis_type, title=title,
-                                     **properties)
+        with warnings.catch_warnings():


Probably worth mentioning what warning you are catching and why it doesn't matter that you are suppressing it (in a comment).

Sure, I'm personally a bit confused why the warning is raised here but not in other cases. May end up filing an issue with bokeh.

jlstevens · 2017-03-23T16:10:03Z

holoviews/plotting/bokeh/callbacks.py

-    * attributes :  The attributes define which attributes to send
+    * extra_models: Any additional models available in handles which
+                    should be made available in the namespace of the
+                    objects.


Giving an example of why you might want this would help here. E.g 'For instance, this can be used to check which tools are currently active'.

jlstevens · 2017-03-23T16:11:43Z

holoviews/plotting/bokeh/callbacks.py

+                    specified as a list of valid JS expressions, which
+                    can reference models requested by the callback,
+                    e.g. ['pan.attributes.active'] would skip the
+                    callback if the pan tool is active.


If the models referenced in the skip conditions need to be in extra_models it would be helpful to say so.

That's what this line was about:

which can reference models requested by the callback,

but I'll try to clarify.

jlstevens · 2017-03-23T16:14:17Z

holoviews/plotting/bokeh/callbacks.py

-
-        timeout = comm_state.timeout + {timeout};
+        // Add current event to queue and process queue if not blocked
+        event_name = cb_obj.event ? cb_obj.event.event_name : undefined


I think this line is expected to change - I've been requested to change how this works in Bokeh.

Okay, can fix that in a follow-up PR when you've made that change.

jlstevens · 2017-03-23T16:19:44Z

holoviews/plotting/bokeh/callbacks.py

        Stream.trigger(self.streams)
+        for stream in self.streams:
+            stream._metadata = None


I would consider just declaring _metadata to be an empty dict by default. I'm assuming this loop is to reset...

jlstevens · 2017-03-23T16:22:26Z

holoviews/streams.py

@@ -124,6 +129,8 @@ def __init__(self, preprocessors=[], source=None, subscribers=[], **params):
        self.subscribers = subscribers
        self.preprocessors = preprocessors
        self._hidden_subscribers = []
+        self._metadata = None


Should probably be an empty dict and I would add a comment what it is for - right now it is about stopping loops.

Agreed, definitely needs a comment.

jlstevens · 2017-03-23T16:25:58Z

holoviews/plotting/bokeh/callbacks.py

+        data = "var data = {};\n"
+        code = conditional + data + attributes + self.code + self_callback
+
+        js_callback = CustomJS(args=references, code=code)


I would consider factoring out a method that returns a CustomJS so all the Javascript is kept separate (probably including the call to attributes_js). The method could be called something like get_customjs...

jlstevens · 2017-03-23T16:26:33Z

holoviews/plotting/bokeh/callbacks.py

+
+        js_callback = CustomJS(args=references, code=code)
+        cb = None
+        if id(handle) in self._callbacks:


Use of id here probably warrants a comment about what it is for...

jlstevens · 2017-03-23T16:28:09Z

holoviews/plotting/bokeh/callbacks.py

@@ -358,14 +485,28 @@ def _process_msg(self, msg):
            return {}


+class PlotDimensionCallback(Callback):


I find the use of the name Dimension really confusing as we use it so much to mean an entirely different concept elsewhere in HoloViews. PlotSizeCallback or PlotWidthHeightCallback or maybe most explicitly PlotInnerSizeCallback?

jlstevens · 2017-03-23T16:30:43Z

holoviews/streams.py

+
+    Depending on the plotting backend certain streams may interactively
+    subscribe to events and changes by the plotting backend. To disable
+    this behavior instantiate the Stream with interactive=False.


I wish there were a better name for this concept by interactive is the best I can think of right now...

How about live?

Then we can pretend we are doing 'live streams' ;-p

Prefer interactive tbh.

@jbednar Any thoughts?

The concept here is you might want a PositionXY stream you want to manage yourself and you don't want the bokeh backend to try to supply you values. By default Bokeh supplies values but this option allows you to switch it off if you want (e.g you want to use streams in the same way between bokeh an matplotlib).

automatic? autoupdate? tied?

Sure, linked.

linked isn't bad

jlstevens · 2017-03-23T16:41:17Z

holoviews/plotting/bokeh/callbacks.py

@@ -215,13 +279,18 @@ def _filter_msg(self, msg, ids):

    def on_msg(self, msg):
        for stream in self.streams:
-            ids = self.handle_ids[stream]
+            metadata = self.handle_ids[stream]


I think I would just call this handle_ids here....

jlstevens · 2017-03-23T16:41:43Z

holoviews/plotting/bokeh/callbacks.py

            filtered_msg = self._filter_msg(msg, ids)
            processed_msg = self._process_msg(filtered_msg)
            if not processed_msg:
                continue
            stream.update(trigger=False, **processed_msg)
+            stream._metadata = {h: {'id': hid, 'events': self.events}
+                                for h, hid in metadata.items()}


... and use ... for h, hid in handle_ids.items() here.

Also, if you expect to use _metadata for anything else, then you'll need a key e.g 'prevent_loop_ids' (or similar).

jlstevens · 2017-03-23T20:52:37Z

There are a few minor outstanding things - just tell me when you think it is ready to merge and I'll merge (as long as the tests are passing).

philippjfr · 2017-03-23T20:53:45Z

I've addressed all your comments except for the change you are still going to make in bokeh. It's just that you commented on the line above in some cases.

jlstevens · 2017-03-23T20:54:27Z

Ok, merging.

github-actions · 2024-10-26T01:45:02Z

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.

philippjfr added the status: WIP label Feb 24, 2017

philippjfr force-pushed the bokeh_event_callbacks branch from a5961ed to 8aa4997 Compare February 24, 2017 04:27

philippjfr added tag: backend: bokeh type: feature A major new feature labels Feb 24, 2017

philippjfr force-pushed the bokeh_event_callbacks branch 5 times, most recently from e97ec94 to bfdec8a Compare March 20, 2017 19:04

jlstevens reviewed Mar 21, 2017

View reviewed changes

philippjfr force-pushed the bokeh_event_callbacks branch from 47ee18c to c850de3 Compare March 21, 2017 16:14

philippjfr removed the status: WIP label Mar 23, 2017

jlstevens reviewed Mar 23, 2017

View reviewed changes

philippjfr added 3 commits March 23, 2017 16:50

Add support for bokeh event callbacks

fe95442

Handle bokeh attribute change events in Callbacks

4df2923

Fixes for deduplicating and merging tools

69f6501

philippjfr added 14 commits March 23, 2017 16:50

Fixes for merging of tools and referencing tools on callbacks

e9eb973

Made bokeh callbacks more general

092f7f0

Debounce multiple events separately

d95636b

Callbacks supply stream metadata

9e504ca

Improved handling of plot range on stream events

5448112

Cleanup of bokeh Callback JS

dd0718f

Updated and added streams and callbacks

cfc227f

Added interactive attribute to Stream

9647b0f

Small fixes for callback and tool handling

2d8a722

Renamed Callback attributes and updated docstring

e9c5062

Added stub for callbacks support on TablePlot

09ce4b3

Improved naming in bokeh Callback JS code

dfb3746

Skip bokeh streams tests for version below 0.12.5

40cbea0

Cleanup and refactoring of Streams and Callbacks

5d54235

philippjfr force-pushed the bokeh_event_callbacks branch from 5087067 to 5d54235 Compare March 23, 2017 17:39

Renamed Stream.interactive to linked

522952a

philippjfr force-pushed the bokeh_event_callbacks branch from 95a254f to 522952a Compare March 23, 2017 19:49

jlstevens merged commit 07ab51e into master Mar 23, 2017

jbednar deleted the bokeh_event_callbacks branch March 23, 2017 22:44

github-actions bot locked as resolved and limited conversation to collaborators Oct 26, 2024

		@@ -358,14 +485,28 @@ def _process_msg(self, msg):
		return {}


		class PlotDimensionCallback(Callback):

Add support for bokeh event callbacks #1148

Add support for bokeh event callbacks #1148

Conversation

philippjfr commented Feb 24, 2017 • edited Loading

Summary of how it works

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbednar Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philippjfr commented Mar 23, 2017

jlstevens Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlstevens Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlstevens Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

jlstevens Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlstevens commented Mar 23, 2017

philippjfr commented Mar 23, 2017

jlstevens commented Mar 23, 2017

github-actions bot commented Oct 26, 2024

philippjfr commented Feb 24, 2017 •

edited

Loading

jbednar Mar 21, 2017 •

edited

Loading

jlstevens Mar 23, 2017 •

edited

Loading

jlstevens Mar 23, 2017 •

edited

Loading

jlstevens Mar 23, 2017 •

edited

Loading

jlstevens Mar 23, 2017 •

edited

Loading