Skip to content
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

Make use of JupyterLab mimetype renderers #1249

Merged
merged 10 commits into from
Mar 23, 2023

Conversation

martinRenou
Copy link
Member

@martinRenou martinRenou commented Oct 31, 2022

Code changes

This PR changes the logic for rendering cell outputs, it switches from relying on nbconvert for handling different mimetypes to relying on JupyterLab.

The PR also has the benefit of removing the custom Widget Manager from Voila! It will load the @jupyter-widgets/controls package just as any other widget extension.

User-facing changes

This adds support for many custom JupyterLab renderers: jupyterlab-fasta, vega, jupyterlab-geojson.

Screenshot from 2022-10-31 13-56-33

Backwards-incompatible changes

This might break some assumptions in the template, as it's overwriting the data_priority block in the nbconvert template.

@github-actions
Copy link
Contributor

Binder 👈 Launch a Binder on branch martinRenou/voila/lab_renderers

@martinRenou martinRenou added the enhancement New feature or request label Oct 31, 2022
@martinRenou
Copy link
Member Author

martinRenou commented Oct 31, 2022

TODO:

  • HTML do not seem to render properly yet
  • SVG do not seem to render properly yet
  • The "classic" template is broken
  • The "reveal" template seems broken
  • HTML tables are now centered, they were not before

@martinRenou
Copy link
Member Author

Panel seems to work with this PR. Though the app is not updating when clicking on buttons (unlike in JupyterLab).

I suspect panel uses the update_display logic that Voila does not support (yet?).

Friendly ping @philippjfr, is that right that panel uses IPython.display.update_display? I don't see any reference to that in Panel.

Screenshot from 2022-10-31 16-24-08

@martinRenou
Copy link
Member Author

Bokeh works but suffers from the same issue as Panel, which I suppose makes sense?

Screenshot from 2022-10-31 16-42-53

@philippjfr
Copy link

We do not use update_display, instead our Jupyter(Lab) extension lets us access a comm. As an alternative you can try wrapping the Panel app in pn.ipywidget(...). That probably requires downgrading to ipywidgets<8 though.

@philippjfr
Copy link

I suppose that doesn't really help you test out the render mime situation. I can take a look at what the problems on that side of things are when I get back from vacation.

@maartenbreddels
Copy link
Member

We do not use update_display, instead our Jupyter(Lab) extension lets us access a comm

So if comm works, bokey should work? Could you give us some pointers to the code where this is done?

@martinRenou
Copy link
Member Author

I got bokeh to work in Voila using this PR:

bokeh.webm

The issue was that I was not serving Voila at the right port (bokeh expected me to serve on localhost:8888).

@philippjfr I wonder if this UX could be improved using only kernel comms instead of custom web sockets?

@philippjfr
Copy link

philippjfr commented Nov 10, 2022

Sorry I've been traveling for several weeks now. Just to summarize the current approaches for working with Bokeh/Panel interactively in notebooks:

  1. Embedding a server: This is indeed fiddly due to setting up ports correctly and I would not recommend it as the default approach.
  2. Wrapping in an ipywidget: The second approach we support is to wrap Bokeh model in the jupyter_bokeh.widget.BokehModel ipywidget. This uses the regular IPyWidget comms to shuttle messages across the websocket. Overall this approach is pretty clean but I've always found performance slightly less than optimal for some reason. There may also still be some lingering compatibility issues with ipywidgets 3.0 for this approach. The easiest way to invoke this is to wrap your Bokeh/Panel component with pn.ipywidget(...). We primarily use this approach in other notebook environments like VSCode.
  3. Using a global comm: This is the default approach we use for Panel and HoloViews and although conceptually it's very hacky it has been the most robust approach for us for several years now. The basic idea is that we create a Proxy object around the Jupyter comm manager which we patch onto a global object (window.PyViz.comm_manager) both in JupyterLab (via our JLab extension) and in classic notebook. While rendering Panel objects we then connect to a Jupyter Comm via this proxy object. The relevant code for this lives here and here.

@martinRenou
Copy link
Member Author

martinRenou commented Nov 10, 2022

Thanks for the comment!

  1. Using a global comm

Concerning this point, how does the user code look like in this case? Should I be using this example with using output_notebook?

So, if I understand correctly, the @pyviz/jupyterlab_pyviz labextension should probably work with this PR, but if panel does not update it probably means it does not work. I will debug a bit more.

@maartenbreddels
Copy link
Member

2. Overall this approach is pretty clean but I've always found performance slightly less than optimal for some reason

I wonder if that's the server parsing all json, and encoding it again (that should improve due to @davidbrochart work on jupyter server I guess). My guess is that in situation 1 you just have a WebSocket, and skip a few layers in ipykernel/jupyter_server.

@philippjfr
Copy link

So, if I understand correctly, the @pyviz/jupyterlab_pyviz labextension should probably work with this PR,

Yup, that sounds right. Also happy to take a look.

@jtpio jtpio added this to the 0.5.0 milestone Nov 14, 2022
@jtpio
Copy link
Member

jtpio commented Nov 14, 2022

The PR also has the benefit of removing the custom Widget Manager from Voila! It will load the @jupyter-widgets/controls package just as any other widget extension.

Really nice! 👍

@jtpio
Copy link
Member

jtpio commented Nov 14, 2022

  • HTML do not seem to render properly yet
  • SVG do not seem to render properly yet

This might need adding the corresponding lab extensions to the app.


Some additional observations after a quick check on the UI tests failures:

@github-actions
Copy link
Contributor

github-actions bot commented Mar 20, 2023

Benchmark report

The execution time (in milliseconds) are grouped by test file, test type and browser.
For each case, the following values are computed: min <- [1st quartile - median - 3rd quartile] -> max.

Results table
Test file voila-tree-classic.ipynb voila-tree-light.ipynb voila-tree-dark.ipynb voila-tree-miami.ipynb basics.ipynb bqplot.ipynb dashboard.ipynb gridspecLayout.ipynb interactive.ipynb ipympl.ipynb mimerenderers.ipynb bokeh.ipynb multiple_widgets.ipynb query-strings.ipynb reveal.ipynb
Render
chromium
actual 66 <- [72 - 76 - 91] -> 132 63 <- [68 - 73 - 89] -> 126 66 <- [71 - 78 - 91] -> 116 74 <- [79 - 90 - 110] -> 165 2617 <- [2664 - 2754 - 2943] -> 4035 2456 <- [2468 - 2493 - 2511] -> 2598 2443 <- [2547 - 2592 - 2685] -> 2868 2454 <- [2488 - 2559 - 2673] -> 2730 2009 <- [2061 - 2141 - 2194] -> 2450 3137 <- [3190 - 3324 - 3411] -> 3768 6614 <- [6684 - 6767 - 6955] -> 7366 4364 <- [4673 - 5153 - 6097] -> 8745 3813 <- [3832 - 3871 - 3998] -> 4163 1662 <- [1668 - 1723 - 1831] -> 2169 2765 <- [2777 - 2797 - 2872] -> 3106
expected 3379 <- [3442 - 3517 - 3701] -> 3876 2976 <- [3227 - 3321 - 3421] -> 3604 3608 <- [3623 - 3709 - 3793] -> 3825 4453 <- [4453 - 4523 - 4661] -> 4748 2559 <- [2655 - 2656 - 2660] -> 2674 3982 <- [4079 - 4213 - 4356] -> 4743 12183 <- [18509 - 19553 - 20811] -> 21515 15319 <- [15660 - 15796 - 15912] -> 16056 1517 <- [1920 - 1997 - 2103] -> 2113

❗ Test metadata have changed
--- /dev/fd/63	2023-03-22 15:31:29.037327991 +0000
+++ /dev/fd/62	2023-03-22 15:31:29.041327999 +0000
@@ -4,49 +4,49 @@
     "BENCHMARK_REFERENCE": "actual"
   },
   "browsers": {
-    "chromium": "97.0.4666.0"
+    "chromium": "94.0.4595.0"
   },
   "systemInformation": {
     "cpu": {
-      "brand": "Xeon® Platinum 8370C",
+      "brand": "Xeon® E5-2673 v3",
       "cache": {
-        "l1d": 98304,
+        "l1d": 65536,
         "l1i": 65536,
-        "l2": 2097152,
-        "l3": 50331648
+        "l2": 524288,
+        "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 avx512f avx512dq rdseed adx smap clflushopt avx512cd avx512bw avx512vl xsaveopt xsavec xsaves 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": "106",
+      "model": "63",
       "physicalCores": 2,
       "processors": 1,
       "revision": "",
       "socket": "",
-      "speed": 2.8,
+      "speed": 2.4,
       "speedMax": null,
       "speedMin": null,
-      "stepping": "6",
+      "stepping": "2",
       "vendor": "GenuineIntel",
       "virtualization": false,
       "voltage": ""
     },
     "mem": {
-      "total": 7281278976
+      "total": 7291699200
     },
     "osInfo": {
       "arch": "x64",
       "build": "",
-      "codename": "Jammy Jellyfish",
+      "codename": "Focal Fossa",
       "codepage": "UTF-8",
       "distro": "Ubuntu",
-      "kernel": "5.15.0-1034-azure",
+      "kernel": "5.8.0-1040-azure",
       "logofile": "ubuntu",
       "platform": "linux",
-      "release": "22.04.2 LTS",
-      "serial": "2eb8d65c617f413abe6f4d67d8f306ea",
+      "release": "20.04.3 LTS",
+      "serial": "cfc067bfcb844f35865e279a1b0e66c5",
       "servicepack": "",
       "uefi": false
     }

@martinRenou martinRenou marked this pull request as ready for review March 20, 2023 16:53
Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice thanks @martinRenou changes look good!

Trying to test the PR on Binder gives the following error:

image

Not sure it's related to this change though.

packages/voila/src/plugins.ts Outdated Show resolved Hide resolved
voila/exporter.py Outdated Show resolved Hide resolved
voila/handler.py Outdated Show resolved Hide resolved
@trungleduc
Copy link
Member

pre-commit.ci run

We were pulling ipykernel as part of the ipywidgets dependency...
@trungleduc
Copy link
Member

Thanks @martinRenou

@martinRenou martinRenou merged commit 839c42a into voila-dashboards:main Mar 23, 2023
@martinRenou martinRenou deleted the lab_renderers branch March 23, 2023 08:15
@jtpio
Copy link
Member

jtpio commented Mar 23, 2023

Nice, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants