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

Refactor template system #208

Merged

Conversation

maartenbreddels
Copy link
Member

cc @t-makaro @MSeal
This is a proof of concept of the template system for both voila and nbconvert.

voila's current layout is:

Voila setup:
 * $prefix
   * jupyter
     * voila
       * templates
           * default
               * nbconvert_templates
                   * voila.tpl
              * templates
                  * tree.html
               * static
                   * voila.js
           * mycustom
               * nbconvert_templates
                   * voila.tpl

the new layout I propose is:

 * $prefix
   * jupyter
       * templates
         * voila
           * default
               * index.html
               * tree.html
               * resources
                   * voila.js
           * mycustom
               * index.html
                # example file content
               {%- extends 'nbconvert/default/index.html' -%}
               ... custom stuff    ...
               <script>
                    {% include "resources/voila.js" %}
                    {% include "default/resources/voila.js" %} # the same
                    {% include "voila/default/resources/voila.js" %} # the same
                    </script>
         * nbconvert
           * default
               * index.html
               * index.tex

The search path for jinja2 will be (when --template=mycustom:

    * voila
        * $prefix/share/jupyter/templates/
        * $prefix/share/jupyter/templates/voila
        * $prefix/share/jupyter/templates/voila/mycustom
        * $prefix/share/jupyter/templates/voila/default
        * $prefix/share/jupyter/templates/nbconvert
        * $prefix/share/jupyter/templates/nbconvert/default

Breaking changes

  • $prefix/jupyter/voila/templates/<name>/nbconvert_templates/voila.tpl to -> $prefix/share/jupyter/templates/voila/<name>/index.html
  • $prefix/jupyter/voila/templates/<name>/static to -> $prefix/share/jupyter/templates/voila/<name>/resources
  • Voila.nbconvert_template_paths and Voila.server_template_paths becomes Voila.template_paths

Cons

There can be confusion about the variables available for the templates, index.html for instance goes through nbconvert, while tree.html goes through tornado.

Pros

  • Including the directories above avoid 'shadowing' filenames and allows extending templates with the same name from a different directory.
  • This hardly needs documentation, the directory structure maps intuitive to the jijna2 loader / extends expression.
  • The file extension .html (and .tex) gives syntax highlighting. @MSeal suggested using .j2.html that some editor/extensions support to give jinja2 syntax highlighting.
  • We can choose to write an nbconvert template instead of a voila template so we can also generate static html files.
  • Having the resources directory as subdirectory allows easy file including, voila will serve the directory as static files
  • Having tree.html and index.html in the same directory makes it easier to use the same style for both.

TODO

  • write the changes in a changelog
  • add tests
  • I did not rename base.tpl, lab.tpl, not sure if that is needed at this moment

@maartenbreddels maartenbreddels force-pushed the refactor_templates branch 3 times, most recently from a05cb6c to 44b0402 Compare June 7, 2019 18:10
.travis.yml Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@vidartf
Copy link
Contributor

vidartf commented Jun 17, 2019

Including the directories above avoid 'shadowing' filenames and allows extending templates with the same name from a different directory.

Trying to come up with a case where this might break:

* $prefix
  * jupyter
    * templates
      * alice
        * default
          * index.html
      * bob
        * default
          * index.html
        * mycustom
          * index.html
             # file content
             {%- extends 'default/index.html' -%}

Which file will default/index.html in mycustom resolve to? The intuitive expectation is bob/default/index.html, since the small number of prefixes indicate a preference for a local file. However, the way the search paths are listed, I think it will resolve to alice/default/index.html.

* $prefix
  * jupyter
    * templates
      * alice
        * default
          * some.js
      * bob
        * default
          * some.js
        * mycustom
          * index.html
             # file content
             {% include "default/some.js" %}

And what about the JS file? Should it resolve to the alice or bob version?

  • Case i: The user wants the JS in alice to replace the JS in bob.
  • Case ii: The user wants to use the JS in bob, but by accident, the files were named the same in alice and bob (e.g. index.js, bundle.js, dashboard.js, etc.)

@vidartf
Copy link
Contributor

vidartf commented Jun 17, 2019

Just to clarify example 1: I think the resolution here is determined by the OS sorting of the search paths.

@martinRenou
Copy link
Member

cc. @maartenbreddels @jtpio

I tried this refactored version of the template system, see https://github.com/martinRenou/voila-material/pull/1. It seems to work perfectly, apart from the browser-open.html file, it does not seem to use my file, but the one from the default template.

@maartenbreddels
Copy link
Member Author

Which file will default/index.html in mycustom resolve to? The intuitive expectation is bob/default/index.html

Indeed, but it's determined by the application, for instance, in voila we call:

collect_paths(['voila', 'nbconvert'], 'foo')

For template 'foo', such that voila will 'override' nbconvert.

In your case, the apllication 'bob' would call:

collect_paths(['bob', 'alice'], 'mycustom')

Does that make sense?

@maartenbreddels
Copy link
Member Author

  • Case ii: The user wants to use the JS in bob, but by accident, the files were named the same in alice and bob (e.g. index.js, bundle.js, dashboard.js, etc.)

It should go to the bob/default/some.js file since the bob directory has higher precedence than alice (assuming my previous comment/code).

We had a talk about it this afternoon, I think the behavior we want is very intuitive, so it behaves as you expect, still, it is very difficult to explain (which I don't like).

voila/paths.py Outdated Show resolved Hide resolved
@SylvainCorlay
Copy link
Member

I still have issues wit this design:

  1. This sorts of breaks the pattern PREFIX/share/jupyter/package_name/:

with this proposal, both nbconvert and voila will add "templates" in PREFIX/share/jupyter/templates/, but what a template means is different with each package.

  • In the case of nbconvert template, a template is (so far) only a jinja template. It may be for HTML or for other formats used by other TemplateExporter subclasses that have nothing to do with web frontends.

    I do think that we will eventually want to have nbconvert template be directories and include other resources, but in a way that is not web-specific.

  • In the case of voila, a template may include nbconvert HTML templates, tornado templates, static resources, etc.

    Note: I would like voila templates to have the ability to include JSON configuration files to be loaded by traitlets configurables classes such as the HTML header preprocessors.

  1. I don't think that we should have tornado and nbconvert templates live in the same directory.

Both are jinja (and I agree they should all use the .html extension), but they are not rendered at the same point and don't have access to the same python variables. Placing them in the same directory may be confusing for voila template authors.

@SylvainCorlay
Copy link
Member

SylvainCorlay commented Jun 20, 2019

Another proposal could be

  1. Regarding convergence between voila and nbconvert templates
  • I would really like to see nbconvert adopt a configuration directory approach for the TemplateExporters. It would be a great improvement over the current situation.
  • I would like an "nbconvert template" to actually by a directory, which may include the jinja template per se as well as additional resources such as JSON configuration files to be loaded by the traitlets machinery so that we can associate a template with e.g. a specified implementation of a preprocessor.

But beyond that, we should probably not try to make voila and nbconvert converge on this.

  1. Once nbconvert has a PREFIX/share/jupyter/nbconvert/templates/ containing a directory per template,

Then we could modify the voila template to look like

PREFIX/share/jupyter/voila/templates/template_name/
|
├── conf.json          # Template configuration file
├── nbconvert/         # This directory IS an nbconvert template with this new pattern
├── static/            # Static directory
└── tornado/           # Custom tornado templates
  1. How to turn a pure nbconvert template into a voila template?

A way to do this could be to allow users to not provide a nbconvert directory in their voila template, and merely specify the name of an nbconvert template to use as a base. The augmented voila.html / index.html including the javascript could be generated automatically.

@maartenbreddels
Copy link
Member Author

I don't think that we should have tornado and nbconvert templates live in the same directory.

I think we should, it hurts my puristic me, but practical me wins here. I think 90% of the people just want to modify existing templates, and will not care nor understand the difference. Most people will fiddle with the html, and will just modify tree.html and index.html to look similar, and skim over the jinja stuff.

I think this setup will actually allow static html (nbconvert) and voila templates to be very similar. For instance if we put the file 'style.css' at PREFIX/share/jupyter/templates/nbconvert/foo/resources/style.css

We can do from PREFIX/share/jupyter/templates/nbconvert/foo/index.html

....
<% block css %>
<style>
{% include "resources/style.css" %}
<style>
<% endblock css %>
....

And make it voila compatible by putting in filePREFIX/share/jupyter/templates/voila/foo/resources/index.html

<% extend nbconvert/foo/index.html %>
<% block css %>
<style src="static/style.css" %}
<style>
<% endblock css %>

And make voila server the resources directory under static (which I think we should rename to resources to make it all simpler/consistent)

This way we can have many templates that inline the css/js to have a single html file, while voila serves them, with minimal pain.

It does require clear documentation on which template has which variables available, but I think the convenience wins here.

@SylvainCorlay
Copy link
Member

I think we should, it hurts my puristic me, but practical me wins here.

Even ourselves got confused about it in the early days of voila when all was in the same directory.

I am not really sure this change really makes things more convenient.

How about a dedicated discussion about this tomorrow? I may have another approach that may reconcile us.

@maartenbreddels
Copy link
Member Author

NOTE: these comments were written after a long live discussion, but still submitted for a complete history of the train of thought.

with this proposal, both nbconvert and voila will add "templates" in PREFIX/share/jupyter/templates/, but what a template means is different with each package.

that may be, but most templates are really similar, at least the resources they use, and HTML structure they would use. Having them close together (same directory) makes sense (for a user/template-writer at least).

Even ourselves got confused about it in the early days of voila when all was in the same directory.

I'm not sure we were confused, we did realize that indeed one template goes through jinja via nbconvert, while others go though jinja via tornado.

At the end of the day we still need to document which template has which variables available, I don't think it will help by putting the templates in different directories (it does not add information). I think no template designer really cares about who passes it to jinja, they will only care that it is a jinja template, and which variables/extension etc are available, and in all cases, we need to document that.

I think we agree that we want to have templates that use sth like this:

{%- extends 'default/index.html' -%}
{%- extends 'bar/index.html' -%}
{%- extends 'nbconvert/foo/index.html' -%}

If we have a directory structure that maps to this, it will be very intuitive, and most people wouldn't need to read the documentation.

However, if we could decide to have a directory structure as you propose. e.g.

  • PREFIX/share/jupyter/nbconvert/templates/bar/tornado/index.html
  • PREFIX/share/jupyter/voila/templates/foo/nbconvert/index.html

we would have the following issues:

  • The extend path will be very long/ugly, e.g. {%- extends 'nbconvert/templates/foo/nbconvert/index.html' -%} instead of {%- extends 'nbconvert/foo/index.html' -%}
  • To avoid this we need a custom jijna Loader to do the file mapping. I'm afraid that will not be understood unless people read the documentation. I feel this idea is hard to explain, and people would rather not read docs.
  • open-browser.html, tree.html and voila.html/index.html will be 95% the same for a fully customized template if they do not share the same directory, where should the common templates/snippets go?.

How about a dedicated discussion about this tomorrow? I may have another approach that may reconcile us.

Yes, I'd really like to have best user experience solution, but without making any design mistakes.

@maartenbreddels
Copy link
Member Author

After a long discussion, we found some middle ground:

 * $prefix
   * jupyter
       * voila
         * templates
           * default
               * index.html
               * tree.html
               * resources
                   * voila.js
           * mycustom
               * index.html
                # example file content
               {%- extends 'nbconvert/templates/default/index.html' -%}
               ... custom stuff    ...
               <script>
                    {% include "resources/voila.js" %}
                    {% include "default/resources/voila.js" %} # the same
                    {% include "voila/templates/default/resources/voila.js" %} # the same
                    </script>
      * nbconvert
         * templates
           * default
               * index.html
               * index.tex

Basically, the rename is:

  • templates/voila -> voila/templates
  • templates/nbconvert -> nbconvert/templates

The reasoning behind it being that these are application specific directories, and we are not claiming to have a 'Jupyter template system'.

It means the extends path will be a bit longer, e.g. nbconvert/templates/default/index.html instead of nbconvert/templates/default/index.html. The upside is that we can also have a skeleton directory in the nbconvert directory, which would be a natural place for for instance null.tpl.

voila/paths.py Show resolved Hide resolved
@maartenbreddels maartenbreddels force-pushed the refactor_templates branch 2 times, most recently from 7fe3aa9 to e76fdb9 Compare November 28, 2019 14:18
@maartenbreddels maartenbreddels changed the title WIP: Refactor template system Refactor template system Nov 29, 2019
@maartenbreddels maartenbreddels force-pushed the refactor_templates branch 3 times, most recently from e676e5b to 4123c82 Compare November 29, 2019 15:49
@maartenbreddels
Copy link
Member Author

The classic template does not render widgets correctly with voila, while it does with nbconvert. The reason is that with nbconvert we get the widgets from the CDN, which include all css variables. If we use voila, we rely on voila.js, which does not include all css variables. A possible solution could be to generate two bundles.

@voila-dashboards voila-dashboards deleted a comment from martinRenou May 26, 2020
@SylvainCorlay SylvainCorlay force-pushed the refactor_templates branch 4 times, most recently from e7eeec1 to af608ae Compare May 27, 2020 09:27
@SylvainCorlay SylvainCorlay merged commit db56398 into voila-dashboards:master May 27, 2020
@SylvainCorlay
Copy link
Member

giphy

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.

6 participants