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

Fix default VOILA_WS_BASE_URL value in preheating mode #1141

Merged

Conversation

vkaidalov-rft
Copy link

References

Resolves #1140.

Code changes

Use VOILA_BASE_URL as the default value if VOILA_WS_BASE_URL has not been specified to make the wait_for_request and get_query_string functions backwards-compatible in the preheating mode.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 8, 2022

Binder 👈 Launch a Binder on branch vkaidalov-rft/voila/fix-default-ws-base-url

@github-actions
Copy link
Contributor

github-actions bot commented Apr 8, 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 98 <- [99 - 108 - 124] -> 176 91 <- [96 - 97 - 110] -> 143 95 <- [100 - 102 - 115] -> 147 83 <- [86 - 93 - 100] -> 128 2998 <- [3066 - 3152 - 3327] -> 5389 2942 <- [2990 - 3032 - 3052] -> 3480 3117 <- [3168 - 3177 - 3337] -> 3516 3254 <- [3256 - 3256 - 3426] -> 3649 2277 <- [2300 - 2347 - 2536] -> 2632 4442 <- [4537 - 4645 - 4938] -> 5365 9816 <- [10937 - 11354 - 11388] -> 11411 8354 <- [8385 - 8467 - 8479] -> 8494 1725 <- [1767 - 1768 - 1875] -> 2066 2929 <- [2978 - 2987 - 3041] -> 3548
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-04-11 10:11:36.482331025 +0000
+++ /dev/fd/62	2022-04-11 10:11:36.482331025 +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": 7283847168
+      "total": 7291699200
     },
     "osInfo": {
       "arch": "x64",
@@ -42,11 +42,11 @@
       "codename": "Focal Fossa",
       "codepage": "UTF-8",
       "distro": "Ubuntu",
-      "kernel": "5.13.0-1021-azure",
+      "kernel": "5.8.0-1040-azure",
       "logofile": "ubuntu",
       "platform": "linux",
-      "release": "20.04.4 LTS",
-      "serial": "2f7f553bd7b042d1bf1c5b62ae644666",
+      "release": "20.04.3 LTS",
+      "serial": "cfc067bfcb844f35865e279a1b0e66c5",
       "servicepack": "",
       "uefi": false
     }

@trungleduc trungleduc added the bug Something isn't working label Apr 11, 2022
@trungleduc trungleduc added this to the 0.3.x milestone Apr 11, 2022
@trungleduc
Copy link
Member

Thanks @vkaidalov-rft for your PR!
I want to add some tests for this fix to prevent any future breaking changes. Can i push directly these tests to your branch?

@vkaidalov-rft
Copy link
Author

Hi @trungleduc, sure!
Is there anything I should do to let you push commits directly into my branch?

@vkaidalov-rft
Copy link
Author

vkaidalov-rft commented Apr 11, 2022

Hi @trungleduc, sure! Is there anything I should do to let you push commits directly into my branch?

I can see I've "allowed edits by maintainers" for this PR, so I suppose nothing else is needed. I haven't used this feature yet 😄

@trungleduc
Copy link
Member

trungleduc commented Apr 11, 2022

Thanks! I will ping you when I push my commits

@trungleduc
Copy link
Member

@vkaidalov-rft I updated your fix since the default WebSocket handler is at server_url and not base_url (

voila/voila/app.py

Lines 500 to 502 in 9afaaf0

url_path_join(self.server_url, r'/voila/query/%s' % _kernel_id_regex),
RequestInfoSocketHandler
)
)

And server_url defaults to base_url if it is not set

self.server_url = self.server_url or self.base_url

So it will work as expected.

@vkaidalov-rft
Copy link
Author

Thank you, @trungleduc! I've checked your changes and can confirm everything still works as expected 👍

@trungleduc
Copy link
Member

@martinRenou can you take a look at this PR as I contributed quite extensively on this one?

Copy link
Member

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

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

Thanks!

@martinRenou martinRenou merged commit de712ea into voila-dashboards:main Apr 20, 2022
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.

Kernels don't get the query string if --Voila.base_url is specified and preheating is turned on
3 participants