Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Remove dispatcher plot dependencies and upgrade bokeh #279
base: master
Are you sure you want to change the base?
Remove dispatcher plot dependencies and upgrade bokeh #279
Changes from 8 commits
a426466
9bd0ebe
ef9864b
d6113e2
02d204d
74ed463
47fe79e
15bfa5f
b45b207
a103607
7abe190
d81f799
0473701
3d9201a
693e88d
ba4ec4f
04d428f
0161ffc
6d83cea
3d90f3e
dd60c21
006945b
4dff727
148e659
11bebd5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 723 in oda_api/plot_tools.py
Codecov / codecov/patch
oda_api/plot_tools.py#L723
Check warning on line 805 in oda_api/plot_tools.py
Codecov / codecov/patch
oda_api/plot_tools.py#L805
Check warning on line 1074 in oda_api/plot_tools.py
Codecov / codecov/patch
oda_api/plot_tools.py#L1074
Check warning on line 1076 in oda_api/plot_tools.py
Codecov / codecov/patch
oda_api/plot_tools.py#L1076
Check warning on line 1083 in oda_api/plot_tools.py
Codecov / codecov/patch
oda_api/plot_tools.py#L1082-L1083
Check warning on line 1086 in oda_api/plot_tools.py
Codecov / codecov/patch
oda_api/plot_tools.py#L1085-L1086
Check warning on line 1091 in oda_api/plot_tools.py
Codecov / codecov/patch
oda_api/plot_tools.py#L1091
Check warning on line 1099 in oda_api/plot_tools.py
Codecov / codecov/patch
oda_api/plot_tools.py#L1093-L1099
Check warning on line 1107 in oda_api/plot_tools.py
Codecov / codecov/patch
oda_api/plot_tools.py#L1101-L1107
Check warning on line 1113 in oda_api/plot_tools.py
Codecov / codecov/patch
oda_api/plot_tools.py#L1112-L1113
Check warning on line 1115 in oda_api/plot_tools.py
Codecov / codecov/patch
oda_api/plot_tools.py#L1115
Check warning on line 1119 in oda_api/plot_tools.py
Codecov / codecov/patch
oda_api/plot_tools.py#L1118-L1119
Check warning on line 1121 in oda_api/plot_tools.py
Codecov / codecov/patch
oda_api/plot_tools.py#L1121
Check warning on line 1127 in oda_api/plot_tools.py
Codecov / codecov/patch
oda_api/plot_tools.py#L1127
Check warning on line 1135 in oda_api/plot_tools.py
Codecov / codecov/patch
oda_api/plot_tools.py#L1135
Check warning on line 1143 in oda_api/plot_tools.py
Codecov / codecov/patch
oda_api/plot_tools.py#L1140-L1143
Check warning on line 1151 in oda_api/plot_tools.py
Codecov / codecov/patch
oda_api/plot_tools.py#L1150-L1151
Check warning on line 1157 in oda_api/plot_tools.py
Codecov / codecov/patch
oda_api/plot_tools.py#L1157
Check warning on line 1161 in oda_api/plot_tools.py
Codecov / codecov/patch
oda_api/plot_tools.py#L1161
Check warning on line 1166 in oda_api/plot_tools.py
Codecov / codecov/patch
oda_api/plot_tools.py#L1163-L1166
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In dispatcher, we fixed the bokeh version to be exactly
2.4.2
The purpose of this is that it has to coincide with the version of the accompanying js library in the frontend.
On the other hand, it's probably a good time to update it to the latest version, which is currently 3.5.1. One of the motivations is that, strictly speaking, the support for python 3.10 was added in bokeh 3.0, and we test oda_api package against 3.8 - 3.11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some complication is that it should be done in coordinated fashion between oda_api, dispatcher, frontend, and possibly gallery (not sure, please confirm)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably drop 3.8 and maybe 3.9 but we need to update the container base to reconcile all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the gallery it should not be necessary, same for the frontend, so it's probably only oda_api and dispatcher
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Frontend serves bokeh js libs that are used in conjunction with what dispatcher returns. That's why I also thought about gallery, if there are any bokeh-based products there.
So the change is small, just to updating the version, but it's backwards-incompatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to try, how it works with frontend, because it injects bokeh==2.4.2 in a page head
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to specify, the update is about the
get_html_image
function used to generate LC plots for the gallery.For the frontend, a dedicated PR is needed on the dispatcher
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't quite follow how it works in gallery. Each plot there have their own bokeh version injected, and they don't conflict because each product is on a different page, right?
In frontend, it may be more complicated, say some products are cached with previous version of bokeh, and some other is generated with recent version injected. But all the tabs are on the same page, and here we may encounter problems
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, more specifically, it is done here:
oda_api/oda_api/plot_tools.py
Lines 715 to 719 in 02e47e3
Yes I can totally see it. And a potential workaround would require probably a lot of work. is it for sure not retro-compatible ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw failures when bokeh version in python and js are different. Between major releases, they surely won't be compatible.