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

Add support for bokeh event callbacks #1148

Merged
merged 18 commits into from Mar 23, 2017

Conversation

Projects
None yet
3 participants
@philippjfr
Copy link
Contributor

philippjfr commented Feb 24, 2017

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

Summary of how it works

  1. Check if any of the skip_conditions apply, if so, skip

  2. Grab all the requested attributes from the models and accumulate them in data

  3. Check if there is a Comm instantiated otherwise open it and store it in a global registry

  4. Check if there is a comm_state for the Comm otherwise make one defining a event_buffer, a blocked flag and a time

  5. 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

  6. 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)

  7. 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

@philippjfr philippjfr added the WIP label Feb 24, 2017

@philippjfr philippjfr force-pushed the bokeh_event_callbacks branch from a5961ed to 8aa4997 Feb 24, 2017

@philippjfr philippjfr force-pushed the bokeh_event_callbacks branch 5 times, most recently from e97ec94 to bfdec8a Mar 18, 2017

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

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

This comment has been minimized.

@jlstevens

jlstevens Mar 21, 2017

Contributor

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 comment has been minimized.

@philippjfr

philippjfr Mar 21, 2017

Author Contributor

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

This comment has been minimized.

@philippjfr

philippjfr Mar 21, 2017

Author Contributor

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.

This comment has been minimized.

@jlstevens

jlstevens Mar 21, 2017

Contributor

Agreed.

This comment has been minimized.

@jlstevens

jlstevens Mar 23, 2017

Contributor

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

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

This comment has been minimized.

@jlstevens

jlstevens Mar 21, 2017

Contributor

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!)

This comment has been minimized.

@philippjfr

philippjfr Mar 21, 2017

Author Contributor

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

This comment has been minimized.

@jlstevens

jlstevens Mar 21, 2017

Contributor

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.

This comment has been minimized.

@philippjfr

philippjfr Mar 21, 2017

Author Contributor

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

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

This comment has been minimized.

@jlstevens

jlstevens Mar 21, 2017

Contributor

Is this relevant to hooking up to events?

This comment has been minimized.

@philippjfr

philippjfr Mar 21, 2017

Author Contributor

Please stop reviewing this PR.

This comment has been minimized.

@jlstevens

jlstevens Mar 21, 2017

Contributor

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.

This comment has been minimized.

@philippjfr

philippjfr Mar 21, 2017

Author Contributor

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.

This comment has been minimized.

@jbednar

jbednar Mar 21, 2017

Contributor

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.

This comment has been minimized.

@jbednar

jbednar Mar 21, 2017

Contributor

Or WIP and WIP-pre-review...

This comment has been minimized.

@jlstevens

jlstevens Mar 21, 2017

Contributor

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.

This comment has been minimized.

@jbednar

jbednar Mar 21, 2017

Contributor

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.

This comment has been minimized.

@jlstevens

jlstevens Mar 21, 2017

Contributor

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.

This comment has been minimized.

@philippjfr

philippjfr Mar 21, 2017

Author Contributor

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

@philippjfr philippjfr force-pushed the bokeh_event_callbacks branch from 47ee18c to c850de3 Mar 21, 2017

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Mar 23, 2017

Ready for review.

@philippjfr philippjfr removed the WIP label Mar 23, 2017

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

This comment has been minimized.

@jlstevens

jlstevens Mar 23, 2017

Contributor

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

This comment has been minimized.

@philippjfr

philippjfr Mar 23, 2017

Author Contributor

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.

* 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.

This comment has been minimized.

@jlstevens

jlstevens Mar 23, 2017

Contributor

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'.

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.

This comment has been minimized.

@jlstevens

jlstevens Mar 23, 2017

Contributor

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

This comment has been minimized.

@philippjfr

philippjfr Mar 23, 2017

Author Contributor

That's what this line was about:

which can reference models requested by the callback,

but I'll try to clarify.

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

This comment has been minimized.

@jlstevens

jlstevens Mar 23, 2017

Contributor

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

This comment has been minimized.

@philippjfr

philippjfr Mar 23, 2017

Author Contributor

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

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

This comment has been minimized.

@jlstevens

jlstevens Mar 23, 2017

Contributor

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

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

This comment has been minimized.

@jlstevens

jlstevens Mar 23, 2017

Contributor

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

This comment has been minimized.

@philippjfr

philippjfr Mar 23, 2017

Author Contributor

Agreed, definitely needs a comment.

data = "var data = {};\n"
code = conditional + data + attributes + self.code + self_callback

js_callback = CustomJS(args=references, code=code)

This comment has been minimized.

@jlstevens

jlstevens Mar 23, 2017

Contributor

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...


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

This comment has been minimized.

@jlstevens

jlstevens Mar 23, 2017

Contributor

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

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


class PlotDimensionCallback(Callback):

This comment has been minimized.

@jlstevens

jlstevens Mar 23, 2017

Contributor

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?

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.

This comment has been minimized.

@jlstevens

jlstevens Mar 23, 2017

Contributor

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

This comment has been minimized.

@jlstevens

jlstevens Mar 23, 2017

Contributor

How about live?

This comment has been minimized.

@jlstevens

jlstevens Mar 23, 2017

Contributor

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

This comment has been minimized.

@philippjfr

philippjfr Mar 23, 2017

Author Contributor

Prefer interactive tbh.

This comment has been minimized.

@jlstevens

jlstevens Mar 23, 2017

Contributor

@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).

This comment has been minimized.

@jbednar

jbednar Mar 23, 2017

Contributor

automatic? autoupdate? tied?

This comment has been minimized.

@philippjfr

philippjfr Mar 23, 2017

Author Contributor

linked?

This comment has been minimized.

@jbednar

jbednar Mar 23, 2017

Contributor

Sure, linked.

This comment has been minimized.

@jlstevens

jlstevens Mar 23, 2017

Contributor

linked isn't bad

@@ -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]

This comment has been minimized.

@jlstevens

jlstevens Mar 23, 2017

Contributor

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

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()}

This comment has been minimized.

@jlstevens

jlstevens Mar 23, 2017

Contributor

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

This comment has been minimized.

@jlstevens

jlstevens Mar 23, 2017

Contributor

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

@philippjfr philippjfr force-pushed the bokeh_event_callbacks branch from 5087067 to 5d54235 Mar 23, 2017

@philippjfr philippjfr force-pushed the bokeh_event_callbacks branch from 95a254f to 522952a Mar 23, 2017

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Mar 23, 2017

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

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Mar 23, 2017

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

This comment has been minimized.

Copy link
Contributor

jlstevens commented Mar 23, 2017

Ok, merging.

@jlstevens jlstevens merged commit 07ab51e into master Mar 23, 2017

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.5%) to 76.973%
Details
s3-reference-data-cache Test data is cached.
Details

@jbednar jbednar deleted the bokeh_event_callbacks branch Mar 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.