Template Caching - Ticket #6262

[Mike Malone]

Sup,

I've been working on template caching (ticket #6262) with a mister
Alex Gaynor. The patch is getting pretty stable and I think it's close
to merge-able, so if anyone wants to take a look at what we've got and
provide feedback: go!

Interesting background reading for people who haven't been part of
this conversation:
http://groups.google.com/group/django-developers/browse_thread/thread/b289b871285b86f5/b97ba9e2e9b9ad86

Ticket: http://code.djangoproject.com/ticket/6262

Latest patch: http://code.djangoproject.com/attachment/ticket/6262/cache_templates.5.diff

The more code reviewers the better, but if you don't have time to read
through the nitty gritty here's a high level overview of the changes:

1. The workhorse django.template.loader.get_template() function will
now return compiled Template objects directly if a template loader
returns one (where a Template object is defined as something with a
render() method).
2. A RenderContext has been added to Context instances. This is
necessary so we can give template.Node instances a thread-safe place
to store state between calls to Node.render().
3. The built-in template tags were updated to use the render context
(specifically, CycleNode, BlockNode, and ExtendsNode).
4. A caching loader has been added to the set of default loaders. To
use it, you instantiate the loader, passing a list of other loaders
that it should wrap. The first time you ask the loader for a template
it'll go through the wrapped loaders and find it, then cache it in
memory. Subsequent requests for the same template are served from
cache. Hazzah!

The patch is pretty complete, it includes tests and docs, so please
take a look! I stuck the updated docs up on my website temporarily, so
if you want to take a look at them in a slightly more readable format
you can check out these URLs:
http://immike.net/django-docs/ref/templates/api.html#loader-types
http://immike.net/django-docs/howto/custom-template-tags.html#template-tag-thread-safety
http://immike.net/django-docs/ref/templates/api.html#using-an-alternative-template-language

Thanks,

Mike

[Alex Gaynor]

> --
>
> You received this message because you are subscribed to the Google Groups "Django developers" group.
> To post to this group, send email to django-d...@googlegroups.com.
> To unsubscribe from this group, send email to django-develop...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/django-developers?hl=.
>
>
>

As Mike said I've been reviewing this patch and I'm basically happy
with it's current state. My one thought was whether to add a
CACHE_TEMPLATE_LOADERS setting, and any loaders in that are cached by
the CachedLoader (assuming it's instantiated with no arguments, it'll
still be possible to explicitly instantiate it with loaders).

Alex

--
"I disapprove of what you say, but I will defend to the death your
right to say it." -- Voltaire
"The people's good is the highest law." -- Cicero
"Code can always be simpler than you think, but never as simple as you
want" -- Me

[Russell Keith-Magee]

On Thu, Nov 12, 2009 at 9:15 AM, Mike Malone <mjma...@gmail.com> wrote:
> Sup,
>
> I've been working on template caching (ticket #6262) with a mister
> Alex Gaynor. The patch is getting pretty stable and I think it's close
> to merge-able, so if anyone wants to take a look at what we've got and
> provide feedback: go!

Hi Mike,

Thanks for the update. I'll do a detailed teardown over the next
couple of days and get back to you.

Yours,
Russ Magee %-)

[Russell Keith-Magee]

On Thu, Nov 12, 2009 at 9:15 AM, Mike Malone <mjma...@gmail.com> wrote:

> Sup,
>
> I've been working on template caching (ticket #6262) with a mister
> Alex Gaynor. The patch is getting pretty stable and I think it's close
> to merge-able, so if anyone wants to take a look at what we've got and
> provide feedback: go!
>
> Interesting background reading for people who haven't been part of
> this conversation:
> http://groups.google.com/group/django-developers/browse_thread/thread/b289b871285b86f5/b97ba9e2e9b9ad86
>
> Ticket: http://code.djangoproject.com/ticket/6262
>
> Latest patch: http://code.djangoproject.com/attachment/ticket/6262/cache_templates.5.diff

Hi Mike,

Here are my review comments. On the whole, I like what I see. These
are pretty much all fairly minor bugs, documentation, or cosmetic
interface changes.

* In the process of reversing the direction of the stack in Context,
the get() method has been neglected - it's still using the old stack
direction.

* The '.loader' extension on TEMPLATE_LOADER entries is consistent
with the old TEMPLATE_LOADER settings (i.e., pointing at a specific
instance/method), but not with the other pluggable backend APIs that
we have.

For example, when you specify the caching and mail backends, you
provide the name of the module, and it is assumed that the backend
module will contain a CacheClass and EmailBackend class, respectively.
For the sake of consistency and clarity, I'd rather see the 'loader'
name suffix as the implied (and required) name for the object in the
loader module, rather than needing to explicitly specify the name of
the loader instance.

Obviously, the legacy support for the get_template_loader function
will need to be an exception here, but moving forward, we should be
aiming at a clean API.

* On the subject of specifying loaders - in order to use the cached
loader, we need to import the cached loader, and have support for
callable loaders in find_template_loader. Requiring imports in the
settings file seems like a bit of a wart to me.

Here's an alternate proposal - rather than allowing callables, how
about allowing entries in TEMPLATE_LADERS to be tuples, and
interpreting the [1:] elements of the tuple as the arguments to the
loader - for example:

TEMPLATE_LOADERS = (
('django.template.loaders.cached', (
'django.template.loaders.filesystem.loader',
'django.template.loaders.app_directories.loader',
)
)
)

Theoretically, this means we could do away with the TEMPLATE_DIRS
setting, since we could specify template directories in the same
fashion. In practice, this probably isn't worth doing, but it is worth
pointing out as a possibility.

This also means we could get away from needing a module-level
instantiation of Loader() objects - you just look in the module for
the Loader class, and find_template_loader instantiates it with the
appropriate arguments (or no arguments if the loader is specified as a
string, rather than a tuple)

* As someone who will need to maintain this documentation over time,
I don't want to have to keep abreast of changes in other template
languages to ensure that Django's documentation is accurate. I have no
problems with mentioning Jinja and Cheetah as other languages that
could be supported in theory, but I'd rather not give the specific
implementation example.

* Also on documentation - the load_template_source methods should be
mentioned in internals/deprecation.txt, and the cached templates
feature should noted in the 1.2 release notes.

* The load_template_source methods should raise a PendingDeprecationWarning

* Lastly - and this is really the only hairy one - the fate of
django.template.loader.find_template_source().

This method isn't documented publicly, but the backwards-compatibility
notes provide that "everything documented in the linked documents
below, and all methods that don’t begin with an underscore – will not
be moved or renamed without providing backwards-compatible aliases."
It then goes on to specifically mention template APIs as a stable
area. By my reading, this means we can't just rename the method.

However, I think there is a fairly simple fix. Keep the new
find_template() method as is, but change find_template_source to be a
wrapper around get_template() that raises a PendingDeprecationWarning,
and raises a full exception if the template that is found is already
compiled (i.e., has a render method). This means that you won't be
able to use a cached template loader if your code is dependent on
find_template_source, but it also means that all existing uses of
find_template_source should continue to work.

*****

Other than these fairly minor bits, I'm happy. However, I would like
to see at least one other set of eyeballs giving a review of this
patch before commit - in particular, to the changes around BlockNode
and ExtendsNode. These are the bits that are the biggest changes
conceptually, and while I can't see any problems, I'd prefer to get a
sanity check from someone else in the core.

Yours,
Russ Magee %-)

[Alex Gaynor]

I think this could get fairly complicated quickly, for example is this valid?

TEMPLATE_LOADERS = (
('django.templaet.loaders.cached', (
('django.template.loaders.filesystem', (
'/home/alex/django/templates/',
)
)
)

Frankly it seems more trouble then it's worth, adding a
CACHED_TEMPLATE_LOADER setting, though vaguely un-dry seems a good
deal cleaner.

> --
>
> You received this message because you are subscribed to the Google Groups "Django developers" group.
> To post to this group, send email to django-d...@googlegroups.com.
> To unsubscribe from this group, send email to django-develop...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/django-developers?hl=.
>
>
>

[Russell Keith-Magee]

On Mon, Nov 16, 2009 at 10:04 PM, Alex Gaynor <alex....@gmail.com> wrote:
> On Mon, Nov 16, 2009 at 8:58 AM, Russell Keith-Magee
> <freakb...@gmail.com> wrote:
>> On Thu, Nov 12, 2009 at 9:15 AM, Mike Malone <mjma...@gmail.com> wrote:
>>> Sup,
>>>
>>> I've been working on template caching (ticket #6262) with a mister
>>> Alex Gaynor. The patch is getting pretty stable and I think it's close
>>> to merge-able, so if anyone wants to take a look at what we've got and
>>> provide feedback: go!

...

>>  * On the subject of specifying loaders - in order to use the cached
>> loader, we need to import the cached loader, and have support for
>> callable loaders in find_template_loader. Requiring imports in the
>> settings file seems like a bit of a wart to me.
>>
>> Here's an alternate proposal - rather than allowing callables, how
>> about allowing entries in TEMPLATE_LADERS to be tuples, and
>> interpreting the [1:] elements of the tuple as the arguments to the
>> loader - for example:
>>
>> TEMPLATE_LOADERS = (
>>    ('django.template.loaders.cached', (
>>            'django.template.loaders.filesystem.loader',
>>            'django.template.loaders.app_directories.loader',
>>        )
>>    )
>> )
>>
> I think this could get fairly complicated quickly, for example is this valid?
>
> TEMPLATE_LOADERS = (
>    ('django.templaet.loaders.cached', (
>        ('django.template.loaders.filesystem', (
>            '/home/alex/django/templates/',
>        )
>    )
> )

Sure. Why not? The intention of what you have written seems pretty
clear to me (typo and missing brackets notwithstanding :-).

> Frankly it seems more trouble then it's worth, adding a
> CACHED_TEMPLATE_LOADER setting, though vaguely un-dry seems a good
> deal cleaner.

That would be the other way to skin that particular cat, and it would
be the way that is consistent with the existing TEMPLATE_DIRS setting
for the filesystem loader.

My only hesitation here is that I'm not wild about the proliferation
of new settings that results. For me, the appeal of keeping this all
in a single setting is that we get to consolidate the configurations
of every template loader in a predictable way.

Russ %-)

[Johannes Dollinger]

Couldn't you just make this explicit (and more readable)?

{{{
# settings.py
TEMPLATE_LOADERS = ('project.template_loaders.loader',)
}}}

{{{
# template_loaders.py
from django.template.loaders.cached import Loader as CachingLoader
from django.template.loaders.filesystem import Loader as
FileSystemLoader

loader = CachingLoader([
FileSystemLoader([
'/path/to/templates',
]),
])
}}}

If I read the patch correctly, that should already be possible and
solves arbitrarily complex initialization for loaders.
If there has to be a convenience addition, how about a simple boolean
`WRAP_TEMPLATE_LOADERS_WITH_CACHING_LOADER=True#FIXME: find a better
name`?

__
Johannes

[Mike Malone]

K, I just uploaded another patch to ticket #6262. Comments inline.

On Mon, Nov 16, 2009 at 5:58 AM, Russell Keith-Magee
<freakb...@gmail.com> wrote:
> On Thu, Nov 12, 2009 at 9:15 AM, Mike Malone <mjma...@gmail.com> wrote:
>> Sup,
>>
>> I've been working on template caching (ticket #6262) with a mister
>> Alex Gaynor. The patch is getting pretty stable and I think it's close
>> to merge-able, so if anyone wants to take a look at what we've got and
>> provide feedback: go!
>>
>> Interesting background reading for people who haven't been part of
>> this conversation:
>> http://groups.google.com/group/django-developers/browse_thread/thread/b289b871285b86f5/b97ba9e2e9b9ad86
>>
>> Ticket: http://code.djangoproject.com/ticket/6262
>>
>> Latest patch: http://code.djangoproject.com/attachment/ticket/6262/cache_templates.5.diff
>
> Hi Mike,
>
> Here are my review comments. On the whole, I like what I see. These
> are pretty much all fairly minor bugs, documentation, or cosmetic
> interface changes.
>
>  * In the process of reversing the direction of the stack in Context,
> the get() method has been neglected - it's still using the old stack
> direction.

Good catch. Fixed in the new patch. I also added a test since this
functionality didn't seem to be covered.

Interesting idea. I had mixed feelings about the import. I'd rather
have this than a new setting.

The only problem here is that it's a bit tricky to tell whether you're
using the old style (a string specifying a callable) or the new style
(a string specifying a module that has a 'Loader' class). I've
implemented this in the new patch, and it seems to work, but it could
probably use a code review. If we're all in agreement I'll go ahead
and update the docs to reflect this new style.

>  * As someone who will need to maintain this documentation over time,
> I don't want to have to keep abreast of changes in other template
> languages to ensure that Django's documentation is accurate. I have no
> problems with mentioning Jinja and Cheetah as other languages that
> could be supported in theory, but I'd rather not give the specific
> implementation example.

Yea, I wasn't sure about including that example. I wrote it up as a
proof of concept, but it probably belongs somewhere as a blog post
instead of being in the official docs.

>  * Also on documentation - the load_template_source methods should be
> mentioned in internals/deprecation.txt, and the cached templates
> feature should noted in the 1.2 release notes.
>
>  * The load_template_source methods should raise a PendingDeprecationWarning

Included in the latest patch.

>  * Lastly - and this is really the only hairy one - the fate of
> django.template.loader.find_template_source().
>
> This method isn't documented publicly, but the backwards-compatibility
> notes provide that "everything documented in the linked documents
> below, and all methods that don’t begin with an underscore – will not
> be moved or renamed without providing backwards-compatible aliases."
> It then goes on to specifically mention template APIs as a stable
> area. By my reading, this means we can't just rename the method.
>
> However, I think there is a fairly simple fix. Keep the new
> find_template() method as is, but change find_template_source to be a
> wrapper around get_template() that raises a PendingDeprecationWarning,
> and raises a full exception if the template that is found is already
> compiled (i.e., has a render method). This means that you won't be
> able to use a cached template loader if your code is dependent on
> find_template_source, but it also means that all existing uses of
> find_template_source should continue to work.

Made find_template_source wrap find_template. It raises an exception
if a compiled template (something with a 'render' method) is found.

Mike