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

Various fixes for bokeh static_source optimization #1413

Merged
merged 5 commits into from May 28, 2017

Conversation

Projects
None yet
2 participants
@philippjfr
Copy link
Contributor

philippjfr commented May 6, 2017

In #1396 it was suggested that the static_source optimization in bokeh should only kick in if both the element and its data are static.

@philippjfr philippjfr added the bokeh label May 6, 2017

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented May 6, 2017

Looks good.

I can't really see how to test this right now so I'll merge once the tests pass.

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented May 6, 2017

So this approach does not work since it just disables the optimization entirely. The issue is twofold a) when a custom style is applied the element id changes, b) something in the plotting pipeline applies a dynamic method to the data, which again ends up changing the element id. Therefore the plotting code never sees the same element id twice and the optimization does nothing. Will have to think about this.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented May 6, 2017

Problem b) seems like the real issue to me as I don't think disabling the optimization for a) means it will always be disabled.

@philippjfr philippjfr added the WIP label May 8, 2017

@philippjfr philippjfr force-pushed the static_source_opt_fix branch from 28350a3 to bdc69a7 May 28, 2017

@@ -488,6 +489,7 @@ def __init__(self, data, id=None, **params):
"""
self.data = data
self.id = id
self._plot_id = plot_id or obj_id(self)

This comment has been minimized.

@jlstevens

jlstevens May 28, 2017

Contributor

Just use __builtins__.id to avoid the name shadowing issue.

@@ -488,6 +489,7 @@ def __init__(self, data, id=None, **params):
"""
self.data = data
self.id = id

This comment has been minimized.

@jlstevens

jlstevens May 28, 2017

Contributor

Do we ever actually set the id via the constructor?

This comment has been minimized.

@jlstevens

jlstevens May 28, 2017

Contributor

Ah, probably for clone. Nevermind my question!

This comment has been minimized.

@philippjfr

philippjfr May 28, 2017

Author Contributor

Yes, clone does all the time.

else:
current_id = id(element.data)
current_id = element._plot_id

This comment has been minimized.

@jlstevens

jlstevens May 28, 2017

Contributor

Is it no longer taking into account the id of the data? It seems to do that in tabular...

@@ -147,7 +147,7 @@ def sync_sources(self):
from the same object.
"""
get_sources = lambda x: (id(x.current_frame.data), x)
filter_fn = lambda x: (x.shared_datasource and x.current_frame and
filter_fn = lambda x: (x.shared_datasource and x.current_frame is not None and

This comment has been minimized.

@jlstevens

jlstevens May 28, 2017

Contributor

Just to confirm: is this a a fix unrelated to the optimization itself?

This comment has been minimized.

@philippjfr

philippjfr May 28, 2017

Author Contributor

Seems so, not sure why I pushed it here, but it is an important fix.

previous_id = self.handles.get('previous_id', None)
current_id = id(self.current_frame.data) if self.current_frame else None
current_id = None if self.current_frame is None else id(element)+id(element.data)

This comment has been minimized.

@jlstevens

jlstevens May 28, 2017

Contributor

Summing ids is not a safe test for uniqueness as many pairs of ids could sum to the same value (even if it is unlikely!). Note that collisions might not even be that rare as the id doesn't return a hash - it is more like to a memory address.

To check the identity of both the element and the data I would use the tuple (id(element), id(element.data)) and not the sum.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented May 28, 2017

Looks good with the latest fixes. One last import issue to fix and then I'll merge once the tests go green. That said, are there any meaningful unit tests we should set up to test the optimization?

@philippjfr philippjfr force-pushed the static_source_opt_fix branch from a80c3cb to 43b3c38 May 28, 2017

@philippjfr philippjfr force-pushed the static_source_opt_fix branch from 43b3c38 to a203695 May 28, 2017

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented May 28, 2017

Thanks for adding the unit tests! Merging.

@jlstevens jlstevens merged commit bd7b264 into master May 28, 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 increased (+0.2%) to 78.996%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the static_source_opt_fix branch Jun 17, 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.