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
Merged

Add support for bokeh event callbacks #1148

merged 18 commits into from Mar 23, 2017

Conversation

@philippjfr
Copy link
Member

@philippjfr 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 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 20, 2017
@@ -92,19 +104,41 @@ class Callback(object):
attributes = {}

js_callback = """
function unique_events(events) {{
Copy link
Contributor

@jlstevens jlstevens Mar 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Loading

Copy link
Member Author

@philippjfr philippjfr Mar 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Member Author

@philippjfr philippjfr Mar 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Contributor

@jlstevens jlstevens Mar 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

Loading

Copy link
Contributor

@jlstevens jlstevens Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

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

@jlstevens jlstevens Mar 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Member Author

@philippjfr philippjfr Mar 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Contributor

@jlstevens jlstevens Mar 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member Author

@philippjfr philippjfr Mar 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

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

Copy link
Contributor

@jlstevens jlstevens Mar 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this relevant to hooking up to events?

Loading

Copy link
Member Author

@philippjfr philippjfr Mar 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please stop reviewing this PR.

Loading

Copy link
Contributor

@jlstevens jlstevens Mar 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member Author

@philippjfr philippjfr Mar 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member

@jbednar jbednar Mar 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member

@jbednar jbednar Mar 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or WIP and WIP-pre-review...

Loading

Copy link
Contributor

@jlstevens jlstevens Mar 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member

@jbednar jbednar Mar 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Contributor

@jlstevens jlstevens Mar 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member Author

@philippjfr philippjfr Mar 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

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

@philippjfr philippjfr commented Mar 23, 2017

Ready for review.

Loading

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

@jlstevens jlstevens Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Member Author

@philippjfr philippjfr Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

* 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.
Copy link
Contributor

@jlstevens jlstevens Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

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.
Copy link
Contributor

@jlstevens jlstevens Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Member Author

@philippjfr philippjfr Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what this line was about:

which can reference models requested by the callback,

but I'll try to clarify.

Loading

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

@jlstevens jlstevens Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Member Author

@philippjfr philippjfr Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

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

@jlstevens jlstevens Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

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

@jlstevens jlstevens Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Member Author

@philippjfr philippjfr Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, definitely needs a comment.

Loading

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

js_callback = CustomJS(args=references, code=code)
Copy link
Contributor

@jlstevens jlstevens Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading


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

@jlstevens jlstevens Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

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


class PlotDimensionCallback(Callback):
Copy link
Contributor

@jlstevens jlstevens Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Loading

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.
Copy link
Contributor

@jlstevens jlstevens Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Contributor

@jlstevens jlstevens Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about live?

Loading

Copy link
Contributor

@jlstevens jlstevens Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Member Author

@philippjfr philippjfr Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer interactive tbh.

Loading

Copy link
Contributor

@jlstevens jlstevens Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Member

@jbednar jbednar Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

automatic? autoupdate? tied?

Loading

Copy link
Member Author

@philippjfr philippjfr Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

linked?

Loading

Copy link
Member

@jbednar jbednar Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, linked.

Loading

Copy link
Contributor

@jlstevens jlstevens Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

linked isn't bad

Loading

@@ -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]
Copy link
Contributor

@jlstevens jlstevens Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

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()}
Copy link
Contributor

@jlstevens jlstevens Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Contributor

@jlstevens jlstevens Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

@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
Copy link
Contributor

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

Loading

@philippjfr
Copy link
Member Author

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

Loading

@jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Mar 23, 2017

Ok, merging.

Loading

@jlstevens jlstevens merged commit 07ab51e into master Mar 23, 2017
4 checks passed
Loading
@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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants