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

Bokeh 0.11 Compatibility #393

Merged
merged 14 commits into from
Jan 7, 2016
Merged

Bokeh 0.11 Compatibility #393

merged 14 commits into from
Jan 7, 2016

Conversation

philippjfr
Copy link
Member

This PR adds various fixes to ensure HoloViews is compatible with the new 0.11 bokeh release.

It is now ready for merge.

if LooseVersion(bokeh.__version__) >= LooseVersion('0.11'):
old_bokeh = False
from bokeh.io import _CommsHandle
from bokeh.util.notebook import get_comms
Copy link
Contributor

Choose a reason for hiding this comment

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

How much can we trust that _CommsHandle is something we can rely on in future? From the name, it doesn't look like something we are expected to use directly...

Copy link
Member Author

Choose a reason for hiding this comment

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

Certainly possible that they will change it but there's no way around it. Bokeh notebook support is currently set up to only keep one CommsHandle open at the same so we can't rely on their core API. It's certainly worth keeping in mind though and replace it with a call to a higher-level API once/if that becomes possible.

Copy link
Member

Choose a reason for hiding this comment

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

It's worth posting an issue on the Bokeh site asking whether this is an ok long-term solution, and/or request a better one be made available.

Copy link
Member Author

Choose a reason for hiding this comment

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

I know that handling multiple Comms at once is on their radar and might even land in 0.11.1. If need be I can mirror CommsHandle in HoloViews, it's a tiny class that's just meant to hold a few attributes.

@jlstevens
Copy link
Contributor

I've made some comments above but I'm afraid I don't know enough details to do a very thorough review. I do have one general comment though...

The changes here seem to be of two types: 1. actual changes in the bokeh API to smooth over with conditional statements 2. improvements to how the code works that is compatible across both bokeh versions (i.e understanding how to leverage bokeh better).

The second type of change seems general and I assume fairly stable. The first type of change is a lot of special cases and I worry that as bokeh keeps developing (and the API keeps getting tweaked) that the list of these fixes will keep growing. I'm not sure it is needed now but I might consider a class in util or even a file compat.py if we think the number of version dependent fixes will keep expanding in future.

@philippjfr
Copy link
Member Author

Agree that we'll have to add a compat.py module if there are a lot more changes to the API coming. It could simply supply various version agnostic functions to do specific things. Currently it doesn't seem worth it though.

@jlstevens
Copy link
Contributor

Tests are all passing so I'll merge now.

jlstevens added a commit that referenced this pull request Jan 7, 2016
@jlstevens jlstevens merged commit 817aead into master Jan 7, 2016
@jlstevens jlstevens deleted the bokeh0.11_compat branch January 28, 2016 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants