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

Safe embed [doc-build] #1040

Merged
merged 4 commits into from
Jan 30, 2020
Merged

Safe embed [doc-build] #1040

merged 4 commits into from
Jan 30, 2020

Conversation

philippjfr
Copy link
Member

Reverts #1021

Just to recap some of the conversation/explanation from that PR:

Just to explain both the issues you are having with HoloViews and what this workaround is meant to achieve. Embedding in Panel (and previously the HoloMap support in HoloViews) work by collecting all the widgets in an app that have a discrete set of values and then taking their cross product to come up with the state space. The embedding code then iterates over that state space and records the events that are generated as each change is applied. This works well for simple cases but can exhibit issues with more complex cases when combined with HoloViews. Specifically HoloViews tries to compute the minimal set of events to go from one state to the next. Imagine you have states A, B and C, the embedding code will go from A->B and record the events, and then go from B->C and record the events. However, since it's computing minimal diffs the transition from C->B may not include all the events to fully restore the view to the original state A, because it has only recorded the diff to go from A->B and A and B may be more similar than C and A. This problem is particularly acute when we change column labels.

The correct solution would be to ensure that during embedding HoloViews always includes all the events necessary to fully restore the state. An alternative solution would be to find a path through the state space that applies all the events to get to that state consecutively. Both are pretty difficult problems, particularly while also still minimizing the size of the resulting data. This PR is a total hammer to solve this problem, instead of saying "let's try to compute a minimal diff between A and B" it says, let's just embed the entire plot for each state change. This will result in much bigger file sizes and trigger a full rerender of the plot but it also guarantees all the state changes are recorded in full.

The new approach in this PR provides a context manager which simply short circuits bokeh's mechanism to see if some property has changed or not, always telling it that the property has changed and an event should be generated. This means that for each state change all properties that are altered will be recorded. This is still no 100% guarantee that all changes are captured since a user might have conditional logic but it should capture the vast majority of cases in HoloViews and using changes in HoloViews we can make sure that all necessary events are captured.

@codecov
Copy link

codecov bot commented Jan 29, 2020

Codecov Report

Merging #1040 into master will increase coverage by 0.01%.
The diff coverage is 87.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1040      +/-   ##
==========================================
+ Coverage   85.76%   85.78%   +0.01%     
==========================================
  Files         105      105              
  Lines       12019    12275     +256     
==========================================
+ Hits        10308    10530     +222     
- Misses       1711     1745      +34
Impacted Files Coverage Δ
panel/pane/holoviews.py 84.63% <100%> (+1.42%) ⬆️
panel/io/notebook.py 62.71% <100%> (+0.58%) ⬆️
panel/config.py 48.27% <100%> (+1.6%) ⬆️
panel/io/embed.py 88.55% <71.42%> (+0.03%) ⬆️
panel/io/save.py 85.48% <0%> (+8.12%) ⬆️
panel/viewable.py 85.58% <0%> (+3.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2701968...7e3c169. Read the comment docs.

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.

1 participant