Formalizing template loader and debug api's

[Preston Timmons]

Hi all,

I've been working on #15053, which enables Django template loaders to extend

templates of the same name recursively. The most common use-case for this is

extending the admin templates without needing to copy the application templates

into a project.

To illustrate the new algorithm, here are some examples:

(-> indicates the next file that is extended)

(fs indicates filesystem template folders)

1. Extend admin template from filesystem directory

fs/admin/base.html # extends admin/base.html

-> django/admin/base.html

2. Extend in multiple filesystem directories

fs/base.html # extends base.html

-> fs2/base.html # extends base.html

-> fs3/base.html

3. Extend using both a filesystem and app loader with multiple directories

fs/base.html # extends base.html

-> fs2/base.html # extends base.html

-> app1/base.html # extends base.html

-> app2/base.html # extends base2.html

-> fs/base2.html # extends base2.html

-> fs2/base2.html

I found it difficult to support this algorithm with the existing template

loaders, and as you can see from example 3, the extends algorithm isn't

necessarily linear with respect to template loaders anymore.

Therefore, the patch I submitted changes three main areas. The first two are

mostly separate from Aymeric's recent changes, but the third is not.

1. Added a new loader api and made it consistent across all loaders.

2. Modified the extends node to track extended templates and passes

    them as a skip argument to the loaders.

3. Updated the debug view template postmortem api to work with the new

    algorithm, as well as added support for the egg and cached loader.

Below is an explanation of what I changed in 1 and 2 and a proposal for

3 that hopefully works with multiple template engines.

## 1. Add new template loader apis

I tried to solve this patch without changing the template loader apis, but I

eventually decided this was inevitable for two reasons:

1. The loaders don't implement a consistent api. For instance, the

filesystem and app loaders define a get_template_source method that is

used elsewhere for debug information, whereas the egg and cached loaders do

not. The cached loader implements load_template but not load_template_source.

2. The loaders need to be able to skip templates that have already been

extended, but adding a new argument to load_template is difficult to do in a

backwards-compatible way.

This led me to the following loader api:

### Loader API

Loader.get_template_sources

A method that yields all paths the loader will look at for a given template
name. For example, if the filesystem loader receives "index.html" as an
argument, this yields the full path of "index.html" for each directory on the
search path. The cached loader yields tuples of (loader_class, template_paths)
for each configured loader.

This method already exists for the filesystem and app loaders. This patch

implements it for all loaders.

Loader.get_internal

Returns the contents for a template given a ``source`` as returned by

``get_template_sources``.

This is where a filesystem loader would read contents from the filesystem,

or a database loader would read from the database. If a matching template

doesn't exist this should raise a ``TemplateDoesNotExist`` error.

BaseLoader.get_template

Returns a ``Template`` object for a given ``template_name`` by looping

through results from ``get_template_sources`` and calling ``get_internal``.

This returns the first matching template. If no template is found,

``TemplateSyntaxError`` is raised.

This method takes an optional ``skip`` argument. ``skip`` is a set 

containing template sources. This is used when extending templates to

allow enable extending other templates of the same name. It's also used

to avoid recursion errors.

In general, it will be enough to define ``get_template_sources`` and

``get_internal`` for custom template loaders. ``get_template`` will

usually not need to be overridden.

This loader api enables the following changes:

## 2. Update the extends tag

The new extends tag keeps track of which sources were tried in the local

context. These sources are passed to the loader ``get_template`` method as

the ``skip`` argument. In doing so, the extends tag never extends the same 

source file twice. This enables recursive extending, and also avoids 

filesystem recursion errors when extending is truly circular.

The main caveat here is that I changed Template.origin to always be available

on the template--not just when TEMPLATE_DEBUG is true. Without this, the

extends tag has no way to know the source path of the current template.

I think this change is okay, but I don't know the original reasons for

excluding this in the first place. It could be simply because there was no

use-case presented outside of debug.

## 3. Debug api

Django displays a template postmortem message when a matching template isn't

found. This is a linear list grouped by template loader. This api is currently

supported only by the filesystem and app directories loaders.

This api works by looping through template loaders that define a

``get_template_sources`` method, calling that method, and using os.path

functions on each source to see if it exists as a file or is readable. This

information is passed in as ``loader_debug_info`` to the debug template.

There are some limitations to this approach:

* It only works with Django loaders that define a ``get_template_sources`` 

  method -- the filesystem and app loaders

* It only works if ``get_template_sources`` are filesystem paths

* It assumes each loader will only be called at most once and in order. This

  isn't always the case if template recursion is enabled.

* The postmortem doesn't account for multiple template engines.

This patch proposes a new debug api that works with all loaders. It simplifies

the logic in the debug views quite a bit, and I think it can work for multiple

template engines as well, as explained below.

### The approach

Rather than trying to rerun template engines or loaders, this patch passes in

debug information as part of ``TemplateDoesNotExist``. This is done by updating

the multiple places where Django catches ``TemplateDoesNotExist`` exceptions

and silently throws them away. Instead of simply silencing them, this patch

accumulates which templates failed and includes them in subsequent

``TemplateDoesNotExist`` exceptions that are raised.

This information is kept as a list of tuples saved to a ``tried`` attribute of

the exception.

Here's a somewhat complex example of this attribute when using multiple

loaders. It extends through multiple loaders, but fails when no more matching

templates exist:

[

    # Note: (loader, path, status)

    (filesystem.Loader, "/path/to/fs/base.html", "Found")

    (filesystem.Loader, "/path/to/fs/base.html", "Skipped")

    (app_directories.Loader, "/path/to/app/base.html", "Found")

    (filesystem.Loader, "/path/to/fs/base.html", "Skipped")

    (filesystem.Loader, "/path/to/app/base.html", "Skipped")

    (app_directories.Loader, "/path/to/app2/base.html", "Source does not exist")

]

Here's a before and after of what the postmortem looks like:

Before:

After:

After (complex example):

### Multiple engine support

I'll publish an updated pull request once Aymeric's multiple template engines

branch lands. At that time, I think the ``tried`` attribute will change to

this:

[

    # (loader, path, status)

    (engine, filesystem.Loader, "/path/to/fs/base.html", "Found")

    (engine, filesystem.Loader, "/path/to/fs/base.html", "Skipped")

    (engine, app_directories.Loader, "/path/to/app/base.html", "Found")

    (engine, filesystem.Loader, "/path/to/fs/base.html", "Skipped")

    (engine, filesystem.Loader, "/path/to/app/base.html", "Skipped")

    (engine, app_directories.Loader, "/path/to/app2/base.html", "Source does not exist")

]

With this api, other engines can display a postmortem by simply passing a

``tried`` value into ``TemplateDoesNotExist``. I'll qualify simply, though,

because other template engines don't necessarily provide an easy way to get the

list of templates that were tried in their internal exceptions. This seems to

be the case with Jinja2.

Jinja2 loaders have a list_templates method, but it's not that helpful here,

escecially since the results are sorted alphabetically. With that said, I don't

think it would be too bad to support the postmortem for the main loaders:

Filesystem, PackageLoader, ChoiceLoader, etc. It does require reimplementing

some code, though, to loop through the loader search paths.

With multiple engine support, I see the template postmortem info changing

to the format below. This information is optional in case engine is unable to

provide it. Here's a simple example with some engines, 1, 2, and 3, a template

that doesn't exist:

Django tried loading templates with <engine1>:

  * path/to/template1 (Source does not exist)

  * otherpath/to/template1 (Source does not exist)

Django tried loading templates with <engine2>:

  * path/to/template1 (Source does not exist)

  * otherpath/to/template1 (Source does not exist)

Django tried loading templates with <engine3>:

  * template1 (Source does not exist)

  (this engine doesn't provide the list of templates that were tried)

## Conclusion

Overall, I'm pretty happy with this new api. I have a few concerns though,

which are as follows:

1. This patch deprecates the old loader apis: Loader.load_template,

    Loader.load_template_sources, etc. I think this is good, but there's already

    a lot of deprecations happening with the multiple engine branch.

2. The origin api needs a design decision. I made that available regardless of

    the debug setting. Also, it sounds like Aymeric may be doing some work to

    refactor this api.

3. The debug api is also something Aymeric is scheduled to work on. Aymeric,

    if you've got something better in mind I've no problem waiting for that to be

    in place.

Feedback is welcome.

Thanks.

Preston

[Unai Zalakain]

Thanks for your great work Preston, I tried to tackle #15053 myself and
I found it was quite a complex issue. I hope I will find some time to
look at it in more detail!

><https://lh5.googleusercontent.com/-6QNQEJ2KRgk/VJ-PD146t3I/AAAAAAAAAL4/7I9_bfG0_CA/s1600/ee0e758e-7e63-11e4-9a72-7571d591de87.png>
>After:
>
><https://lh5.googleusercontent.com/-dV-rsPD1Mng/VJ-PWou7LiI/AAAAAAAAAMA/1jMpl_i8u1M/s1600/17f55944-7e64-11e4-8ff5-8ab12d135f48.png>
>After (complex example):
>
><https://lh6.googleusercontent.com/-01W4-h6L8jY/VJ-Pj8bPeEI/AAAAAAAAAMI/cT9qEU8jWj8/s1600/3010746e-7e64-11e4-919b-d222b66a28d7.png>

>--
>You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
>To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
>To post to this group, send email to django-d...@googlegroups.com.
>Visit this group at http://groups.google.com/group/django-developers.
>To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/9389e64f-a0cb-475b-9b42-3964e03046be%40googlegroups.com.
>For more options, visit https://groups.google.com/d/optout.


--
unai

[Riccardo Magliocchetti]

Hello Preston,

Il 28/12/2014 06:30, Preston Timmons ha scritto:
> Hi all,
>
> I've been working on #15053, which enables Django template loaders to extend
> templates of the same name recursively. The most common use-case for this is
> extending the admin templates without needing to copy the application
> templates
> into a project.

Thanks a lot for working on this!

> ### Loader API
[snip]

> Loader.get_internal
>
> Returns the contents for a template given a ``source`` as returned by
> ``get_template_sources``.

The name is a bit confusing to me, what about Loader.get_contents instead?

thanks,
riccardo

[Preston Timmons]

Hi Unai. Yes, your previous work was helpful for me in developing this solution. Some of Aymeric's recent refactorings have simplified things since you last worked on it as well.

Hi Riccardo. Thanks for the suggestion. I don't mind changing the name of get_internal if others agree. That's an easy fix.

[Aymeric Augustin]

Hi Preston,

Thanks for working on this ticket — and for bearing with my changes :-)

On 28 déc. 2014, at 06:30, Preston Timmons <preston...@gmail.com> wrote:

## 1. Add new template loader apis

I tried to solve this patch without changing the template loader apis, but I

eventually decided this was inevitable for two reasons:

1. The loaders don't implement a consistent api. For instance, the

filesystem and app loaders define a get_template_source method that is

used elsewhere for debug information, whereas the egg and cached loaders do

not. The cached loader implements load_template but not load_template_source.

Even if it isn’t consistently implemented, the loader API is documented:

https://docs.djangoproject.com/en/1.7/ref/templates/api/#using-an-alternative-template-language

To sum up:

load_template_source(template_name) —> (template_string, template_origin)

load_template(template_name) —> (template, template_origin)

The behavior is loosely defined; anything that matches the signature is fine.

template_origin is especially ill-defined.

Are you proposing to change the API outright? Or are you planning to

implement a deprecation path for existing third-party template loaders?

2. The loaders need to be able to skip templates that have already been

extended, but adding a new argument to load_template is difficult to do in a

backwards-compatible way.

At first sight adding a “skip” optional argument to Engine.get_template,

Engine.find_template, BaseLoader.__call__ and BaseLoader.load_template

may work. Could you explain where the difficulty lies?

This led me to the following loader api:

### Loader API

Loader.get_template_sources

A method that yields all paths the loader will look at for a given template
name. For example, if the filesystem loader receives "index.html" as an
argument, this yields the full path of "index.html" for each directory on the
search path. The cached loader yields tuples of (loader_class, template_paths)
for each configured loader.

This method already exists for the filesystem and app loaders. This patch

implements it for all loaders.

How would a loader that loads templates from a database implement this

method?

It seems to me that the proper way to define such a method is “yields some

opaque objects that implement the following API: …” I suspect that API

would be quite similar to the Origin API — maybe an idealized version if

the current version doesn’t suffice.

Loader.get_internal

Returns the contents for a template given a ``source`` as returned by

``get_template_sources``.

This is where a filesystem loader would read contents from the filesystem,

or a database loader would read from the database. If a matching template

doesn't exist this should raise a ``TemplateDoesNotExist`` error.

Attaching this method to the objects yielded by get_template_sources would

provide better encapsulation than attaching it to the Loader itself.

BaseLoader.get_template

Returns a ``Template`` object for a given ``template_name`` by looping

through results from ``get_template_sources`` and calling ``get_internal``.

This returns the first matching template. If no template is found,

``TemplateSyntaxError`` is raised.

This method takes an optional ``skip`` argument. ``skip`` is a set 

containing template sources. This is used when extending templates to

allow enable extending other templates of the same name. It's also used

to avoid recursion errors.

In general, it will be enough to define ``get_template_sources`` and

``get_internal`` for custom template loaders. ``get_template`` will

usually not need to be overridden.

With my suggestions, skip would become an iterable of opaque objects.

These objects would have to be hashable and implement equality.

Your design makes sense. My main suggestion is to fit it in the current APIs.

I would try to reuse load_template instead of introducing get_template and to

hook to a revamped Origin API. The Origin API was documented in Django

1.7; changing it it less disruptive than changing the Loader API. Did you try

this, and if you did, why didn’t it work?

## 2. Update the extends tag

The new extends tag keeps track of which sources were tried in the local

context. These sources are passed to the loader ``get_template`` method as

the ``skip`` argument. In doing so, the extends tag never extends the same 

source file twice. This enables recursive extending, and also avoids 

filesystem recursion errors when extending is truly circular.

The main caveat here is that I changed Template.origin to always be available

on the template--not just when TEMPLATE_DEBUG is true. Without this, the

extends tag has no way to know the source path of the current template.

I think this change is okay, but I don't know the original reasons for

excluding this in the first place. It could be simply because there was no

use-case presented outside of debug.

Yes, origin was a debug-only API for performance reasons. Most of Django's

template engine dates back to 2006. Back then when performance was a

different story from what it i are today, due to improvements in hardware,

improvements in CPython and availability of PyPy.

## 3. Debug api

Django displays a template postmortem message when a matching template isn't

found. This is a linear list grouped by template loader. This api is currently

supported only by the filesystem and app directories loaders.

My general idea is that Django should define generic debugging APIs that all

template backends, built-in or third-party, can implement. The debugging API

when a template isn’t found shouldn’t know about template loaders because

they’re specific to the Django Template Language.

(…)

### The approach

Rather than trying to rerun template engines or loaders, this patch passes in

debug information as part of ``TemplateDoesNotExist``. This is done by updating

the multiple places where Django catches ``TemplateDoesNotExist`` exceptions

and silently throws them away. Instead of simply silencing them, this patch

accumulates which templates failed and includes them in subsequent

``TemplateDoesNotExist`` exceptions that are raised.

This information is kept as a list of tuples saved to a ``tried`` attribute of

the exception.

That makes a lot of sense. django.template.TemplateDoesNotExist is Django’s

canonical API for dealing with these errors. For example Django’s jinja2 backend

reraises jinja2’s TemplateNotFound as TemplateDoesNotExist.

Here's a somewhat complex example of this attribute when using multiple

loaders. It extends through multiple loaders, but fails when no more matching

templates exist:

[

    # Note: (loader, path, status)

    (filesystem.Loader, "/path/to/fs/base.html", "Found")

    (filesystem.Loader, "/path/to/fs/base.html", "Skipped")

    (app_directories.Loader, "/path/to/app/base.html", "Found")

    (filesystem.Loader, "/path/to/fs/base.html", "Skipped")

    (filesystem.Loader, "/path/to/app/base.html", "Skipped")

    (app_directories.Loader, "/path/to/app2/base.html", "Source does not exist")

]

This API is still tied to the Django Template Language. I would prefer a generic

list of (“template identifier”, “loading mechanism”, “reason for failure”) that every

backend could implement.

Furthermore, I’m not sure what this list represents. Is it a “template call stack”

i.e. the chain of templates included up to the point where loading a template

failed? Or is it a list of ways the template backend tried to load the failing

template before giving up?

I think the list is intended to be the latter but your complex example below

makes me think it may be the former.

  

### Multiple engine support

I'll publish an updated pull request once Aymeric's multiple template engines

branch lands. At that time, I think the ``tried`` attribute will change to

this:

[

    # (loader, path, status)

    (engine, filesystem.Loader, "/path/to/fs/base.html", "Found")

    (engine, filesystem.Loader, "/path/to/fs/base.html", "Skipped")

    (engine, app_directories.Loader, "/path/to/app/base.html", "Found")

    (engine, filesystem.Loader, "/path/to/fs/base.html", "Skipped")

    (engine, filesystem.Loader, "/path/to/app/base.html", "Skipped")

    (engine, app_directories.Loader, "/path/to/app2/base.html", "Source does not exist")

]

Adding the engine to the API makes sense, however my suggestion to

make the rest of the data more generic remains.

## Conclusion

Overall, I'm pretty happy with this new api. I have a few concerns though,

which are as follows:

1. This patch deprecates the old loader apis: Loader.load_template,

    Loader.load_template_sources, etc. I think this is good, but there's already

    a lot of deprecations happening with the multiple engine branch.

Yes, you need a better argument than “difficult to do in a backwards-compatible

way” :-)

2. The origin api needs a design decision. I made that available regardless of

    the debug setting. Also, it sounds like Aymeric may be doing some work to

    refactor this api.

I agree with making it always available. The real question is about the API itself.

We documented the existing concrete implementations in Django 1.7 but they’re

inconsistent and their purpose is still unclear.

https://docs.djangoproject.com/en/1.7/ref/templates/api/#template-origin

3. The debug api is also something Aymeric is scheduled to work on. Aymeric,

    if you've got something better in mind I've no problem waiting for that to be

    in place.

I was mainly thinking about providing tracebacks for errors in templates. Jinja2

has fancy integration with Python’s traceback system. Django emulates it in its

debug view with exc.django_template_source and get_template_exception_info.

That’s independent from template loading exceptions.

-- 

Aymeric.

[Preston Timmons]

Hi Aymeric,

Thanks for the feedback! 

    Even if it isn’t consistently implemented, the loader API is documented:
    https://docs.djangoproject.com/en/1.7/ref/templates/api/#using-an-alternative-template-language
    To sum up:
    load_template_source(template_name) —> (template_string, template_origin)
    load_template(template_name) —> (template, template_origin)
    The behavior is loosely defined; anything that matches the signature is fine.
    template_origin is especially ill-defined.
    Are you proposing to change the API outright? Or are you planning to
    implement a deprecation path for existing third-party template loaders?

The deprecation is gradual. Old loaders continue to work but can't take advantage

of recursive extending until they implement get_template_sources and

get_internal.

    How would a loader that loads templates from a database implement this
    method?
    It seems to me that the proper way to define such a method is “yields some
    opaque objects that implement the following API: …” I suspect that API
    would be quite similar to the Origin API — maybe an idealized version if
    the current version doesn’t suffice.

My original thought was it would yield a model instance which is then passed to

get_internal. Your right, though. It'd be better to return an origin or

other opaque object. This is a nice change. I'll update the loaders so they

have a consistent return value.

    Attaching this method to the objects yielded by get_template_sources would
    provide better encapsulation than attaching it to the Loader itself.

Agreed.

    At first sight adding a “skip” optional argument to Engine.get_template,
    Engine.find_template, BaseLoader.__call__ and BaseLoader.load_template
    may work. Could you explain where the difficulty lies?

There are a few reasons I chose to go with a new api:

First, the advantage of "get_template" over "load_template" is that it calls

"get_template_sources" directly. This means the skip and debug information is

readily available in a single place.

If "skip" is added to load_template instead, it also needs to be added to

"load_template_source". That means the skip and debug logic needs to be

duplicated in each of the loaders, including third-party ones. This is because

"load_template_source" is where the looping through template sources currently

exists.

Second, the cached loader reimplements most of load_template with additional

logic. If "get_template_sources" can be counted on, this cleans it up quite a

bit.

Third, returning (template, origin) is redundant if origin is a property of

template. If we change load_template, that ends up sticking around, unless

we later introduce another deprecation for it.

With that said, it's possible to refactor "load_template" to stop using

"load_template_source" and instead call "get_template_sources". If it does

this I can address 1 and 2. I'm not sure if we can do 3. We'd still need to

do the following:

* Add a deprecation period because get_template_sources is required to support

  template recursion.

* If get_template_sources is used, there still needs to be a get_internal

  method of some sort. I could try to reuse load_template_source, but it's

  a pretty big change in behavior of what that function currently does. I'm

  not sure this lends itself to a nice upgrade path.

Therefore, I see the deprecation period for 3rd-party loaders being one of:

1. Add get_template_sources and get_internal

2. Add get_template_sources and modify load_template_source

Do you still prefer refactoring load_template instead? If so, should I add

get_internal or try to modify load_template_source?

    Your design makes sense. My main suggestion is to fit it in the current APIs.
    I would try to reuse load_template instead of introducing get_template and to
    hook to a revamped Origin API. The Origin API was documented in Django
    1.7; changing it it less disruptive than changing the Loader API. Did you try
    this, and if you did, why didn’t it work?

Okay. I think this is how the api would look then:

get_template_sources(template_name) -> yields origin(s)

get_internal(origin) -> template_string

get_template(template_name) -> template

--or--

load_template(template_name) -> (template, template_origin)

    This API is still tied to the Django Template Language. I would prefer a generic
    list of (“template identifier”, “loading mechanism”, “reason for failure”) that every
    backend could implement.

Yes, this is an easy change. I'll add it into my branch.

    Furthermore, I’m not sure what this list represents. Is it a “template call stack”
    i.e. the chain of templates included up to the point where loading a template
    failed? Or is it a list of ways the template backend tried to load the failing
    template before giving up?

Sorry. I didn't explain that example clearly, and it was slightly wrong. If by

"template call stack" you mean the complete list of sources that were tried, then

yes. This means that for a "get_template" call, it's simply a linear list of all tried

sources. For extending, it includes information about the initial get_template

call, then all the subsequent calls in the extends node.

For instance, if you had a filesystem loader with two folders that extended a

missing "base.html", tried would appear as:

[

    (engine, "/path/to/fs/index.html", "filesystem.Loader", "Found")

    (engine, "/path/to/fs/base.html", "filesystem.Loader", "Source does not exist")

    (engine, "/path/to/fs2/base.html", "filesystem.Loader", "Source does not exist")

]

Assume the same example found "base.html" in fs and it also extended

"base.html". If "base.html" didn't exist in fs2, tried would appear as:

[

    (engine, "/path/to/fs/index.html", "filesystem.Loader", "Found")

    (engine, "/path/to/fs/base.html", "filesystem.Loader", "Found")

    (engine, "/path/to/fs/base.html", "filesystem.Loader", "Skipped")

    (engine, "/path/to/fs2/base.html", "filesystem.Loader", "Source does not exist")

]

Again, if neither base.html or index.html exist in fs, and fs2 has index.html but not

base.html:

[

    (engine, "/path/to/fs/index.html", "filesystem.Loader", "Source does not exist")

    (engine, "/path/to/fs2/index.html", "filesystem.Loader", "Found")

    (engine, "/path/to/fs/base.html", "filesystem.Loader", "Source does not exist")

    (engine, "/path/to/fs2/base.html", "filesystem.Loader", "Source does not exist")

]

Perhaps it'd be helpful to annotate "Found" as "Extended" when extending.

Did that explain it better? If not, I can try again. :)

    I agree with making it always available. The real question is about the API itself.
    We documented the existing concrete implementations in Django 1.7 but they’re
    inconsistent and their purpose is still unclear. 

    https://docs.djangoproject.com/en/1.7/ref/templates/api/# template-origin

Oops, that was my fault. :/

I was looking for something easy to contribute to:

https://code.djangoproject.com/ticket/16096

In the pull request discussion Tim was wary about removing the debug check for

adding the origin. The use-case for the ticket was to display the current

template path to front-end devs. I'm not sure it has seen much usage in the

wild.

    I was mainly thinking about providing tracebacks for errors in templates. Jinja2
    has fancy integration with Python’s traceback system. Django emulates it in its
    debug view with exc.django_template_source and get_template_exception_info.
    That’s independent from template loading exceptions.

Okay. Great. I see your branch has been merged. I'll start work on getting mine

updated with your suggestions so far.

Thanks!

Preston

[Aymeric Augustin]

2014-12-29 4:53 GMT+01:00 Preston Timmons <preston...@gmail.com>:

Did that explain it better? If not, I can try again. :)

Yes, I understand better now.

The debugging information is two dimensional data: for each level of

template extending, for each template loader, you have one entry —

entries are 4-uples in your current proposal.

The format you described is a flattened view of this list. That's good

because it avoids encoding the concepts of "template extension" or

"template loader" in the API, keeping it generic.

If entries contain the template identifier i.e. the string you pass to

get_template then the two dimensional view can almost be

reconstructed — almost because it won't detect when a template

extends another template with the same name, the very feature

you're working on adding...

At this point, I think we should talk about template origins. What

is their purpose? What is the complete abstract Origin API? The

current API only includes a `name` attribute and an unspecified

`reload()` method; I don't think that's sufficient.

I will audit current uses of Origin in Django. Could you specify

the requirements for objects yielded by get_template_sources?

Then we'll see if they're compatible. If we can avoid introducing

a new concept, that's always better.

Thanks,

--

Aymeric.

[Preston Timmons]

    The debugging information is two dimensional data: for each level of
    template extending, for each template loader, you have one entry —

    entries are 4-tuples in your current proposal.

Yes, this is correct. Although there may not be an entry for each loader on

each level of extending. The looping stops if a matching template is found.

    At this point, I think we should talk about template origins. What
    is their purpose? What is the complete abstract Origin API? The
    current API only includes a `name` attribute and an unspecified
    `reload()` method; I don't think that's sufficient.

Origin currently has these attributes:

name

For filesystem and app loaders this is the full path to the template.

For the egg loader this is "egg:<app_config.name>:<pkg_name>".

For locmem this is just the template name.

loader

This is the load_template_source method of the loader that found the template.

loadname

This is the original template_name used for load_template_source.

dirs

This is a deprecated value set when dirs is passed to get_template.

reload

This is used by the debug views to get the source content of the template. It

calls self.loader--which is really load_template_source--with loadname and

dirs.

My branch makes some changes to this class. (Note: this will change a bit if

get_template_sources returns an origin instead)

1. name is the value as returned by get_template_sources. For filesystem and

   app loaders this is still the full path. For the egg and cached loaders it's

   a tuple (app_config.name, template_name) or (loader, source).

2. loader is changed to reference the loader instance rather than the

   load_template_source method.

3. A tried attribute is added. This is the same format as passed to

   TemplateDoesNotExist.

4. reload is updated to call loader.get_internal when available, otherwise

   it falls back to loader.load_template_source.

This is what I use origin for:

1. I add origin.name to the loader extends history. This enables the current

template to be skipped rather than re-extended.

2. If an extended template is found, the new origin.name is added to the

extends history. Also, the original origin.tried and new origin.tried are

combined to give a complete list of templates found before and after extending.

3. If an extended template isn't found, the TemplateDoesNotExist tried value

is combined with the original origin.tried to give a complete list of what

templates were tried before failing.

4. Loader.get_template sets tried on origin with the values returned by

get_template_sources.

## Potential changes

If get_template_sources is changed to return an origin, it seems to make sense

that "tried" would simply be a list of origins instead of a 4-tuple. There

would need to be a status attribute added. This would default to None since we

don't know the status until get_internal is called.

"tried" currently lives on origin. This seems weird to me since origin.tried

would contain a reference to itself. It would probably be better to move that

to the template object. The extends node would then access the list of origins

up to that point, rather than just the current origin.

The extends history would be simplified a bit. Currently, it's a dict that

tracks history for each loader. This is done to eliminate any possible clashes

if two template loaders return the same source value. This could be updated to

just be a list of origins.

It's sufficient for my patch if origin has these values:

origin.name -> the source path - may be a string, tuple, model instance, etc.

origin.loader -> the template loader

origin.loadname -> the template_name value

origin.status -> the status of the template. This is set after get_internal is

called.

origin.loader_name -> returns a string representation of the loader class. This

is used in the debug view postmortem.

## Other template engines

If "tried" is a list of origins, the other template engines would need to use

the origin class if they display a postmortem.

In order to get a correct string representation of the underlying loader in the

postmortem, engines could subclass Origin and implement a custom

loader_name method. Using Origin then should be similar to using the tuple.

I don't think it will cause any tie-in to Django-specific loaders.

Would origin.reload be used for other engines? If so, it could also be added in

the subclass. I'll have to take some time to understand how the traceback works

before I could comment much on this, though.

Thanks.

Preston

[Preston Timmons]

Hi Aymeric,

I got a chance to update my patch with the use of origins. The good news is

that it's simpler than the old implementation. I have a few api questions

below. The updated branch is here for now:

https://github.com/prestontimmons/django/compare/ticket-15053-origin?expand=1

As a quick recap:

First, this branch uses origins everywhere: for extends history, for

TemplateDoesNotExist errors, etc. There's no more 4-tuple of statuses.

Second, I use these attributes on the origin:

engine

loader

loadname -> template name

name -> full template path

status -> Skipped, Found, Source does not exist

tried -> a list of origins tried up to this one

reload -> gets template contents

__eq__

__hash__ -> added so the cache loader can use origin as a cache key

Third, loaders implement these methods:

get_template_sources -> yields origins

get_internal -> takes an origin and returns the template contents

Fourth, TemplateDoesNotExist takes an optional tried argument which is also

a list of origins. This will be used in the debug views. I didn't change the

debug views in this branch yet, though.

Questions

Origin attributes

I'd like to do some renaming:

origin.loadname -> origin.template_name

I think it's hard to remember the difference between origin.name and

origin.loadname. I think "template_name" is more conventional.

origin.reload -> origin.read

If the method to read a template contents lives on the origin, I chose to 

prefer using the origin method rather than the loader method directly. There's

not much consequence here, but it means it's no longer a special method just

for the debug view.

tried and status

The nice thing about putting the tried and status on origin are that it works

well with a recursive api. It even means I could get rid of the extra context

variable used to store extends history. The sucky things are that tried and

status are set and manipulated outside the origin constructor. The cached

loader also has to be sure to reset the origin before returning the cached

template. That might be warranted, but part of me wonders if it's lousy api.

Deprecation path

If we want to maintain the load_template api, I can change it to accept the

skip argument. Within load_template I can add a hasattr check for

"get_internal". If it exists, use the new code. Otherwise, use the old

"load_template_sources" method.

Since adding a kwarg to load_template could potentially break 3rd-party

loaders that override it, I could add an _accepts_skip_kwarg property to

base.Loader like you've done with _accepts_engine_in_init = True. This

would enable Engine.find_template to exclude that if necessary.

If we do this, I'd like to deprecate the (template, template_origin)

return value in favor of just template. Returning a tuple is redundant.

Ditto for Engine.find_template.

If we use get_template instead, we don't need to deprecate the return value and

we don't need _accepts_skip_kwargs. Instead we can just recommend use of the

new method until load_template is removed.

Either way, load_template_sources is obsoleted.

Which deprecation path do you think is preferable?

Reading template contents

If we have an origin.read() function, an option to consider is updating each

loader to return a custom origin object rather than adding "get_internal"

or "load_template_source" to the loader. These could be something like

FileOrigin, EggOrigin, CacheOrigin, DbOrigin, etc.

I can't think of any deeper advantages this holds, though. So I leave it at

that.

Adding skip to Engine.get_template/Engine.find_template

I wondered whether skip should also be added to Engine.get_template. Doing so

eliminates some looping logic from Engine.find_template that is reimplemented

in the extends node. After implementing it though, I've decided against it:

1) In general, users don't work with origins directly. Therefore I don't think

it has much applicability outside the internal loaders.

2) It takes quite a bit of fiddling around to maintain a linear list of

templates that were found or skipped. Even though the new implementation 

removed some duplication, it also wasn't any simpler than the old.

Conclusion

If the api so far makes sense, I'll update the debug view post-mortem

messages and add in the docs and deprecation warnings.

Thanks,

Preston

[Aymeric Augustin]

Hi Preston,

On 14 janv. 2015, at 20:43, Preston Timmons <preston...@gmail.com> wrote:

As a quick recap:

First, this branch uses origins everywhere: for extends history, for

TemplateDoesNotExist errors, etc. There's no more 4-tuple of statuses.

Good.

Second, I use these attributes on the origin:

engine

loader

loadname -> template name

name -> full template path

status -> Skipped, Found, Source does not exist

tried -> a list of origins tried up to this one

reload -> gets template contents

__eq__

__hash__ -> added so the cache loader can use origin as a cache key

Perhaps we’ll bikeshed names at some point but let’s leave the “naming

things” problem aside for now.

“name” must be optional for loaders that don’t load templates from files.

“loadname” must be optional too for templates instantiated from strings.

Here optional means that things don’t break if the value is None.

Third, loaders implement these methods:

get_template_sources -> yields origins

get_internal -> takes an origin and returns the template contents

I don’t like the code duplication between get_internal and

load_template_source. I assume your design ended up that way because

you needed get_internal and load_template_sources will be deprecated.

Is this correct?

Fourth, TemplateDoesNotExist takes an optional tried argument which is also

a list of origins. This will be used in the debug views. I didn't change the

debug views in this branch yet, though.

TemplateDoesNotExist is used both internally by the DTL and in generic

APIs such as django.template.loader.get_template. If it’s designed to

accept a list of tried origins, then we need:

- a generic Origin without engine, loader, status and tried (at least)

- a DTL Origin as described above.

Does this make sense?

Questions

Origin attributes

I'd like to do some renaming:

origin.loadname -> origin.template_name

I think it's hard to remember the difference between origin.name and

origin.loadname. I think "template_name" is more conventional.

Yes. Maintaining compatibility with LoaderOrigin as documented in 1.7

appears to be a lost cause anyway.

origin.reload -> origin.read

If the method to read a template contents lives on the origin, I chose to 

prefer using the origin method rather than the loader method directly. There's

not much consequence here, but it means it's no longer a special method just

for the debug view.

I think the appropriate way for origins to return template code would be to

call back into the template loaders. Reading template contents in the job

of the loader. It should be possible to write a single origin class that works

for most loaders.

The sole purpose of the reload() method is to display tracebacks in templates.

This logic should be moved out of the debug view and into the template

backends. See #24119. As a consequence, we should leave it outside of this

refactoring and simply preserve it for DTL origins until that ticket gets fixed.

tried and status

The nice thing about putting the tried and status on origin are that it works

well with a recursive api. It even means I could get rid of the extra context

variable used to store extends history. The sucky things are that tried and

status are set and manipulated outside the origin constructor. The cached

loader also has to be sure to reset the origin before returning the cached

template. That might be warranted, but part of me wonders if it's lousy api.

I’ll take your word on this. I haven’t spent enough time working on the

implementation to suggest an alternative.

Deprecation path

If we want to maintain the load_template api, I can change it to accept the

skip argument. Within load_template I can add a hasattr check for

"get_internal". If it exists, use the new code. Otherwise, use the old

"load_template_sources" method.

Since adding a kwarg to load_template could potentially break 3rd-party

loaders that override it, I could add an _accepts_skip_kwarg property to

base.Loader like you've done with _accepts_engine_in_init = True. This

would enable Engine.find_template to exclude that if necessary.

If we do this, I'd like to deprecate the (template, template_origin)

return value in favor of just template. Returning a tuple is redundant.

Ditto for Engine.find_template.

If we use get_template instead, we don't need to deprecate the return value and

we don't need _accepts_skip_kwargs. Instead we can just recommend use of the

new method until load_template is removed.

Either way, load_template_sources is obsoleted.

Which deprecation path do you think is preferable?

If you’re going to change both its arguments and its return type, you can change

the method's name :-) The second solution looks better to me. Hopefully we can

write a “just do this” guide to help people maintaining custom template loaders

upgrading.

Conclusion

If the api so far makes sense, I'll update the debug view post-mortem

messages and add in the docs and deprecation warnings.

Yes.

-- 

Aymeric.

[Preston Timmons]

    Perhaps we’ll bikeshed names at some point but let’s leave the “naming
    things” problem aside for now.
    “name” must be optional for loaders that don’t load templates from files.
    “loadname” must be optional too for templates instantiated from strings.
    Here optional means that things don’t break if the value is None.

If we combine LoaderOrigin and StringOrigin, then yes, it makes sense for

these to be optional. The origin will then gain an additional "source"

attribute from StringOrigin.

    I don’t like the code duplication between get_internal and
    load_template_source. I assume your design ended up that way because
    you needed get_internal and load_template_sources will be deprecated.
    Is this correct?

Correct.

To minimize duplication I can modify load_template_sources to use get_internal.

The test suite won't hit this code path anymore, though. That means I should

probably add additional tests to cover the deprecated methods.

That leaves the cached loader. If we modify Loader.load_template to

call Loader.get_template when get_internal is available, then I think the

cached.Loader.load_template and cached.loader.find_template methods can be

safely removed.

I'll go ahead and make these updates in the branch.

    TemplateDoesNotExist is used both internally by the DTL and in generic
    APIs such as django.template.loader.get_template. If it’s designed to
    accept a list of tried origins, then we need:
    - a generic Origin without engine, loader, status and tried (at least)
    - a DTL Origin as described above.
    Does this make sense?

I think this is fine.  "tried" and "loader" are internal concepts to Django.

If an engine is able to support the post-mortem an origin needs only to be able

to return a string representation of the load method and a status. These are

optional if the engine can't provide this information.

    Yes. Maintaining compatibility with LoaderOrigin as documented in 1.7
    appears to be a lost cause anyway.

Okay.

    I think the appropriate way for origins to return template code would be to
    call back into the template loaders. Reading template contents in the job
    of the loader. It should be possible to write a single origin class that works
    for most loaders.
    The sole purpose of the reload() method is to display tracebacks in templates.
    This logic should be moved out of the debug view and into the template
    backends. See #24119. As a consequence, we should leave it outside of this
    refactoring and simply preserve it for DTL origins until that ticket gets fixed.

Sounds good.

    If you’re going to change both its arguments and its return type, you can change
    the method's name :-) The second solution looks better to me. Hopefully we can
    write a “just do this” guide to help people maintaining custom template loaders
    upgrading.

Okay. I'll post back when I've implemented things further.

[Preston Timmons]

Hi Aymeric,

It took me awhile, but I haz an update:

https://github.com/django/django/pull/3475

Loader cleanup

This addresses the code duplication between get_internal and

load_template_sources. I also made the app directories loader inherit

from the filesystem loader, since the only real difference between the two

are the directories they look in.

Cached loader

The cached loader didn't clean up so nicely. I thought I could remove

the custom load_template implementation, but a problem arises if the cached

loader is configured with both recursive and non-recursive loaders. Therefore,

I kept the old implementation in place for now.

Updating the cached loader was a bit tricky. The old loader caches templates

and TemplateDoesNotExist exceptions with a key defined by template_name and

template_dirs. This isn't as straightforward when the skip argument is added

to the equation.

The options were then to create a custom cache key per skip argument, or defer

caching to the lower-level methods. During extending, skip always includes the

originating template. That would lead to many more cache misses, so I chose to

do the latter instead. When skip isn't provided, I use the simple algorithm.

There may be a simpler approach, here.

I've done some performance testing without noticing much difference, but this

is something I'll test some more. I also experimented with using lru_cache

instead of local dictionaries. This simplifies the implementation but is

measurably slower.

Origin hash

The cache algorithm I added uses the origin object as a cache key. The "name"

attribute is used as part of the key value. Therefore, the underlying loader

must be sure to set name to a unique value per template.

You mention above that "name" must be optional for loaders that don't load

templates from the filesystem, but I'm not sure this is true. Either name,

or another attribute on origin, needs to uniquely identify the template within

a loader. Otherwise, there's not a way to implement __eq__ and __hash__. The

locmem loader is a non-filesystem loader that is able to do this. I also

implemented a sample db loader that does as well:

https://github.com/prestontimmons/dbloader/blob/master/dbloader/loader.py

Can you think of a loader where we wouldn't want to require a unique name?

Egg and db loader origin

Some loaders use different identifiers for templates. The filesystem loaders

are just file paths and the locmem loader is just a string key, but the egg

loader is a tuple, (app_config.name, pkg_name) and the sample db loader is

the model instance.

The egg and db loaders don't have an obvious attribute on origin to save the

extra information to. You'll see in those cases that I subclassed origin as

EggOrigin and DbOrigin.

Does that makes sense, or should I look for a way to incorporate that

into LoaderOrigin?

LoaderOrigin vs StringOrigin

I looked at combining these, but didn't yet since it doesn't impact the feature

this patch is implementing. If they're combined to a DTLOrigin, should that

live in django.template.base? It can't live in django.template.loader since

that module isn't safe for django.template.base to import.

context.origin

In order for the origin to be available to the ExtendsNode, I also used the

context.origin = self.origin hack in the Context.render method. I saw the logic

you used to only set engine on toplevel_render. When I moved this assignment

into the if statement or not, it didn't seem to make a difference.

Is that a hack we can live with for now?

Debug view

This branch includes an update to the debug views. The best way to view the

output is to run the debug page in a browser. I added a sample project here

with various scenarios:

https://github.com/prestontimmons/project-15053

There's one case where the debug postmortem isn't optimal. If multiple engines

are configured and a template is found by the second engine that fails

extending, the postmortem won't include the templates that were tried by the

initial get_template call to the first engine. I'm not seeing an obvious way

to persist that information at the moment.

Things to do

The next things I plan to do is measure the cached loader performance more

closely and add the docs.

-- 

Preston

[Preston Timmons]

My pull request is updated with a simplified cache loader and docs. I also found

some nicer solutions to some of the hackier things, like making origin available

to the ExtendsNode.

[Preston Timmons]

Hi Aymeric,

I'm thinking of proposing an alternative to the cached loader. This new

approach makes Django faster in general.

To start, I put together some benchmarks here:

https://github.com/prestontimmons/templatebench

The goal was to identify where Django spends it's time. Is it the loaders that

are slow? The parsing? The rendering? Something else?

Here are some basic timings from my Macbook air. This is the cumulative time

to run 1000 iterations:

Instantiating a basic template, i.e. Template("hello"):

0.0344369411

Parsing a complex template with extends and includes:

0.3044617176

Unsurprising so far, but the time for parsing measurably grows as the template

has more to parse.

Running get_template with a simple template, like "hello":

0.1308078766

Running get_template on a complex template:

0.4068300724

With a simple template, more time is spent finding the template than parsing

it. As template contents grow, though, the parsing time far outgrows the

template loading time.

Running get_template on a template with 200 includes:

12.2357971668

Here's a classic case where Django bombs. The parsing time really adds up.

Time to render a basic template:

0.0240666866

Time to render a complex template:

0.1018106937

In this case, the rendering of a complex template takes four times more than

a simple template. This is compared to a 10 times increase in parsing time

from the previous benchmark. A chunk of this time is also parsing, though, due

to extends and include nodes. All in all, the parsing time grows much quicker

than the render time does.

Based on these benchmarks, I've come to believe most of the time in Django

templates is spent on parsing, not on loading templates or rendering. The

cached loader is effective because it removes the need to reparse templates

more than once.

Interesting enough, Jinja2 has different results:

Running get_template with a simple template, like "hello":

0.0112802982

Running get_template with a complex template:

0.0122888088

Even complex templates make little difference in parsing time for Jinja2.

Running get_template on a template with 200 includes:

0.0110247135

Many includes don't make a difference.

Time to render a basic template:

0.0134618282

Time to render a complex template:

0.0217206478

For a complex template, Jinja2 rendering is about 50% faster than Django.

Even so, the overall time difference is small since rendering is quick

anyway.

After digging into Jinja2, I think this is because the Jinja2 environment

keeps an internal cache of templates. If a template is in the cache, it

calls the template "uptodate" method. If "uptodate" is true, the cached

template is used. For filesystem loaders, this incurs a filesystem hit each

time, but that's fine. File system calls aren't the bottleneck. Parsing is.

With that, I wondered if we couldn't do something similar in Django. I made an

experimental commit here, based on my branch:

https://github.com/prestontimmons/django/commit/4683300c60033ba0db472c81244f01ff932c6fb3

This adds internal caching to django.template.engine.Engine and to the extends

node. It also adds an "uptodate" method to the template origin so templates are

reparsed when modified. This is different than the cached loader, which never

checks if templates are changed. That means it's also viable in development.

Running get_template with a simple template, like "hello":

Before: 0.1308078766

After:  0.0192785263

Jinja2: 0.0112802982

Running get_template on a complex template:

Before: 0.4068300724

After:  0.0204186440

Jinja2: 0.0122888088

By parsing only when necessary these benchmarks see a 10-20x speed up.

Running get_template on a template with 200 includes:

Before: 12.2357971668

After:  0.0179648399

Using include many times is now an option.

So far, all the tests pass, and I've been testing with other templates. The

implementation seems almost too easy for the increase in speed.

Granted there's not a dealbreaker I haven't noticed yet, I'd like to propose

that we follow Jinja2's example by adding internal caching in place of the

cache loader. It has a nice speed increase and simplifies things for my

recursive loader branch as well.

There is one risk I can think of. External template tags can store state

on the Node instance rather than context.render_context. This is warned

against in the docs and is not thread-safe. In practice though, if the cached

loader isn't used, a developer could be unaware that they have a problem

at all. Switching to an internal cache would cause those to be revealed.

Even so, I think that can be handled with documentation.

Do you think it's worth making an attempt to formalize this?

Preston

[Aymeric Augustin]

Hi Preston,

Thanks for your continued work on this and sorry for my slow answers.

On 30 janv. 2015, at 03:24, Preston Timmons <preston...@gmail.com> wrote:

> The cache algorithm I added uses the origin object as a cache key. The "name"
> attribute is used as part of the key value. Therefore, the underlying loader
> must be sure to set name to a unique value per template.

Indeed, since you added skipping, you can’t use template_name (the value
passed to get_template()) as the cache key.

> Can you think of a loader where we wouldn't want to require a unique name?

Well, given what I just said above, we don’t have a choice here. We must
make this a requirement of the template loader API.

> Does that makes sense, or should I look for a way to incorporate that
> into LoaderOrigin?

Looking at the implementation, it feels weird to add attributes to the origin
that are only used by the loader… If origins were dumb bags of data this
would be all right but they have a reload() method that calls the loader.

Such mutual dependencies never end well :-/ That’s a broader design
problem not directly related to your refactoring, but while you’re breaking
all the things, perhaps we could fix it too.

> LoaderOrigin vs StringOrigin
>
> I looked at combining these, but didn't yet since it doesn't impact the feature
> this patch is implementing. If they're combined to a DTLOrigin, should that
> live in django.template.base? It can't live in django.template.loader since
> that module isn't safe for django.template.base to import.

Yes, it can go into django.template.base or django.template.origin, a new
module.

By the way, I would like to check if we can deprecate the dirs argument of
LoaderOrigin. It may only be used by deprecated code as of Django 1.8.

> context.origin
>
> In order for the origin to be available to the ExtendsNode, I also used the
> context.origin = self.origin hack in the Context.render method. I saw the logic
> you used to only set engine on toplevel_render. When I moved this assignment
> into the if statement or not, it didn't seem to make a difference.
>
> Is that a hack we can live with for now?

Well this isn’t an example you’re supposed to follow… ;-)

context.origin doesn’t make a lot of sense. Wouldn’t context.template be better?
Then you could walk to context.template.origin.

In fact context.template.engine would be much better than context.engine. I’ll try
to fix that in 1.8 before it’s too late.

> There's one case where the debug postmortem isn't optimal. If multiple engines
> are configured and a template is found by the second engine that fails
> extending, the postmortem won't include the templates that were tried by the
> initial get_template call to the first engine. I'm not seeing an obvious way
> to persist that information at the moment.

This isn’t a big deal. Using multiple Django template engines is an edge case.

--
Aymeric.

[Aymeric Augustin]

On 17 févr. 2015, at 05:46, Preston Timmons <preston...@gmail.com> wrote:

> Even so, I think that can be handled with documentation.

And an option to disable it. This is very easy to implement.

> Do you think it's worth making an attempt to formalize this?

Yes, I do.

This is an independent follow-up to the big refactoring we’re talking about, isn’t it?

--
Aymeric.

[Preston Timmons]

Thanks for your continued work on this and sorry for my slow answers.

No worries. I've been taking the time to get a better grasp of Django templates as a whole.

 

> Does that makes sense, or should I look for a way to incorporate that
> into LoaderOrigin?

Looking at the implementation, it feels weird to add attributes to the origin
that are only used by the loader… If origins were dumb bags of data this
would be all right but they have a reload() method that calls the loader.

Such mutual dependencies never end well :-/ That’s a broader design
problem not directly related to your refactoring, but while you’re breaking
all the things, perhaps we could fix it too.

Yes. I'll look at this. The reload method isn't documented and I think it can

go away if the debug view is updated.

 

By the way, I would like to check if we can deprecate the dirs argument of
LoaderOrigin. It may only be used by deprecated code as of Django 1.8.

Deprecate or remove? :)

This argument is one I don't think is documented anywhere. 

 

In fact context.template.engine would be much better than context.engine. I’ll try
to fix that in 1.8 before it’s too late.

Sure. I forgot to mention it but my latest patch actually solves this by passing origin

in context.render_context. This makes sense because render_context is available

only in the current render scope, rather than modifying the global context. I'm just

as happy with accessing context.template, though.

[Preston Timmons]

Yes, I do.

This is an independent follow-up to the big refactoring we’re talking about, isn’t it?

Cool.

It depends. The cached loader is what I'm least happy with in my refactoring. This 

internal cache idea I think is simpler, more performant, and easier to understand.

Adding it makes the cached loader changes unnecessary.

Because the changes to support it are minimal, I'll at least make a side-branch that

replaces the cached loader changes with an internal cache. If it pans out and doesn't

balloon into further unrelated changes, it might be easier to include it in this refactoring.

Preston

[Aymeric Augustin]

2015-02-17 23:47 GMT+01:00 Preston Timmons <preston...@gmail.com>:

It depends. The cached loader is what I'm least happy with in my refactoring. This 

internal cache idea I think is simpler, more performant, and easier to understand.

Adding it makes the cached loader changes unnecessary.

In that case, would it be possible to do the caching improvements before the main

refactoring? I'm trying to keep patches at a reviewable size :-)

--

Aymeric.

[Preston Timmons]

In that case, would it be possible to do the caching improvements before the main

refactoring? I'm trying to keep patches at a reviewable size :-)

--

Aymeric.

Maybe. The existing loaders don't provide a nice way to add Origin.uptodate. It would

require an informal hack that only supported Django builtin loaders initially.

I'll start by separating out some of the cleanup/groundwork changes I did into separate

PRs. If those can go in first, then there'll be a lot less noise in the patch.

At the moment, I think it will be easiest to land the cache improvements after the

loaders are updated.

[Preston Timmons]

A small update:

I have a local branch working again, but nothing ready to review.

I have decided the origin.status and origin.tried apis I mentioned above aren't

going to work. For templates to be safely cacheable in a multi-threaded

environment, they basically need to be treated as immutable. Anything that

manipulates a template or it's subobjects is problematic.

I think I can work with this restraint, though.

The next things I'll probably submit is to remove the origin.reload method. As

mentioned above, we should prefer template loading to go through the loaders

instead. I've made progress here but have went down the rabbit hole of cleaning

up the debug implementation.

It looks promising that we can combine the debug parsing, as brought up in another

thread, with minimal overhead. Most of the extra time is due to extra function calls,

not tokenizing. These calls aren't necessary if there's a single parser and lexer

class. I think we can avoid copying debug info to every token and node as well.

This can also address #24119.

Unfortunately, this part of the code isn't tested well. When I have additional tests

written I'll submit a PR.

Preston

[Preston Timmons]

My branch is updated. This now combines the origin classes into one. It also

includes a simpler cache algorithm than before. This same algorithm can be

used if we implement an internal cache to the engine, which is now a small

addition to this patch.

I'd like to see origin.reload go away but it's left intact in this branch. My other

PR to rework template debug information for multiple engines removes this.

A related goal for both of these branches is to simplify the debug view to only

deal with the presentation of the debug information rather than creating or

replaying information. It looks like we can simplify it a lot.

Preston