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 to be easily extensible and voila compatible #1056

Merged
merged 1 commit into from
Oct 17, 2019

Conversation

maartenbreddels
Copy link
Collaborator

cc @SylvainCorlay @martinRenou @jtpio

This is the followup of what was discussed in #1028, planned for voila in voila-dashboards/voila#208.

The basic idea is that templates are a directory in either:
$prefix/nbconvert/templates/<template_name> where $prefix is determined by jupyter_paths.
On OSX it would be:

	/Users/maartenbreddels/Library/Jupyter/nbconvert/templates/default
	/Users/maartenbreddels/miniconda3/envs/voila/share/jupyter/nbconvert/templates/default
	/usr/local/share/jupyter/nbconvert/templates/default
	/usr/share/jupyter/nbconvert/templates/default

With this PR, #1005 would simply be a directory share/jupyter/nbconvert/templates/lab and now the old full.tpl is a template called 'notebook'.

In combination with voila-dashboards/voila#208 we can make all nbconvert template compatible by only overriding the {% block ipywidgets %}, and instead of inlining CSS, we can request it from the server.

Loose ends

  • the latex templates clutter the default directory a bit, maybe we can put these in different template directories. `
  • The meaning ofTemplateExporter.template_file changed a bit, it's cannot be an absolute path anymore, it now refers to the filename in the search path, resolved by jinja2 only. (also the reason unit tests fail).
  • We discussed using index.j2.html instead of filenames for templates, I think without the .j2 is looks cleaner.
  • Some systems may want to list template (eg voila), filtered by what kind of exporter is supported, we could list that in the conf.json file.
  • the html exporter should maybe use the notebook template by default, since now it's using the old 'basic.tpl' template.

@MSeal MSeal added this to the 6.0 milestone Jun 28, 2019
@MSeal MSeal requested review from t-makaro and MSeal June 28, 2019 16:56
@MSeal
Copy link
Contributor

MSeal commented Jun 28, 2019

@t-makaro should probably help review this as he's been more involved on the template side of things recently.

@MSeal
Copy link
Contributor

MSeal commented Jul 26, 2019

I need to do some testing this weekend to help make sure everything is in working order, but it looks like all the changes were what we discussed. I'm not sure if we agreed for this change to be in 6.0 release or in 5.x path, if you remember?

The test suite is quite angry and will need some attention for the new patterns.

@MSeal
Copy link
Contributor

MSeal commented Jul 26, 2019

the latex templates clutter the default directory a bit, maybe we can put these in different template directories. `

Can also be a follow-up PR to avoid making this any larger

@MSeal
Copy link
Contributor

MSeal commented Jul 26, 2019

Some systems may want to list template (eg voila), filtered by what kind of exporter is supported, we could list that in the conf.json file.

Probably needs a separate issue to discuss. We may want this to be a dynamic calculation.

@MSeal
Copy link
Contributor

MSeal commented Jul 26, 2019

the html exporter should maybe use the notebook template by default, since now it's using the old 'basic.tpl' template.

Need to look at the differences here, but I think there was consensus to have exports be the classic notebook format by default and lab format to be an opt-in pattern, if that's what's being referenced by this question.

@MSeal
Copy link
Contributor

MSeal commented Jul 28, 2019

I need to do some testing this weekend to help make sure everything is in working order, but it looks like all the changes were what we discussed. I'm not sure if we agreed for this change to be in 6.0 release or in 5.x path, if you remember?

The test suite is quite angry and will need some attention for the new patterns.

Missed the 6.0 tag on the PR makes sense. Let me know if you need any help with figuring out tests later on.

@willingc willingc added the status:work-in-process Do not merge label Jul 29, 2019
@maartenbreddels maartenbreddels changed the title POC: Refactor template system to be easily extensible and voila compatible Refactor template system to be easily extensible and voila compatible Jul 31, 2019
@maartenbreddels maartenbreddels changed the base branch from master to 6.x July 31, 2019 12:41
@maartenbreddels
Copy link
Collaborator Author

maartenbreddels commented Jul 31, 2019

I changed the target branch to 6.x, but CI does not run on this, can this be enabled? (works now)

Need to look at the differences here, but I think there was consensus to have exports be the classic notebook format by default and lab format to be an opt-in pattern, if that's what's being referenced by this question.

I've changed it, such that lab is a template: $ nbconvert --template lab ...

The old system of using a template file still exists, but now is available as --template-file=,

@maartenbreddels
Copy link
Collaborator Author

I've seem to have hit a bug in traitlets however, like voila, I want this to work:

$ voila     --template gridstack --theme dark basics.ipynb
$ nbconvert --template gridstack --theme dark --to html --execute basics.ipynb

Such that arguments --template and --theme have the same meaning.

However, it seems the HTMLExporters does not pick this up, the alias --theme gets translated to SlidesExporter.theme (!) (from a debug session):

(Pdb) self.config  # <-- this is HTMLExporter.config
{'NbConvertBase': {'display_data_priority': ['application/vnd.jupyter.widget-state+json', 'application/vnd.jupyter.widget-view+json', 'application/javascript', 'text/html', 'text/markdown', 'image/svg+xml', 'text/latex', 'image/png', 'image/jpeg', 'text/plain']}, 'CSSHTMLHeaderPreprocessor': {'enabled': True}, 'HighlightMagicsPreprocessor': {'enabled': True}, 'RegexRemovePreprocessor': {'enabled': True}, 'TagRemovePreprocessor': {'enabled': True}, 'NbConvertApp': {'export_format': 'html'}, 'TemplateExporter': {'template_name': 'gridstack'}, 'SlidesExporter': {'theme': 'dark'}, 'ExecutePreprocessor': {'enabled': True}}

Maybe I am missing something, if not we should open an issue for traitlets.

@@ -74,7 +74,7 @@ def test_prompt_number(self):
}
}
)
exporter = HTMLExporter(config=no_prompt_conf, template_file='full')
exporter = HTMLExporter(config=no_prompt_conf, template_name='notebook')
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the name of the template should be classic instead of notebook

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For now 'default' is the classic one, it may have been a good idea to have the default html template be a 'nothing' template, without css and all
The classic template would just add some css to the template, have some opinion on the dom structure etc. This way the lab template would not inherit from the classical template.

@@ -1,5 +1,5 @@
{%- extends 'basic.tpl' -%}
Copy link
Member

Choose a reason for hiding this comment

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

does default stand for the default html template?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, I think we can make reveal actually a normal template, just like we want with voila, with just some extra resources. So you would do --template reveal.

@maartenbreddels
Copy link
Collaborator Author

I don't understand check-manifest, I added:

+include share/jupyter/nbconvert/templates/default/static/style.css
+include share/jupyter/nbconvert/templates/lab/static
+include share/jupyter/nbconvert/templates/lab/static/index.css
+include share/jupyter/nbconvert/templates/lab/static/theme-dark.css

To MANIFEST.in, but it still gave a warning, so FTM I'm ignoring that directory.

@t-makaro
Copy link
Contributor

I will take a deeper look at this (hopefully) tomorrow, but I do have a few things on my mind that I'd like to put out there.

  1. Do in-memory templates still work? (Given that template_file and template_name are too different things, I believe the answer is yes.)
  2. Can we have templates of the same name for different file types? Perhaps by having multiple index files within a directory.
template_name/
    index.j2.html
    index.j2.markdown
    index.j2.py

(looking at the code I believe the answer is yes).
3. I do think that template files should have .j2 within them.
4. Does template_path still add the cwd to the search path by default (the the user's ability to change it)? Again, I believe the answer is yes.
5. Can we add some sort of check to make absolute paths still work?
Currently, this line ensures that an absolute path is added to the template search paths if needed. I suppose that template_file can be used with an absolute path?
6. Can index.j2.* files still reference other *.j2.* files inside the same directory? Again, I believe the answer is yes.

@maartenbreddels

However, it seems the HTMLExporters does not pick this up, the alias --theme gets translated to SlidesExporter.theme (!) (from a debug session):

Perhaps related to #822?

@maartenbreddels
Copy link
Collaborator Author

  1. Do in-memory templates still work? (Given that template_file and template_name are too different things, I believe the answer is yes.)

yes, convered in test

2. Can we have templates of the same name for different file types? Perhaps by having multiple index files within a directory.

yes

  1. I do think that template files should have .j2 within them.

I'm 50/50 on it, maybe other may express their opinion, I'll leave it like this for the moment, we can always do a rename at the end.

Does template_path still add the cwd to the search path by default (the the user's ability to change it)? Again, I believe the answer is yes.

yes

Can we add some sort of check to make absolute paths still work?

covered in tests

Can index.j2.* files still reference other *.j2.* files inside the same directory? Again, I believe the answer is yes.

yes, in the same, and other directories.

@maartenbreddels
Copy link
Collaborator Author

After discussion with @SylvainCorlay, the plan is to:
move templates to their separate directory, e.g.:

templates/
          latex/
          markdown/
          python/
          classic/
          lab/

in conf.json, each template can list the mimetype it outputs to (so you can still have multiple mimetypes/formats in 1 template directory, e.g. latex+html). This makes it easier to list only templates of a specific mimetype.
Common template may still go into a directory (say base, or skeleton).

@maartenbreddels
Copy link
Collaborator Author

the suggested change are in, I think this is ready for review.

@t-makaro
Copy link
Contributor

t-makaro commented Aug 7, 2019

Why not rename article.tex to index.tex instead of creating a new file? I believe that the other files inside the latex template directory should be renamed to have .tex extensions instead of .tplx too. Also, the report.tex file could be it's own template.

@t-makaro
Copy link
Contributor

t-makaro commented Aug 7, 2019

In number #1076, I made it possible to use style_*.tplx as their own top level template (making article.tplx redundant). This means in nbconvert 5.6 one can do one of the following:

Jupyter nbconvert --to pdf notebook.ipynb --template=article.tplx
Jupyter nbconvert --to pdf notebook.ipynb --template=style_jupyter.tplx
Jupyter nbconvert --to pdf notebook.ipynb --template=style_ipython.tplx

where the first 2 are equivalent (and the extension can in theory be dropped).

My understand as that after this PR I would do:

Jupyter nbconvert --to pdf notebook.ipynb --template=latex
Jupyter nbconvert --to pdf notebook.ipynb --template-file=style_jupyter.tplx
Jupyter nbconvert --to pdf notebook.ipynb --template-file=style_ipython.tplx

This is where my concern is. It may seem arbitrary to a user to have to use --template-file. If I do:

Jupyter nbconvert --to pdf notebook.ipynb --template=style_jupyter.tplx

will it work? If not, I would refactor these latex templates a fair bit to make these style_*.tplx templates top level. Though I would be happy to do that in a separate PR.

Though I would like if we could detect if a user passed in a template_name with a file extension and use that directly as the template_file then.

@MSeal
Copy link
Contributor

MSeal commented Sep 4, 2019

Ok rebasing 6.x with master added some conflict files unfortunately.

@maartenbreddels
Copy link
Collaborator Author

squash and rebased.
Note: this should be split in 2 commits before merking to give @SylvainCorlay credits for the lab template, but this was impossible to rebase otherwise.

@MSeal
Copy link
Contributor

MSeal commented Oct 15, 2019

@maartenbreddels If you wish you can coauthor the rebase commit by adding https://help.github.com/en/articles/creating-a-commit-with-multiple-authors

I'm good to merge either way, I'll check on this tomorrow and merge it then either way. Thanks a ton for getting this prepped.

@maartenbreddels
Copy link
Collaborator Author

I added two last commits that were needed to make integration with voila smoother, which will make a review easier. I'll squash it all before committing.

@SylvainCorlay do you want to take another look?

The more visible change is in null.tpl/null.j2.
Before we only had thebody block, this was difficult to change, and needed lots of copy pasting. By having body subdivided in body_header/body_loop/body_footer, it's much easier to add something to the header and footer, and not the loop, or visa versa.

@MSeal
Copy link
Contributor

MSeal commented Oct 15, 2019

I can do the squash on merge if you want, or we can leave a few minor commits hanging around.

@MSeal
Copy link
Contributor

MSeal commented Oct 17, 2019

I'm gonna merge this -- we can make changes later if need be :D

@MSeal MSeal merged commit cfb47b5 into jupyter:6.x Oct 17, 2019
@choldgraf
Copy link
Contributor

✨✨✨✨✨💪💪💪💪🤩🤩🤩🤩🤩

@maartenbreddels
Copy link
Collaborator Author

Awesome!
Managed to get the following templates to work with nbconvert and voila

And the voila templates contains almost no code, except for the spinner in the lab template:

@mgeier mgeier mentioned this pull request Oct 26, 2019
@kandersolar kandersolar mentioned this pull request Oct 6, 2020
5 tasks
jasongrout added a commit to jasongrout/nbconvert that referenced this pull request Sep 17, 2021
Fixes jupyter#1430

Many users are reporting that nbconvert 6.x has permission errors when queried or converting files. These seem to arise when nbconvert is installed as root, then used as a user. We hypothesize that the code removed in this commit, which attempts to create template directories throughout the jupyter path hierarchy, succeeds in creating template directories when running as root. Note that it creates the template directories with permission 700. Those permissions as root mean that regular users won’t be able to read these directories, and those are the sort of permission errors we are seeing reported.

This directory creation was originally introduced in jupyter#1028, where I think it was limited to a single user configuration directory (based on the PR discussion). In jupyter#1056, this code appears to have been copied over and applied to the entire Jupyter directory hierarchy.
maartenbreddels pushed a commit that referenced this pull request Sep 17, 2021
* Do not attempt to create additional template paths.

Fixes #1430

Many users are reporting that nbconvert 6.x has permission errors when queried or converting files. These seem to arise when nbconvert is installed as root, then used as a user. We hypothesize that the code removed in this commit, which attempts to create template directories throughout the jupyter path hierarchy, succeeds in creating template directories when running as root. Note that it creates the template directories with permission 700. Those permissions as root mean that regular users won’t be able to read these directories, and those are the sort of permission errors we are seeing reported.

This directory creation was originally introduced in #1028, where I think it was limited to a single user configuration directory (based on the PR discussion). In #1056, this code appears to have been copied over and applied to the entire Jupyter directory hierarchy.

* Filter additional template paths to existing paths to be consistent.
@bdklahn
Copy link

bdklahn commented Sep 1, 2022

Is this issue related, at all, to why there is no "to gridstack"-like entry in the export menu item?
I have been using the Gridstack editor, successfully (with pop-out browser tab rendering), but wondered why there might not be a corresponding entry in the export menu.
I wondered if it were the Gridstack folks who would/could/should add that, but . . .
voila-dashboards/voila-gridstack#180 (comment)

I've been able to get the Voila-DejaVu-like (static page) behavior using the following config settings, in a script.

    c = Config()
    c.TemplateExporter.exclude_input = True
    c.TemplateExporter.exclude_output_prompt = True
    c.TemplateExporter.exclude_input_prompt = True
    c.ExecutePreprocessor.enabled=True
    c.HTMLExporter.template_name = 'gridstack'

Barring any potentially-related fundamental issues, possibly like this, I would think adding a Gridstack render export could be pretty straight-forward(?).

I apologize if this is too unrelated to this and should be in it's own issue.

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

Successfully merging this pull request may close these issues.

None yet

8 participants