-
Notifications
You must be signed in to change notification settings - Fork 505
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
Implement FPS-based Voila server #984
Conversation
@davidbrochart #936 is merged, |
Thanks, I will look at it. |
7c59c1f
to
089dd02
Compare
I just rebased, now that #936 is merged. |
089dd02
to
cdd874b
Compare
Hi @davidbrochart, I know this PR is still in the early phase but what do you think about the idea of refactoring the current |
That sounds good @trungleduc, but what do you mean exactly? Having two methods, one for launching the Tornado application and one for launching the Uvicorn application? |
I'm thinking about having a common interface for both and the CLI app does not need to know about the webserver implementation. |
But then how do you switch between jupyter-server and FPS? Currently it is passed as a CLI option: voila my_notebook.ipynb --fps |
Maybe we need to think more about it but the first thing popped in my head is to keep a mapping between the server configuration ( |
f58e770
to
186f00e
Compare
Benchmark reportThe execution time (in milliseconds) are grouped by test file, test type and browser. Results table
❗ Test metadata have changed--- /dev/fd/63 2021-11-04 11:58:41.981693399 +0000
+++ /dev/fd/62 2021-11-04 11:58:41.981693399 +0000
@@ -8,33 +8,33 @@
},
"systemInformation": {
"cpu": {
- "brand": "Xeon® E5-2673 v4",
+ "brand": "Xeon® E5-2673 v3",
"cache": {
"l1d": 65536,
"l1i": 65536,
"l2": 524288,
- "l3": 52428800
+ "l3": 31457280
},
"cores": 2,
"family": "6",
- "flags": "fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush mmx fxsr sse sse2 ss ht syscall nx pdpe1gb rdtscp lm constant_tsc rep_good nopl xtopology cpuid pni pclmulqdq ssse3 fma cx16 pcid sse4_1 sse4_2 movbe popcnt aes xsave avx f16c rdrand hypervisor lahf_lm abm 3dnowprefetch invpcid_single pti fsgsbase bmi1 hle avx2 smep bmi2 erms invpcid rtm rdseed adx smap xsaveopt md_clear",
+ "flags": "fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush mmx fxsr sse sse2 ss ht syscall nx pdpe1gb rdtscp lm constant_tsc rep_good nopl xtopology cpuid pni pclmulqdq ssse3 fma cx16 pcid sse4_1 sse4_2 movbe popcnt aes xsave avx f16c rdrand hypervisor lahf_lm abm invpcid_single pti fsgsbase bmi1 avx2 smep bmi2 erms invpcid xsaveopt md_clear",
"governor": "",
"manufacturer": "Intel®",
- "model": "79",
+ "model": "63",
"physicalCores": 2,
"processors": 1,
"revision": "",
"socket": "",
- "speed": 2.3,
+ "speed": 2.4,
"speedMax": null,
"speedMin": null,
- "stepping": "1",
+ "stepping": "2",
"vendor": "GenuineIntel",
"virtualization": false,
"voltage": ""
},
"mem": {
- "total": 7289622528
+ "total": 7291699200
},
"osInfo": {
"arch": "x64",
@@ -42,11 +42,11 @@
"codename": "Focal Fossa",
"codepage": "UTF-8",
"distro": "Ubuntu",
- "kernel": "5.11.0-1020-azure",
+ "kernel": "5.8.0-1040-azure",
"logofile": "ubuntu",
"platform": "linux",
"release": "20.04.3 LTS",
- "serial": "92bef7b423eb4bb0b05dd0208eb3d3da",
+ "serial": "cfc067bfcb844f35865e279a1b0e66c5",
"servicepack": "",
"uefi": false
} |
541eee9
to
ecd05cb
Compare
Thank @davidbrochart for your work on this, this is looking very nice! |
One question about testing, can we make all tests run on both |
voila/handler.py
Outdated
self.voila_configuration = kwargs['voila_configuration'] | ||
# we want to avoid starting multiple kernels due to template mistakes | ||
self.kernel_started = False | ||
async def _get(self, path=None): |
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 think we might need docstring and typing for this function because using self
as a function parameter is a little bit confusing.
I didn't look at tests, I think they are very tightly coupled with Tornado and jupyter-server. |
UI tests are now passing. |
ad83d67
to
098cd89
Compare
I think you need to update the screenshots for |
9d776d2
to
e78fddb
Compare
e78fddb
to
ae49c21
Compare
@davidbrochart should we close this PR? It looks like |
I didn't remember we started that. Yes I'll close it and we will need to open a new PR when jupyter-server/jupyverse#277 is merged. |
References
Closes #977
Code changes
fps
flag, allowing to use FPS instead of Tornado.VoilaHandler
depend on a server-agnostic_VoilaHandler
class, so that the FPS server can reuse most of the handler logic.fps-voila
FPS plugin, which implements aFPSVoilaHandler
based on_VoilaHandler
.User-facing changes
By default, Voila is backed by
jupyter-server
(no change). To use FPS, launch Voila with the--fps
flag.Limitations
This is still a POC, which allows to serve a notebook with widgets, but: