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

reenable prelaunch-hook #724

Merged
merged 19 commits into from Jul 25, 2022
Merged

reenable prelaunch-hook #724

merged 19 commits into from Jul 25, 2022

Conversation

timkpaine
Copy link
Member

@timkpaine timkpaine commented Sep 25, 2020

A function that is called prior to the launch of a new kernel instance
when a user visits the voila webpage. Used for custom user authorization
or any other necessary pre-launch functions.

Should be of the form

def hook(req: tornado.web.RequestHandler,
                notebook: nbformat.NotebookNode,
                cwd: str)

Although most customizations can leverage templates, if you need access
to the request object (e.g. to inspect cookies for authentication),
or to modify the notebook itself (e.g. to inject some custom structure,
althought much of this can be done by interacting with the kernel
in javascript) the prelaunch hook lets you do that.

@mlucool
Copy link

mlucool commented Nov 20, 2020

This is a great feature. Can maintainers of this repo comment on if this can be accepted or maybe offer alternative guidance for handling the issue @timkpaine has raised?

@vvatsa
Copy link

vvatsa commented Sep 14, 2021

Would echo the previous sentiment, I too would appreciate if the maintainers can comment on Tims' patch.

@trungleduc
Copy link
Member

@jtpio I think this a a good feature, should we consider it for 0.4.0 ?

@angusfong
Copy link

Echoing the above two comments. Please advise on how I can pass a single string variable to my notebook. I am using voila.app.Voila.start() to create the kernel, per #218 (comment).

@github-actions
Copy link
Contributor

github-actions bot commented Nov 15, 2021

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 116 <- [131 - 156 - 195] -> 309 85 <- [88 - 95 - 114] -> 157 101 <- [108 - 121 - 144] -> 213 90 <- [93 - 101 - 119] -> 150 2739 <- [2838 - 2890 - 2972] -> 3983 2767 <- [2807 - 2871 - 2996] -> 3354 3102 <- [3222 - 3351 - 3462] -> 3735 3000 <- [3098 - 3110 - 3288] -> 3547 2101 <- [2116 - 2132 - 2169] -> 2304 4457 <- [4624 - 4665 - 4926] -> 6122 9230 <- [11418 - 11459 - 12202] -> 12355 7890 <- [7903 - 7933 - 7938] -> 7968 1385 <- [1636 - 1717 - 1733] -> 1753 12002 <- [14350 - 18155 - 25708] -> 48018
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-07-25 13:04:09.018022201 +0000
+++ /dev/fd/62	2022-07-25 13:04:09.018022201 +0000
@@ -4,37 +4,37 @@
     "BENCHMARK_REFERENCE": "actual"
   },
   "browsers": {
-    "chromium": "97.0.4666.0"
+    "chromium": "94.0.4595.0"
   },
   "systemInformation": {
     "cpu": {
-      "brand": "Xeon® Platinum 8171M",
+      "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 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": "4",
+      "stepping": "2",
       "vendor": "GenuineIntel",
       "virtualization": false,
       "voltage": ""
     },
     "mem": {
-      "total": 7281008640
+      "total": 7291699200
     },
     "osInfo": {
       "arch": "x64",
@@ -42,11 +42,11 @@
       "codename": "Focal Fossa",
       "codepage": "UTF-8",
       "distro": "Ubuntu",
-      "kernel": "5.15.0-1014-azure",
+      "kernel": "5.8.0-1040-azure",
       "logofile": "ubuntu",
       "platform": "linux",
-      "release": "20.04.4 LTS",
-      "serial": "2dc2d73cec5c409b97a391e53a41a4ba",
+      "release": "20.04.3 LTS",
+      "serial": "cfc067bfcb844f35865e279a1b0e66c5",
       "servicepack": "",
       "uefi": false
     }

@timkpaine
Copy link
Member Author

timkpaine commented Nov 16, 2021

I am getting this back up and working, is there any specific examples/docs I should add? I no longer have access to how I used this to enable custom token-based authentication at JP Morgan (maybe @vidartf can construct some examples off this) but im planning on adding a simple test to extract a query parameters and run papermill so I can close #484

@trungleduc trungleduc added the enhancement New feature or request label Apr 24, 2022
@trungleduc trungleduc added this to the 0.4.0 milestone Apr 24, 2022
@alkasm
Copy link
Contributor

alkasm commented Apr 26, 2022

This PR would solve my needs for authentication in a very clean way. If I want to support auth via authorization token or cookies, I could simply read them via this hook and then mutate a notebook cell to include the authorization information required to make additional client requests.

Edit: Oh, I guess that's exactly how papermill works. Neat.

.github/workflows/build.yml Outdated Show resolved Hide resolved
@alkasm
Copy link
Contributor

alkasm commented Apr 26, 2022

Well I put the minor fix in a PR here hoping to see the workflows run: #1148
but you need a maintainer to allow the workflows to run if you're a first-time contributor (sensible policy). When you have time, you can apply that commit in this PR or whatever makes the most sense.

@timkpaine
Copy link
Member Author

I will do it here tonight, apologies

@timkpaine
Copy link
Member Author

@alkasm I merged your branch into mine, as a weird side effect your PR and my PR are now both running the tests but I think they're the same

@alkasm
Copy link
Contributor

alkasm commented Apr 26, 2022

Awesome, and no worries on the timeline. I deleted my branch. Thanks for being so responsive on a relatively stale PR!

@timkpaine
Copy link
Member Author

Wanted to note that this is NOT a breaking change, so does not necessarily need to wait for 4.0.

@timkpaine
Copy link
Member Author

Yeah it looks like flaky tests, I tried 4-5 times (rebased and force pushed empty commits) and got different tests failing every time

@timkpaine
Copy link
Member Author

@trungleduc @jtpio @maartenbreddels @martinRenou @davidbrochart ready for review / discussion

@timkpaine
Copy link
Member Author

Just bumping, I can't sponsor this feature right now with my current employer but I can reveal that a large former employer, J.P. Morgan, uses/used this feature extensively which is a decent endorsement:

https://tim.paine.nyc/talks/jupytercon.html#/5/1/1

@trungleduc
Copy link
Member

Hi @timkpaine, thank you very much for your contribution! I'm quite busy during this period but I will take a look at this PR in a few weeks.
My plan is to target this PR to 0.4.0 in order to facilitate the tracking of new features then we can back-port it to 0.3.x

@alkasm
Copy link
Contributor

alkasm commented May 25, 2022

I've noticed that this only works when you serve a single notebook, e.g. by overriding the notebook_path argument on the voila.app.Voila class or equivalently by passing a positional arg to the voila CLI. If you try to serve a directory of notebooks (e.g. with root_dir on the Voila class or by launching the voila CLI with no positional args), none of the requests get intercepted: neither the request which serves the directory nor the /voila/render/<notebook>.ipynb links provided in the directory view.

edit: I think probably adding it to here is sufficient:

voila/voila/app.py

Lines 568 to 573 in e658bbe

VoilaHandler,
{
'template_paths': self.template_paths,
'config': self.config,
'voila_configuration': self.voila_configuration
}),

I'll test that out shortly

edit 2: Yeah, that works

docs/source/customize.rst Outdated Show resolved Hide resolved
docs/source/customize.rst Outdated Show resolved Hide resolved
docs/source/customize.rst Outdated Show resolved Hide resolved
docs/source/customize.rst Show resolved Hide resolved
voila/app.py Outdated Show resolved Hide resolved
@trungleduc
Copy link
Member

Thanks @timkpaine for your contribution, it looks really nice.
I have one question about the hook and preheated kernel mode. In this mode, the notebook is rendered only once, so the hook function is only called once and usually when no user is connected. So any hook that reads the request headers is not usable in preheated kernel mode. How do we inform users about the incompatibility of these two features?

@trungleduc
Copy link
Member

I've noticed that this only works when you serve a single notebook, e.g. by overriding the notebook_path argument on the voila.app.Voila class or equivalently by passing a positional arg to the voila CLI. If you try to serve a directory of notebooks (e.g. with root_dir on the Voila class or by launching the voila CLI with no positional args), none of the requests get intercepted: neither the request which serves the directory nor the /voila/render/<notebook>.ipynb links provided in the directory view.

edit: I think probably adding it to here is sufficient:

voila/voila/app.py

Lines 568 to 573 in e658bbe

VoilaHandler,
{
'template_paths': self.template_paths,
'config': self.config,
'voila_configuration': self.voila_configuration
}),

I'll test that out shortly

edit 2: Yeah, that works

I confirm, the prelaunch_hook does not work with the case of serving a directory.

@timkpaine
Copy link
Member Author

Thanks @timkpaine for your contribution, it looks really nice.
I have one question about the hook and preheated kernel mode. In this mode, the notebook is rendered only once, so the hook function is only called once and usually when no user is connected. So any hook that reads the request headers is not usable in preheated kernel mode. How do we inform users about the incompatibility of these two features?

That is correct, I can add a note to the documentation. I have long rallied against the preheat mode for this reason: #712 (comment)

@trungleduc
Copy link
Member

I think the documentation for this conflict is not enough, we might need some checks on startup and warn users if they use both features.

@trungleduc
Copy link
Member

Thanks @timkpaine!

@trungleduc trungleduc merged commit ec2c75f into voila-dashboards:main Jul 25, 2022
@timkpaine timkpaine deleted the prelaunch_hook branch July 25, 2022 15:37
@timkpaine
Copy link
Member Author

thanks @trungleduc ! cc @vidartf @ptomecek

@jtpio jtpio modified the milestones: 0.4.0, 0.3.x Jul 25, 2022
@jtpio
Copy link
Member

jtpio commented Jul 25, 2022

Just updated the milestone to 0.3.x.

According to the diff and the comment above. there doesn't seem to be breaking changes, so this can be shipped in the next 0.3.x release.

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

7 participants