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

Install function for custom templates, template docs update #1250

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bmorris3
Copy link

@bmorris3 bmorris3 commented Nov 2, 2022

References

This PR addresses #827, as well as spacetelescope/jdaviz#1729.

Code changes

This PR introduces a convenience function for installing custom templates, called install_custom_template. A user specifies a path to a custom template's directory structure and the name of the custom template. The custom template is then symlinked (preferred) or copied (fallback option) to the same directory where the voila base template is located. By default, this function does not overwrite existing templates. Also by default, install_custom_template will ignore any share/ dirs in the current working directory (so if you run install_custom_template in your local voila repo, it won't install your custom template in the voila repo's share/ dir.

Background: this PR was motivated by work on spacetelescope/jdaviz#1729. jdaviz, among other tools, contains a custom voilà template which gets installed with the package. We had to carefully symlink or copy the template files from within our setup.py in order to make the jdaviz custom template available for voilà. In spacetelescope/jdaviz#1792 I developed the functions in this PR, and we realized they might belong upstream in voilà. If this PR is merged, it supersedes spacetelescope/jdaviz#1792.

User-facing changes

I've edited the custom template docs page, mostly by removing out-of-date content and mentioning the new function.

Backwards-incompatible changes

None.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

Binder 👈 Launch a Binder on branch bmorris3/voila/custom-templates

@bmorris3
Copy link
Author

bmorris3 commented Nov 2, 2022

I wasn't sure if this can/should be tested, so I simply tested it locally on the jdaviz custom template by running:

from voila.paths import install_custom_template

install_custom_template('../jdaviz', 'jdaviz-default', overwrite=True)

I can confirm that the jdaviz template directories get symlinked within the correct directories, alongside voilà's "base" template dirs.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 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 multiple_widgets.ipynb query-strings.ipynb reveal.ipynb
Render
chromium
actual 99 <- [111 - 130 - 171] -> 290 69 <- [73 - 83 - 98] -> 140 99 <- [110 - 128 - 165] -> 265 70 <- [74 - 81 - 95] -> 133 2704 <- [2789 - 2858 - 2933] -> 3978 2536 <- [2556 - 2608 - 2665] -> 2740 2761 <- [2782 - 2805 - 2905] -> 3145 2806 <- [2858 - 2979 - 3161] -> 3330 2088 <- [2105 - 2129 - 2242] -> 2286 3393 <- [3418 - 3449 - 3605] -> 3875 6772 <- [6807 - 6811 - 6830] -> 7052 1719 <- [1786 - 1787 - 1823] -> 1832 2826 <- [2882 - 2999 - 3157] -> 3721
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-11-02 16:47:51.411622478 +0000
+++ /dev/fd/62	2022-11-02 16:47:51.411622478 +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 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": 7281311744
+      "total": 7291699200
     },
     "osInfo": {
       "arch": "x64",
@@ -42,11 +42,11 @@
       "codename": "Focal Fossa",
       "codepage": "UTF-8",
       "distro": "Ubuntu",
-      "kernel": "5.15.0-1022-azure",
+      "kernel": "5.8.0-1040-azure",
       "logofile": "ubuntu",
       "platform": "linux",
-      "release": "20.04.5 LTS",
-      "serial": "1cb717add1594749b790a0e303368aa5",
+      "release": "20.04.3 LTS",
+      "serial": "cfc067bfcb844f35865e279a1b0e66c5",
       "servicepack": "",
       "uefi": false
     }

@pllim
Copy link
Contributor

pllim commented Nov 3, 2022

@maartenbreddels
Copy link
Member

Hi @bmorris3 ,

the PR here looks much larger than what was needed in https://github.com/spacetelescope/jdaviz/blob/main/setup.py#L123-L164 , could you describe the large discrepancy ?

Also, I am not sure why moving from setuptools to hatch doesn't allow for the current approach, I believe hatch has a good plugin system that allows custom code to be executed, like what we currently do in setup.py.

Also, the reason for the change in documentation is not clear to me, I see a large portion of documentation being replaced by documentation that doesn't seem to replace it.

Regards,

Maarten

@bmorris3
Copy link
Author

bmorris3 commented Nov 4, 2022

Hi @maartenbreddels,

the PR here looks much larger than what was needed in https://github.com/spacetelescope/jdaviz/blob/main/setup.py#L123-L164 , could you describe the large discrepancy ?

If you compare jdaviz/setup.py:DevelopCmd.run with the proposed function voila.paths.install_custom_template in this PR, you'll see the contents are quite similar. The differences are:

  • The jdaviz implementation is specific to the jdaviz-default template. The implementation in this voila PR is generic
  • The voila PR has a detailed docstring, and comments
  • The voila PR also adds a function called get_existing_template_dirs which is handy for identifying the available voila templates in a given environment.

Also, I am not sure why moving from setuptools to hatch doesn't allow for the current approach, I believe hatch has a good plugin system that allows custom code to be executed, like what we currently do in setup.py.

This PR is somewhat independent from the setuptools/hatch conversation in spacetelescope/jdaviz#1729. In the related PR spacetelescope/jdaviz#1792, I outlined that the only convenient way for a developer to "install" a custom voila template is to pip install the package containing the custom template. But the custom template will not be copied/symlinked when a developer uses editable install mode. Most active devs work with editable installs, so the current dev install would look like:

cd jdaviz
pip install .  # "install" the voila template
pip uninstall jdaviz  # uninstall the static jdaviz installation
pip install -e .  # install the editable package

There's no convenient way to re-install further revisions to the custom template without repeating this process. (More details and discussion are over in that jdaviz PR.)

Also, the reason for the change in documentation is not clear to me, I see a large portion of documentation being replaced by documentation that doesn't seem to replace it.

As pointed out in #827, it seems that the template docs are out of date. The existing documentation was contributed several years ago in #126. If you look at the file/directory structure explained in the voila docs, you'll see it's out of sync with the structure in the voila source code. I've spent some time attempting the git archaeology to sort out how/when this happened, but I haven't found the PR.

In this PR, I've simply removed the section on custom templates, which is no longer accurate. I would contribute corrected docs in their place, but I don't think I am qualified to write out what voila expects.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants