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 file redirect error #1073

Merged
merged 3 commits into from
Jan 27, 2022
Merged

Conversation

trungleduc
Copy link
Member

References

Close #1071 #1049

Code changes

Correctly redirect voila/render/ to voila/files/ for non-notebook items.

@github-actions
Copy link
Contributor

Binder 👈 Launch a Binder on branch trungleduc/voila/fix-file-path

@trungleduc trungleduc added the bug Something isn't working label Jan 24, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jan 24, 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 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 2913 <- [2966 - 3166 - 3499] -> 4552 2457 <- [2468 - 2523 - 2582] -> 2750 2621 <- [2665 - 2745 - 2789] -> 2969 3134 <- [3163 - 3245 - 3273] -> 3487 1917 <- [1948 - 1951 - 1971] -> 2066 3974 <- [4141 - 4421 - 4948] -> 6476 6949 <- [8055 - 8390 - 8592] -> 8950 9805 <- [9810 - 9820 - 10011] -> 10557 1409 <- [1419 - 1423 - 1533] -> 1804 2332 <- [2348 - 2359 - 2397] -> 2532
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-01-27 09:44:19.690449716 +0000
+++ /dev/fd/62	2022-01-27 09:44:19.694449779 +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": 7289610240
+      "total": 7291699200
     },
     "osInfo": {
       "arch": "x64",
@@ -42,11 +42,11 @@
       "codename": "Focal Fossa",
       "codepage": "UTF-8",
       "distro": "Ubuntu",
-      "kernel": "5.11.0-1027-azure",
+      "kernel": "5.8.0-1040-azure",
       "logofile": "ubuntu",
       "platform": "linux",
       "release": "20.04.3 LTS",
-      "serial": "69b21c4f2dfd48689737bdb9de565c02",
+      "serial": "cfc067bfcb844f35865e279a1b0e66c5",
       "servicepack": "",
       "uefi": false
     }

voila/handler.py Outdated
await gen.initialize(template=template_arg, theme=theme_arg)
done = await gen.initialize(template=template_arg, theme=theme_arg)
if not done:
self.redirect_to_file(path)
Copy link
Member

@martinRenou martinRenou Jan 26, 2022

Choose a reason for hiding this comment

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

I am not sure I understand the PR, if the path was not a path to a Notebook file in the first place, maybe we should not use the NotebookRenderer class (line 119 of this file)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, it makes no sense to access non-notebook files through /voila/render/ URL. But in some corner cases, where users have IFrame or FileLink widgets in their notebooks, these widgets will detect the URL to the local files from the current URL of the notebook, which is /voila/render/...
So this PR will redirect these requests to the normal /voila/files/

Copy link
Member

Choose a reason for hiding this comment

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

I see. Could we detect this earlier in the code? Instead of trying to initialize a NotebookRenderer generator, we could probably redirect to the FileHandler somewhere around here?

I guess if the path does not end with .ipynb we are safe to try to redirect?

Copy link
Member Author

@trungleduc trungleduc Jan 26, 2022

Choose a reason for hiding this comment

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

Yes, we can check the file extension against the mapping VoilaConfiguration.extension_language_mapping.

Copy link
Member

Choose a reason for hiding this comment

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

Ok I see, thanks!

@trungleduc trungleduc marked this pull request as draft January 26, 2022 17:01
@trungleduc trungleduc force-pushed the fix-file-path branch 2 times, most recently from ad2a050 to 204c0b3 Compare January 27, 2022 08:19
@trungleduc trungleduc marked this pull request as ready for review January 27, 2022 08:20
voila/handler.py Outdated Show resolved Hide resolved
@martinRenou martinRenou merged commit 2eae88e into voila-dashboards:main Jan 27, 2022
@martinRenou
Copy link
Member

Thanks!

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.

Voila doesn't load static HTML files despite whitelisting
2 participants