Formalizing template loader and debug api's

469 views
Skip to first unread message

Preston Timmons

unread,
Dec 28, 2014, 12:30:02 AM12/28/14
to django-d...@googlegroups.com
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

unread,
Dec 28, 2014, 4:34:13 AM12/28/14
to django-d...@googlegroups.com
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
signature.asc

Riccardo Magliocchetti

unread,
Dec 28, 2014, 8:37:42 AM12/28/14
to django-d...@googlegroups.com
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

unread,
Dec 28, 2014, 12:54:51 PM12/28/14
to django-d...@googlegroups.com
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

unread,
Dec 28, 2014, 1:21:04 PM12/28/14
to django-d...@googlegroups.com
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:

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.


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

unread,
Dec 28, 2014, 10:53:31 PM12/28/14
to django-d...@googlegroups.com
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:


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

unread,
Dec 29, 2014, 5:21:47 AM12/29/14
to django-d...@googlegroups.com
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

unread,
Dec 29, 2014, 8:12:05 PM12/29/14
to django-d...@googlegroups.com
    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

unread,
Jan 14, 2015, 2:43:20 PM1/14/15
to django-d...@googlegroups.com
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:


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

unread,
Jan 18, 2015, 5:59:19 AM1/18/15
to django-d...@googlegroups.com
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

unread,
Jan 19, 2015, 5:51:37 PM1/19/15
to django-d...@googlegroups.com
    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

unread,
Jan 29, 2015, 9:24:18 PM1/29/15
to django-d...@googlegroups.com
Hi Aymeric,

It took me awhile, but I haz an update:


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:


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:


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

unread,
Feb 12, 2015, 5:45:38 PM2/12/15
to django-d...@googlegroups.com
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

unread,
Feb 16, 2015, 11:46:19 PM2/16/15
to django-d...@googlegroups.com
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:


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:

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:

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:


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

unread,
Feb 17, 2015, 4:34:29 PM2/17/15
to django-d...@googlegroups.com
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

unread,
Feb 17, 2015, 4:38:01 PM2/17/15
to django-d...@googlegroups.com
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

unread,
Feb 17, 2015, 5:11:14 PM2/17/15
to django-d...@googlegroups.com
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

unread,
Feb 17, 2015, 5:47:09 PM2/17/15
to django-d...@googlegroups.com
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

unread,
Feb 18, 2015, 3:44:02 AM2/18/15
to django-d...@googlegroups.com
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

unread,
Feb 18, 2015, 2:12:09 PM2/18/15
to django-d...@googlegroups.com

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

unread,
Mar 6, 2015, 1:15:41 PM3/6/15
to django-d...@googlegroups.com
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

unread,
Mar 11, 2015, 11:37:21 PM3/11/15
to django-d...@googlegroups.com
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


Reply all
Reply to author
Forward
0 new messages