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

Conversation

Projects
None yet
3 participants
@philippjfr
Copy link
Contributor

philippjfr commented Jan 5, 2016

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

It is now ready for merge.

@philippjfr philippjfr force-pushed the bokeh0.11_compat branch from 5007d22 to 05f2bda Jan 7, 2016

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

This comment has been minimized.

@jlstevens

jlstevens Jan 7, 2016

Contributor

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

This comment has been minimized.

@philippjfr

philippjfr Jan 7, 2016

Author Contributor

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.

This comment has been minimized.

@jbednar

jbednar Jan 7, 2016

Contributor

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.

This comment has been minimized.

@philippjfr

philippjfr Jan 7, 2016

Author Contributor

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.

if old_bokeh:
json = plotobj.vm_serialize(changed_only=True)
else:
json = plotobj.to_json(False)

This comment has been minimized.

@jlstevens

jlstevens Jan 7, 2016

Contributor

If you find this pattern happening often, I would consider using a utility what does the version check and smooths over the differences between bokeh versions. I'll let you decide if this this worth it - if you only need it in this one instance, it probably doesn't need to be made into a utility right now.

This comment has been minimized.

@philippjfr

philippjfr Jan 7, 2016

Author Contributor

I think I never repeat the same pattern multiple times.

@@ -9,12 +9,13 @@
old_bokeh = True

This comment has been minimized.

@jlstevens

jlstevens Jan 7, 2016

Contributor

old_bokeh isn't a great name as one day the latest version will become old! I would consider stating the version explicitly...i.e bokeh_XX.

This comment has been minimized.

@jbednar

jbednar Jan 7, 2016

Contributor

+1

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Jan 7, 2016

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

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Jan 7, 2016

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

This comment has been minimized.

Copy link
Contributor

jlstevens commented Jan 7, 2016

Tests are all passing so I'll merge now.

jlstevens added a commit that referenced this pull request Jan 7, 2016

Merge pull request #393 from ioam/bokeh0.11_compat
Bokeh 0.11 Compatibility

@jlstevens jlstevens merged commit 817aead into master Jan 7, 2016

3 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.02%) to 70.24%
Details

@jlstevens jlstevens deleted the bokeh0.11_compat branch Jan 28, 2016

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.