-
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
Support for serving multiple apps with index with Bokeh and/ or Panel Server #1
Conversation
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.
Thank you so much for all of this work. Your insight into Panel and Bokeh is really important!
I just have a couple of ideas about this code. I've added a note inline suggesting we do the INDEX_HTML imports only when needed. This is mainly to save breaking if the installed components aren't quite right.
Related, it might make sense only to do the panel/bokeh imports when needed if possible. So a bokeh user doesn't have to have panel installed at all.
If we're going down that route, maybe panel (and possibly bokeh) should be removed from the requirements.txt - it's up to the user to manufacture their own environment. And installing panel can certainly be optional for them.
If we're going to keep requirements.txt then we definitely need a >= version number because I'm sure a lot of the components are fairly recent.
In the other similar commands I generally leave out the requirements for things like panel/voila. The reason is then that bokeh-root-cmd and friends can be installed as part of jhsingle-native-proxy (known as cdsdashboards-singleuser in conda), and still be relatively lightweight but have everything they will need for ContainDS Dashboards. Setting up the rest of the conda env is then down to them as normal. The idea is that if they want panel they will install it, same with voila, and that is something they should know how to do. But guessing that 'bokeh-root-cmd' is needed is not something they will know in general - so that's why I think it is good for all of those small components to be installed even if they are not going to be used.
By the way, please note that the glob doesn't work from cdsdashboards because it is your shell that is expanding the glob when running locally. If you put quotes around the "*.ipynb" you'll see it doesn't pick up the files.
Thanks again - I hope these comments are useful! Please let me know what you think.
|
||
print("Fetching Bokeh script or folder {}".format(app_py_path)) | ||
_BOKEH_INDEX_HTML = str(pathlib.Path(bokeh.server.views.__file__).parent / "app_index.html") |
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.
Please could these HTML extractions happen inside the Bokeh/PanelServer classes?
So maybe there is a get_index_html() static method that performs this operation (and can 'cache' it in a static property too).
Of course this would save performing the operation when it's not needed. Partly, I suggest this for speed, but more importantly in case something goes wrong - especially if a bokeh user doesn't quite have the right panel installed (or has none at all), so the import breaks (when it wasn't actually needed anyway!)
I also have a related note about imports and requirements which I'll add below.
I think this all makes sense. Will look at it. Probably on the weekends. |
@MarcSkovMadsen I had a chance to work on some of this - hope you don't mind. I have refactored so Panel imports happen only when needed. And also tried to distinguish between "serving a folder containing lots of .py/.ipynb files" and "serving an app which is a main.py/main.ipynb in a folder". I think it all makes sense, but your knowledge of Panel/Bokeh would be really helpful of course! |
Implements: ideonate/cdsdashboards#65
This add support for
I walk through the new functionality in this video https://youtu.be/pVjj2vMwSIs. Check it out.
FYI. @danlester