Fixed template assignments with old basic and full names with tpl extension #1387

MSeal · 2020-09-13T20:04:27Z

In the refactor that simplified the template_name fetching we broke using template=basic despite the path to the compatibility templates being added to the search paths. This fixes this by searching the compatibility directory last.

Second half of resolving #1384

MSeal · 2020-09-13T20:05:41Z

nbconvert/exporters/templateexporter.py

+                    if os.path.exists(conf_file):
+                        with open(conf_file) as f:
+                            conf = recursive_update(json.load(f), conf)
+                    break


A minor optimization, this does change the algorithm to find the first instead of the last match. I think this is fine but I'd like other eyes on it in case I need to reverse the iteration ordering.

I am not 100% sure about the break statement.
What if configuration is overriden in a higher precedence extra template dir?

Ping @maartenbreddels. Let me know what you think.

But it will not merge the configuration with other matches?

It wasn't before because it overwrote the conf and conf_file whenever it found a template until the last one found. I don't think it changes the behavior beyond preferring the first exact match over the last exact match. If you're really worried I can remove the optimization.

SylvainCorlay · 2020-09-13T20:20:53Z

thanks for the ping. i will look into this Monday morning paris time.

willingc

LGTM @MSeal. Will leave for @SylvainCorlay to review and comment/merge.

MSeal · 2020-09-13T22:45:01Z

Hmm before merging the comment thread on #1386 should be read. I think we (I?) eliminated the base template pattern and erroneously stated classic maps to that? I posted what I think works around it to get back to original functionality but I'd like some input and what we should do (because it impacts this backwards compatibility PR)

SylvainCorlay · 2020-09-23T13:09:14Z

nbconvert/exporters/templateexporter.py

+                        if os.path.exists(conf_file):
+                            with open(conf_file) as f:
+                                conf = recursive_update(json.load(f), conf)
+                        break


Same here. Not sure the recursion on root_dirs should be conditional on found_at_least_one.

The recursion isn't conditional, it recurses to completion before returning back to this context

SylvainCorlay · 2020-09-23T13:10:17Z

nbconvert/exporters/templateexporter.py

+                            template_names.extend(['classic', 'base'])
+                        if base_template == 'full':
+                            template_names.extend(['lab', 'base'])
+                        break


This approach may collide with an actual template called full or basic.

Maybe we should condition on this at a higher level...

I was thinking of changing the include paths inside template files to include the template name. I think that would resolve a few issues and not require these hacks for the backwards compatibility, as template extensions are similarly running into 2nd hop extension path failures.

Updated PR with the change I discussed -- extending existing templates from custom templates no longer explodes when files are included across template boundaries. e.g. Fixes #1394

changing the include paths inside template files to include the template name

I am not sure about this, because it is a backward incompatible change with respect to the current behavior.

How is it backwards incompatible? The templates behave identically within nbconvert but now when extended from external templates will behave correctly (since they don't have the proper template paths set to include this template). It also protects from multiple inheritance of file extensions -- e.g. there's ambiguity when importing null.js is some places if you didn't include the template name in the extension path.

It is backward incompatible for custom template authors who inherit from base.

…ension

MSeal · 2020-09-24T02:56:57Z

I added the basic template back as well, since it has a need and we made using it more difficult and broke backwards compatibility.

SylvainCorlay · 2020-09-26T08:58:22Z

@MSeal it should not be required to add the path to the base template in jinja templates, unless you override them locally.

Does your change make it necessary?

SylvainCorlay · 2020-09-26T09:01:35Z

@maartenbreddels @MSeal would you like to have a synchronous discussion about how to fix this the best early next week?

MSeal · 2020-09-26T17:12:57Z

@MSeal it should not be required to add the path to the base template in jinja templates, unless you override them locally.

Does your change make it necessary?

Only in the sense that the backwards compatibility templates were not working without it. Similarly there's several open issues pointing out that extending lab/index.html.js and other files within the templating system fail if those templates use relative extension paths within nbconvert.

@maartenbreddels @MSeal would you like to have a synchronous discussion about how to fix this the best early next week?

Would have to be tomorrow (Sunday) or Tuesday morning as my Monday has no wiggle room on my end.

maartenbreddels · 2020-09-28T13:47:21Z

Tuesday works for me.

I think we should not prefix with /lab /base in the extends, as we'll lose the ability to override those files from other directories.

Going over the code, and trying to make the test pass without the prefixes, I don't think we can go with full 5.x compatibility as it is now. I think if we want compatibility with full.tpl and base.tpl, we should just copy the old content of nbconvert 5.x, and keep/add the deprecation warning.

MSeal · 2020-09-28T16:23:33Z

Tuesday works for me.

Invite sent.

we'll lose the ability to override those files from other directories.

I see. Unfortunately since the template_paths don't dynamically reflect extensions made, supporting that pattern breaks the ability for folks to extend the <template>/index.<format>.j2 files as the local extensions fail (not just for backwards compatibility). I didn't see a clean way to automatically add templates that were extended to the template_paths to prevent this issue.

MSeal · 2020-10-02T22:41:09Z

Closing as alternative fixes were merged

MSeal requested review from maartenbreddels, SylvainCorlay and willingc September 13, 2020 20:04

MSeal commented Sep 13, 2020

View reviewed changes

MSeal force-pushed the old-template-names-without-tpl branch from adb1ba2 to 56f0e08 Compare September 13, 2020 20:07

MSeal force-pushed the old-template-names-without-tpl branch from 56f0e08 to d5a22d6 Compare September 13, 2020 20:24

willingc approved these changes Sep 13, 2020

View reviewed changes

This was referenced Sep 16, 2020

'basic' template not found after update to version 6.0.2 #1393

Closed

TemplateNotFound error when extending built-in template #1394

Closed

SylvainCorlay reviewed Sep 23, 2020

View reviewed changes

MSeal added 2 commits September 23, 2020 17:39

Fixed template assignments with old basic and full names with tpl ext…

f921294

…ension

Added basic template back. Fixed transitive extensions

00cf1d2

MSeal force-pushed the old-template-names-without-tpl branch from d5a22d6 to 00cf1d2 Compare September 24, 2020 02:54

Fixed bad reference

92ecebd

maartenbreddels mentioned this pull request Sep 30, 2020

fix: allow custom template files outside of the template system #1429

Merged

This was referenced Oct 1, 2020

Restored basic template from 5.x #1431

Merged

Added proper support for backwards compatibility templates #1432

Merged

MSeal closed this Oct 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed template assignments with old basic and full names with tpl extension #1387

Fixed template assignments with old basic and full names with tpl extension #1387

MSeal commented Sep 13, 2020

MSeal Sep 13, 2020

SylvainCorlay Sep 23, 2020

SylvainCorlay Sep 26, 2020

MSeal Sep 26, 2020

SylvainCorlay commented Sep 13, 2020

willingc left a comment

MSeal commented Sep 13, 2020

SylvainCorlay Sep 23, 2020

MSeal Sep 24, 2020

SylvainCorlay Sep 23, 2020

MSeal Sep 23, 2020

MSeal Sep 24, 2020

SylvainCorlay Sep 26, 2020

MSeal Sep 26, 2020

SylvainCorlay Sep 28, 2020

MSeal commented Sep 24, 2020

SylvainCorlay commented Sep 26, 2020

SylvainCorlay commented Sep 26, 2020

MSeal commented Sep 26, 2020

maartenbreddels commented Sep 28, 2020

MSeal commented Sep 28, 2020

MSeal commented Oct 2, 2020

Fixed template assignments with old basic and full names with tpl extension #1387

Fixed template assignments with old basic and full names with tpl extension #1387

Conversation

MSeal commented Sep 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SylvainCorlay commented Sep 13, 2020

willingc left a comment

Choose a reason for hiding this comment

MSeal commented Sep 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSeal commented Sep 24, 2020

SylvainCorlay commented Sep 26, 2020

SylvainCorlay commented Sep 26, 2020

MSeal commented Sep 26, 2020

maartenbreddels commented Sep 28, 2020

MSeal commented Sep 28, 2020

MSeal commented Oct 2, 2020