-
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
Feature: fetch all widgets in one single comm message using the control channel #766
Feature: fetch all widgets in one single comm message using the control channel #766
Conversation
This will connect to the kernel as soon as possible, and tries to create widget during execution. This avoids having to get all the widgets after execution, and is an alterative to; voila-dashboards#766 Issues: * since the whole jupyter message handling is 'synchronous' it awaits at a top level and goes all the way down, get_model needs to be resolved without any new message coming from the kernel. * Despite making handle_comm_open not returning a promise, we still get a stalling of the whole message q. Despite these issues, the loading time is significantly reduced, since the page renders almost instantly when it succeeds (e.g. when no widgets are missed).
I am rebasing this and exploring it |
7dc80ff
to
90354df
Compare
Adding the widget model creation to the PR. Performance results:
|
The model creation works now (no support for binary buffers yet), but passing all messages through one comm doesn't seem to work, I need to explore more on this. |
I'm afraid this is more difficult, since we may start listening after say 20% of the widgets are created, and if we want to create a widget while we missed the creation of a previously created widget that we depend on, we'll enter a 'deadlock' : We'll await the creation of our depending widget, and during that time we will not process any other message, since we 'await through the whole callstack', and the incoming websocket messages just accumulate. This means that we also cannot ask for the state of a previously created widget. I hope my description makes sense, let me know if want need a video call for some higher bandwidth channel. |
I am not sure I understand your description 😅 getting into a call could help indeed. What I am exploring now is to use your |
@maartenbreddels it seems to work with the following approach:
Performances are still good: Before: 4400msec |
I am also afraid this will go over the 10mb per websocket message. I guess this is still an issue |
Oh I didn't know this was a limitation. Don't websockets send messages in batches? |
@maartenbreddels I went a bit far testing this 😅 I created a grid of 1225 bqplot plots, each plot being a 1000 points line. The entire websocket message was 23_446_769 bytes, and it worked! Time to fetch and create all models |
Haha, cool! |
Oooh it's a tornado limitation, I see... Well, the good thing is we can bypass this with |
b663b7d
to
72ba1b9
Compare
catch { | ||
console.warn('Failed to open "jupyter.widget.control" comm channel, fallback to slow fetching of widgets.'); | ||
// Fallback to the old implementation for old ipywidgets versions (<=7.6) | ||
return this._build_models_slow(); |
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.
If the backend for ipywidgets does not implement the control channel, we fallback to the old implementation
6608b1e
to
5a184b9
Compare
This PR could technically be merged already. Because it falls back to the previous implementation if the control channel is not responding. But let's wait for its sibling to be merged first: jupyter-widgets/ipywidgets#3021 |
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-12-21 15:54:33.209823771 +0000
+++ /dev/fd/62 2021-12-21 15:54:33.209823771 +0000
@@ -4,37 +4,37 @@
"BENCHMARK_REFERENCE": "actual"
},
"browsers": {
- "chromium": "97.0.4666.0"
+ "chromium": "94.0.4595.0"
},
"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": 7289614336
+ "total": 7291699200
},
"osInfo": {
"arch": "x64",
@@ -42,11 +42,11 @@
"codename": "Focal Fossa",
"codepage": "UTF-8",
"distro": "Ubuntu",
- "kernel": "5.11.0-1022-azure",
+ "kernel": "5.8.0-1040-azure",
"logofile": "ubuntu",
"platform": "linux",
"release": "20.04.3 LTS",
- "serial": "4a9fc102c841404db9b929f440d77193",
+ "serial": "cfc067bfcb844f35865e279a1b0e66c5",
"servicepack": "",
"uefi": false
} |
Marking this PR as ready to review. This can be merged and released before ipywidgets supports the control channel in an official release. This way it will work as soon as ipywidgets 8 or 7.7 is out |
781002b
to
bce3284
Compare
bce3284
to
041c686
Compare
Thanks for your review! |
The ui-tests are failing but I guess it's only due to the ipympl update: #1048 |
Through the new control comm target
041c686
to
fbf017c
Compare
Rebased |
Thanks!! |
Btw, the reason we don't see this tornado limit is that it's a limit for incoming traffic, such that clients cannot pump the server full of terrabytes of data which it is forced to keep into memory. Downloading has no limit, since there the server is more in control. |
Related to #764
The way we fetch the widgets from the a kernel, is that we request all comm id's, and send an 'update' message to each comm to request the widget state. That means that for each widget, we send 1 message (update) and we receive 3 messages (busy, comm, idle). This leads to bad performance, since there is a per-message overhead in all layers (kernel, jupyter server, browser). On top of that, we don't know if the Comms are actually widgets.
I think we should have some higher level of communication, to talk 'widget' instead of 'comm', through a single comm object. This will allow us to request the full widget state in 1 request, and get the full state as 1 reply (or multiple if we want to).
Performance results:
Fetching of 1000 Layout widgets from the kernel
That's a 5x increase in performance, and I think much less load on the voila/jupyter_server server.
Note that this PR only downloads the widget data, it does not build the widgets yet.