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

Do not clean up kernel resources after execution #1334

Merged
merged 2 commits into from
Jun 28, 2023

Conversation

martinRenou
Copy link
Member

Revert #969

This should never have been merged. The client still needs the kernel and channels running after execution !

When using a kernel provisionner, this would simply kill the kernel

@github-actions
Copy link
Contributor

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

@martinRenou martinRenou added the bug Something isn't working label Jun 22, 2023
Copy link
Member

@davidbrochart davidbrochart left a comment

Choose a reason for hiding this comment

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

Thanks for making me a co-author!

@martinRenou
Copy link
Member Author

@meeseeksdev please backport to 0.4.x

@martinRenou
Copy link
Member Author

@meeseeksdev please backport to 0.3.x

meeseeksmachine pushed a commit to meeseeksmachine/voila that referenced this pull request Jun 22, 2023
meeseeksmachine pushed a commit to meeseeksmachine/voila that referenced this pull request Jun 22, 2023
martinRenou added a commit that referenced this pull request Jun 22, 2023
…ecution) (#1336)

* Backport PR #1334: Do not clean up resources after execution

* Try getting some CI green

* Iterate

---------

Co-authored-by: martinRenou <martin.renou@gmail.com>
@bsyouness
Copy link

Given that you're reverting a solution to this issue: #896, don't we think it's gonna come back?

@martinRenou
Copy link
Member Author

It's possible that self.executor.kc.stop_channels may still be needed for #896 and not harmful in the case of using a kernel provisionner. Can you confirm on your setup @davidbrochart ?

@davidbrochart
Copy link
Member

I checked that self.executor.kc.stop_channels is not harmful for kernel provisioners. It just disconnects the kernel client, but the kernel is kept alive.

@martinRenou
Copy link
Member Author

It makes sense, cleaning up the kernel client in the Voila executor will help reducing the number of threads opened. We don't need the kernel client after execution because we won't execute more code

@martinRenou
Copy link
Member Author

I'm working on getting the 0.3.x branch to build, then will release a new patch for 0.3.x and 0.4.x. Will continue this PR after that.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 23, 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 107 <- [121 - 142 - 183] -> 299 70 <- [75 - 83 - 95] -> 117 69 <- [72 - 78 - 88] -> 121 73 <- [76 - 80 - 97] -> 115 2708 <- [2819 - 2872 - 3090] -> 4172 2273 <- [2280 - 2294 - 2369] -> 2541 2588 <- [2606 - 2638 - 2802] -> 3248 2447 <- [2474 - 2484 - 2545] -> 2792 2068 <- [2129 - 2130 - 2160] -> 2341 2942 <- [2968 - 3015 - 3202] -> 3626 7505 <- [7561 - 7576 - 7610] -> 7656 3509 <- [3559 - 3591 - 3606] -> 3829 3833 <- [3836 - 3856 - 3873] -> 3874 1661 <- [1662 - 1673 - 1707] -> 2045 3456 <- [3676 - 4067 - 4759] -> 6596
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-06-27 13:46:31.217452067 +0000
+++ /dev/fd/62	2023-06-27 13:46:31.217452067 +0000
@@ -4,51 +4,49 @@
     "BENCHMARK_REFERENCE": "actual"
   },
   "browsers": {
-    "chromium": "112.0.5615.29"
+    "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,
-      "efficiencyCores": 0,
       "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",
-      "performanceCores": 2,
+      "manufacturer": "Intel®",
+      "model": "63",
       "physicalCores": 2,
       "processors": 1,
       "revision": "",
       "socket": "",
-      "speed": 2.6,
+      "speed": 2.4,
       "speedMax": null,
       "speedMin": null,
-      "stepping": "7",
-      "vendor": "Intel",
+      "stepping": "2",
+      "vendor": "GenuineIntel",
       "virtualization": false,
       "voltage": ""
     },
     "mem": {
-      "total": 7268679680
+      "total": 7291699200
     },
     "osInfo": {
       "arch": "x64",
       "build": "",
-      "codename": "Jammy Jellyfish",
+      "codename": "Focal Fossa",
       "codepage": "UTF-8",
       "distro": "Ubuntu",
-      "kernel": "5.15.0-1040-azure",
+      "kernel": "5.8.0-1040-azure",
       "logofile": "ubuntu",
       "platform": "linux",
-      "release": "22.04.2 LTS",
-      "serial": "b53da094229547778a51a873666147e4",
+      "release": "20.04.3 LTS",
+      "serial": "cfc067bfcb844f35865e279a1b0e66c5",
       "servicepack": "",
       "uefi": false
     }

@martinRenou martinRenou marked this pull request as draft June 26, 2023 07:41
martinRenou added a commit to meeseeksmachine/voila that referenced this pull request Jun 26, 2023
martinRenou added a commit that referenced this pull request Jun 26, 2023
…ecution) (#1335)

* Backport PR #1334: Do not clean up resources after execution

* Only clean kernel client

* Fix CI

* OpenSSL issue workaround

---------

Co-authored-by: martinRenou <martin.renou@gmail.com>
@martinRenou martinRenou marked this pull request as ready for review June 26, 2023 11:54
@martinRenou martinRenou changed the title Do not clean up resources after execution Do not clean up kernel resources after execution Jun 26, 2023
@bsyouness
Copy link

Question about the channels. Will the self.executor.kc.stop_channels affect the ability of the widgets to communicate with the kernel? If not, what channels are these that we need to force close?

@bsyouness
Copy link

Also, I'm guessing y'all are planning on releasing a patch for 0.3.x and 0.4.x after the build is complete?

@martinRenou
Copy link
Member Author

martinRenou commented Jun 26, 2023

Will the self.executor.kc.stop_channels affect the ability of the widgets to communicate with the kernel? If not, what channels are these that we need to force close?

With Voila there are basically two clients that communicate with the kernel:

  • the Voila "executor", responsible for executing the Notebook (here in the Python code self.executor.kc). This happens when you see the Voila loading spinner.
  • the Voila front-end, that allows rendered widgets to communicate with the kernel

So what we are cleaning here is the first one. After the notebook execution we don't need the executor anymore, so we close its communication channel.
This is why it helped solving #896 in the first place.

Also, I'm guessing y'all are planning on releasing a patch for 0.3.x and 0.4.x after the build is complete?

0.3.7 and 0.4.1 are out with this change already :)

martinRenou and others added 2 commits June 26, 2023 16:10
Revert voila-dashboards#969

This should never have been merged. The client still needs the kernel
and channels running after execution !

Co-authored-by: davidbrochart <davidbrochart@users.noreply.github.com>
@martinRenou
Copy link
Member Author

Trigger CI

@martinRenou martinRenou reopened this Jun 27, 2023
@martinRenou martinRenou merged commit 8ee8aeb into voila-dashboards:main Jun 28, 2023
@martinRenou martinRenou deleted the do_not_cleanup branch June 28, 2023 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants