-
Notifications
You must be signed in to change notification settings - Fork 2
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?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #279 +/- ##
==========================================
- Coverage 59.00% 58.79% -0.22%
==========================================
Files 23 23
Lines 4945 4997 +52
==========================================
+ Hits 2918 2938 +20
- Misses 2027 2059 +32 ☔ View full report in Codecov by Sentry. |
That's ok, then we will be able to remove the duplication from the dispatcher code. |
@@ -42,13 +42,15 @@ | |||
"astroquery", | |||
"scipy", | |||
"rdflib", | |||
"black" | |||
"black", | |||
"bokeh" |
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.
same for the frontend
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.
with 693e88d the html output for LCs will have bokeh-3.5.1
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.
with 693e88d the html output for LCs will have bokeh-3.5.1
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.
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?
Exactly, more specifically, it is done here:
Lines 715 to 719 in 02e47e3
html_str = html_dict['div'] + '\n' | |
html_str += '<script src="https://cdn.bokeh.org/bokeh/release/bokeh-2.4.2.min.js"></script>\n' + \ | |
'<script src="https://cdn.bokeh.org/bokeh/release/bokeh-widgets-2.4.2.min.js"></script>\n' | |
html_str += html_dict['script'] | |
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
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.
As discussed some time ago, I removed the dependency we had from Dispatcher by moving the class into plot_tools.
There is some code duplication from the two plot tools, but it looks reasonable.
@burnout87 please check if I was supposed to increase the version in setup.cfg and in case remove this change
I updated the requirements both n the requirements.txt file and in setup.py