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

Lab theme handling #1089

Merged
merged 2 commits into from
Feb 14, 2022
Merged

Conversation

martinRenou
Copy link
Member

References

Builds on top of jupyter/nbconvert#1703

Code changes

Add handlers for the JupyterLab federated extension themes. This adds a dependency on jupyterlab_server which provides the ThemesHandler.

User-facing changes

voila --theme=jupyterlab_miami_nights
miami.mp4

Backwards-incompatible changes

None

@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2022

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

@martinRenou
Copy link
Member Author

I need to update the visual regression test references (due to nbconvert change). Will wait for the bot to be added in #1083.

@trungleduc
Copy link
Member

Hi @martinRenou, if I understand correctly jupyter/nbconvert#1703, the content of the CSS files will be embedded directly into the generated HTML, so why in Voila case we need to add static URLs for the CSS files and then depend on jupyterlab_server?

@martinRenou
Copy link
Member Author

One of the advantages of properly serving the theme assets (CSS, images, fonts) is that it allows the browser to cache them. While if we embed them in the HTML the browser will not be able to cache them.

In the case of nbconvert we want the resulting HTML file to be portable (we want to be able to render the HTML file without having to look for the CSS assets somewhere else), that's why embedding makes sense. But in the case of Voila we have a proper server, so we should probably use it.

@martinRenou
Copy link
Member Author

martinRenou commented Feb 10, 2022

Also, adding the jupyterlab_server dependency might seem overkill just for this PR, but it will also be useful later for when Voila is a JupyterLab remix.

@trungleduc
Copy link
Member

Thanks for the explanation! I forgot about the caching, it totally makes sense to serve assets via URL.

@martinRenou
Copy link
Member Author

Sure! :)

I should probably add a visual regression test to this PR

@trungleduc
Copy link
Member

Sure! :)

I should probably add a visual regression test to this PR

Indeed, I think visual regression tests are needed for this kind of change.

@martinRenou
Copy link
Member Author

Update galata references

@martinRenou
Copy link
Member Author

@martinRenou
Copy link
Member Author

Update galata references

@martinRenou
Copy link
Member Author

martinRenou commented Feb 11, 2022

Added support for query parameters on the TreeHandler, it will keep the query when changing directory and rendering Notebooks:

query.mp4

I needed this to be able to test the theme handling in the Tree view, and I guess it's a nice addition?

@martinRenou martinRenou force-pushed the lab_theme_handling branch 2 times, most recently from 223d6d6 to 46d0263 Compare February 11, 2022 10:43
@martinRenou
Copy link
Member Author

Update galata references

@martinRenou martinRenou mentioned this pull request Feb 11, 2022
@martinRenou
Copy link
Member Author

triggering CI

@martinRenou martinRenou reopened this Feb 11, 2022
@martinRenou martinRenou marked this pull request as ready for review February 11, 2022 11:18
@github-actions
Copy link
Contributor

github-actions bot commented Feb 11, 2022

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 ipyvolume.ipynb multiple_widgets.ipynb query-strings.ipynb reveal.ipynb
Render
chromium
actual 69 <- [71 - 79 - 95] -> 136 164 <- [192 - 238 - 332] -> 604 62 <- [64 - 71 - 79] -> 96 66 <- [69 - 75 - 85] -> 108 2447 <- [2502 - 2570 - 2724] -> 3558 2503 <- [2542 - 2556 - 2662] -> 2934 2545 <- [2552 - 2553 - 2563] -> 2893 2969 <- [3040 - 3051 - 3199] -> 3404 1888 <- [1944 - 1956 - 1994] -> 2236 3674 <- [3717 - 3719 - 3817] -> 4132 7377 <- [9217 - 9258 - 9344] -> 9571 9224 <- [9255 - 9314 - 9426] -> 9681 1440 <- [1516 - 1599 - 1766] -> 2039 2535 <- [2536 - 2553 - 2606] -> 2933
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	2022-02-11 13:38:58.373779974 +0000
+++ /dev/fd/62	2022-02-11 13:38:58.373779974 +0000
@@ -4,37 +4,37 @@
     "BENCHMARK_REFERENCE": "actual"
   },
   "browsers": {
-    "chromium": "97.0.4666.0"
+    "chromium": "94.0.4595.0"
   },
   "systemInformation": {
     "cpu": {
-      "brand": "Xeon® Platinum 8272CL",
+      "brand": "Xeon® E5-2673 v3",
       "cache": {
         "l1d": 65536,
         "l1i": 65536,
-        "l2": 2097152,
-        "l3": 36700160
+        "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 mpx 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": "85",
+      "model": "63",
       "physicalCores": 2,
       "processors": 1,
       "revision": "",
       "socket": "",
-      "speed": 2.6,
+      "speed": 2.4,
       "speedMax": null,
       "speedMin": null,
-      "stepping": "7",
+      "stepping": "2",
       "vendor": "GenuineIntel",
       "virtualization": false,
       "voltage": ""
     },
     "mem": {
-      "total": 7284850688
+      "total": 7291699200
     },
     "osInfo": {
       "arch": "x64",
@@ -42,11 +42,11 @@
       "codename": "Focal Fossa",
       "codepage": "UTF-8",
       "distro": "Ubuntu",
-      "kernel": "5.11.0-1028-azure",
+      "kernel": "5.8.0-1040-azure",
       "logofile": "ubuntu",
       "platform": "linux",
       "release": "20.04.3 LTS",
-      "serial": "1b14fffe90ff4198b51ddebf6333f628",
+      "serial": "cfc067bfcb844f35865e279a1b0e66c5",
       "servicepack": "",
       "uefi": false
     }

@martinRenou martinRenou marked this pull request as ready for review February 11, 2022 12:40
ui-tests/tests/voila.test.ts Outdated Show resolved Hide resolved
@@ -76,7 +87,8 @@ def allowed_content(content):
contents=contents,
terminals_available=False,
server_root=get_server_root_dir(self.settings),
theme=self.voila_configuration.theme,
theme=theme_arg,
query=self.request.query,
Copy link
Member

@trungleduc trungleduc Feb 11, 2022

Choose a reason for hiding this comment

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

I'm not sure if we should pass all queries of the tree page to the notebook URLs, maybe just the theme and template?

Copy link
Member Author

Choose a reason for hiding this comment

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

As of this PR, that's the only two query arguments we support, or do we support more arguments?

Copy link
Member

@trungleduc trungleduc Feb 11, 2022

Choose a reason for hiding this comment

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

self.request.query contains all the queries passed to the tree handler, and then it is appended to the notebook URLs. I'm thinking about a more limited scope where only voila-theme and voila-template are transferred to the URL

Updated: it might be nicer to pass all queries, it'll allow providing configurations for all notebooks.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think your original comment will make sense if we add some query arguments specific to the TreeHandler at some point. But we can probably filter the query arguments here only when that happens?

Copy link
Member

Choose a reason for hiding this comment

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

If Voila is served under proxy, there might be some sort of token query for the tree handler, but I think there is no harm in transferring these tokens to the notebook URLs. We can filter the query when we actually use them in the tree handler.

@trungleduc
Copy link
Member

Thanks @martinRenou

@trungleduc trungleduc merged commit 6be66cc into voila-dashboards:main Feb 14, 2022
@martinRenou martinRenou deleted the lab_theme_handling branch February 15, 2022 10:59
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.

None yet

3 participants