[Django] #25791: Implement autoreload behaviour for cached template loader
Django
--------------------------+------------------------------------------------
Reporter: jaap3 | Owner: nobody
Type: New | Status: new
feature |
Component: Template | Version: master
system |
Severity: Normal | Keywords: autoreload templates cached loader
Triage Stage: | Has patch: 1
Unreviewed |
Easy pickings: 0 | UI/UX: 0
--------------------------+------------------------------------------------
It would be nice to be able get the speed benefit of the cached template
loader during development, but without the downside of having to restart
the server every time you change a template. It turns out it's possible
with just a few changes.
Because it's not really possible to configure the cached template loader I
did have to base this patch on the fix for #25788. Enabling autoreloading
for the cached template loader would work like this:
{{{
TEMPLATES = [{
'BACKEND': 'django.template.backends.django.DjangoTemplates',
'DIRS': [os.path.join(BASE_DIR, 'templates')],
'APP_DIRS': True
'OPTIONS': {
'cache_templates': True,
'autoreload': DEBUG
}
}]
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25791>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jaap3 | Owner: nobody
Type: New feature | Status: new
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: autoreload | Triage Stage:
templates cached loader | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
The proposed API of passing template loader specific configuration in
`'OPTIONS'` doesn't seem like a clean separation of concerns. For example,
any third-party template loaders desiring some configuration options
aren't able to patch the `DjangoTemplates` backend similarly.
As for me, I don't see a big need to use the cached template loader during
local development. How much does this actually improve performance? I'd
like to see some confirmation on the DevelopersMailingList from other
users that the additional complexity is worthwhile. Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/25791#comment:1>
Django
Reporter: jaap3 | Owner: nobody
Type: New feature | Status: new
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: autoreload | Triage Stage:
templates cached loader | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by jaap3):
I'm proposing that optional template caching becomes first class behaviour
of the Django template engine. Using the cached template loader is a good
practice in general, so in my opinion it should be trivial to
enable/disable it.
Having caching and auto reloading options on the Engine is actually on par
with the jinja2 engine. Which for example has an auto_reload option (that
defaults to settings.DEBUG)
(https://github.com/django/django/blob/master/django/template/backends/jinja2.py#L31).
With auto reloading in place Django template caching could even be part of
any fresh Django project created with startproject.
As it is now I've seen many cases where people either forget to turn on
caching in production, or forget to turn off caching during development
(which results in lots of head scratching and frustration).
Finally, using the Engine to configure the cached template loader is also
a lot nice than the current way. The fact that you can configure a
template loader by using a nesting of lists was introduced when the cached
template loader landed:
https://github.com/django/django/commit/44b9076bbed3e629230d9b77a8765e4c906036d1
#diff-eed563533d29c9e8e6f1273759b6314cR74
It doesn't seem to be documented, and it could probably be deprecated with
this patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/25791#comment:2>
Django
Reporter: jaap3 | Owner: nobody
Type: New feature | Status: new
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: autoreload | Triage Stage:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by timgraham):
* stage: Unreviewed => Someday/Maybe
Comment:
Thanks for the additional explanation. Can you please run your proposal by
the DevelopersMailingList so it reaches a wider audience?
--
Ticket URL: <https://code.djangoproject.com/ticket/25791#comment:3>
Django
Reporter: jaap3 | Owner: nobody
Type: New feature | Status: new
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: autoreload | Triage Stage:
templates cached loader | Someday/Maybe
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by prestontimmons):
I think this is an interesting idea. I looked into something similar as
part of previous template refactoring, although my plan was to have the
engine handle caching and deprecate the cached loader. An initial patch is
still up here:
https://github.com/prestontimmons/django/commit/4683300c60033ba0db472c81244f01ff932c6fb3
I found this opens the possibility to potentially confusing cases, though.
For instance, if a template is cached for a path, but a newer template is
placed on the filesystem in a directory that would otherwise have higher
precedence, the cached template always win. This doesn't mean autoreload
is a bad idea, but if implemented, it's not a good default for
development.
I'll second Tim that OPTIONS is really meant for engine configuration, not
loader configuration. I'm not sure we want to blur the lines this way.
--
Ticket URL: <https://code.djangoproject.com/ticket/25791#comment:4>
Django
Reporter: jaap3 | Owner: nobody
Type: New feature | Status: new
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: autoreload | Triage Stage:
templates cached loader | Someday/Maybe
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by jaap3):
I think the issue you mention about template precedence is solvable, how
does Jinja2 handle these cases? Jinja2's engine enables auto_reload if
DEBUG is true (and a cache is set) so I'm assuming it will be turned on in
development in many cases.
Since template (origin) invalidation is the responsibility of the loader
(in both our implementations) it should (probably?) be possible to
implement an is_stale/uptodate function that checks if the template is
superseded.
Regarding loader configuration in the Engine options. The Jina2 engine
passes all it's arguments into an Environment
(http://jinja.pocoo.org/docs/dev/api/#jinja2.Environment) that is used by
Jinja2's internals to configure all sorts of things. Django doesn't have
the concept of an environment, but the engine is used in a similar way (in
fact this commit
https://github.com/django/django/commit/29a977ab143f549c8849e9f08ca5800830eaaabb
#diff-2ec90d8b6d7f44124689729e42876209 specifically mentions that template
loaders can look up configuration on the engine.
But, this isn't so much about configuring loaders as it is about making
template caching a more prominent feature of the Django template engine.
That template caching is currently possible by way of a quasi template
loader is an implementation detail.
I'll try to write up something for the developers mailinglist today to get
some more input.
--
Ticket URL: <https://code.djangoproject.com/ticket/25791#comment:5>
Django
Reporter: jaap3 | Owner: nobody
Type: New feature | Status: new
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: autoreload | Triage Stage:
templates cached loader | Someday/Maybe
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by timgraham):
[https://groups.google.com/d/topic/django-
developers/JMwJWTgtFfc/discussion django-developers discussion]
--
Ticket URL: <https://code.djangoproject.com/ticket/25791#comment:6>
Django
Reporter: jaap3 | Owner: nobody
Type: New feature | Status: new
Component: Template system | Version: master
Severity: Normal | Resolution:
templates cached loader |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
* stage: Someday/Maybe => Accepted
Comment:
Aymeric suggests:
The proposed patch for auto-reloading doesn’t seem too bad in terms of
complexity. Its main drawback is that it add a new auto-reload mechanism,
distinct from django.utils.autoreload.
[[BR]]
I would prefer to trigger the autoreloader on template file changes to
make it easier to switch e.g. to watchman in the future.
--
Ticket URL: <https://code.djangoproject.com/ticket/25791#comment:7>
Django
Reporter: Jaap Roes | Owner: nobody
Type: New feature | Status: new
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: autoreload | Triage Stage: Accepted
templates cached loader |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Tom Forbes):
* cc: Tom Forbes (added)
Comment:
With my work on #27685 this should be easy to do. I've implemented a
generic `autoreload_started` and `file_changed` signal, and the i18n
package uses this to reset it's translation cache when a `.mo` file
changes. We could implement similar logic here to call `reset()` on the
cached loader when a template changes?
--
Ticket URL: <https://code.djangoproject.com/ticket/25791#comment:8>
Django
Reporter: Jaap Roes | Owner: Tom
| Forbes
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: autoreload | Triage Stage: Accepted
templates cached loader |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Tom Forbes):
* owner: nobody => Tom Forbes
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/25791#comment:9>
Django
Reporter: Jaap Roes | Owner: Tom
| Forbes
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: autoreload | Triage Stage: Accepted
templates cached loader |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Tom Forbes):
Patch: https://github.com/django/django/pull/10870
I implemented an autoreloader hook for this, but I felt that simply not
caching templates when DEBUG is True was far simpler and hits the majority
use case.
--
Ticket URL: <https://code.djangoproject.com/ticket/25791#comment:10>
Django
Reporter: Jaap Roes | Owner: Tom
| Forbes
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: autoreload | Triage Stage: Accepted
templates cached loader |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tom Forbes):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/25791#comment:11>
Django
Reporter: Jaap Roes | Owner: Tom
| Forbes
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: autoreload | Triage Stage: Accepted
templates cached loader |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Tim Graham):
Can you explain what you mean by the "majority use case"? The benefit
seems to be that you can use the same `TEMPLATES` settings in development
and production (although #25788 already helps with that). The downside is
that you can no longer get the speed benefit of the cached template loader
in development.
--
Ticket URL: <https://code.djangoproject.com/ticket/25791#comment:12>
Django
Reporter: Jaap Roes | Owner: Tom
| Forbes
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: autoreload | Triage Stage: Accepted
templates cached loader |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Tom Forbes):
By the majority use case I meant people that simply want to have the same
template settings across production and development settings, rather than
duplicating pretty much the same configuration. We do loose the speed
benefit but we never really had it in the first place as (I believe) very
few people used the cached loader in development due to this very issue.
That being said, having re-read the comments here I'm not sure if that
really is the majority use case or just me (in my experience development
is fast enough, but the two slightly different settings is annoying).
The autoreloader is flexible enough to subscribe to template changes, but
the issue is that it's really not simple to do at the cached template
loader level as everything is pretty abstract (and quite rightly so). Also
we run into an issue where the loaders are not actually imported until the
first template is rendered which means that the initial autoreloader
started signal is not correctly registered.
I will see if I can find a better implementation, but the import issue is
especially annoying.
--
Ticket URL: <https://code.djangoproject.com/ticket/25791#comment:13>
Django
Reporter: Jaap Roes | Owner: Tom
| Forbes
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: autoreload | Triage Stage: Accepted
templates cached loader |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 0 => 1
Comment:
I'm not convinced that disabling the cached template loader when
debug=True is much improvement and really solves this ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/25791#comment:14>
Django
Reporter: Jaap Roes | Owner: Tom
| Forbes
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: autoreload | Triage Stage: Accepted
templates cached loader |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Jaap Roes):
I want to add that not having some form template caching during
development may cause performance issues that only happen during
development. This might cause a developer to try to mitigate the perceived
slowness by rolling their own form of template caching. A real world
example of this happening is this wagtail issue:
https://github.com/wagtail/wagtail/pull/3077
--
Ticket URL: <https://code.djangoproject.com/ticket/25791#comment:15>
Django
Reporter: Jaap Roes | Owner: Tom
| Forbes
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: autoreload | Triage Stage: Accepted
templates cached loader |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tom Forbes):
* needs_better_patch: 1 => 0
Comment:
Second attempt at a patch: https://github.com/django/django/pull/12928
--
Ticket URL: <https://code.djangoproject.com/ticket/25791#comment:16>
Django
Reporter: Jaap Roes | Owner: Tom
| Forbes
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: autoreload | Triage Stage: Accepted
templates cached loader |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
Comment:
The basics of the patch seems small and simple here so +1. I'm just going
to mark it PNI whilst we discuss bootstrapping the registration with the
autoloader on the PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/25791#comment:17>
Django
Reporter: Jaap Roes | Owner: Tom
| Forbes
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: autoreload | Triage Stage: Accepted
templates cached loader |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tom Forbes):
* needs_better_patch: 1 => 0
Comment:
I'm not sure what to do about the registrations. I don't hate the way I've
currently done it, and I don't really see it as a big problem to be
honest. We just have an `autoreload` module imported in the
`django.template` namespace 🤷♀️.
I'm happy to do it another way, but I can't think of one and nothing has
been suggested so far. So I'm going to unmark this as PNI for now.
--
Ticket URL: <https://code.djangoproject.com/ticket/25791#comment:18>
Django
Reporter: Jaap Roes | Owner: Tom
| Forbes
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
templates cached loader | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Carlton Gibson):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/25791#comment:19>
Django
Reporter: Jaap Roes | Owner: Tom
| Forbes
Component: Template system | Version: master
Keywords: autoreload | Triage Stage: Ready for
templates cached loader | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Carlton Gibson <carlton@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"658bcc16f1b814b3a063d3fa16fabaea8b471863" 658bcc1]:
{{{
#!CommitTicketReference repository=""
revision="658bcc16f1b814b3a063d3fa16fabaea8b471863"
Fixed #25791 -- Implement autoreload behaviour for cached template loader.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25791#comment:21>
Django
Reporter: Jaap Roes | Owner: Tom
| Forbes
Component: Template system | Version: master
Severity: Normal | Resolution:
templates cached loader | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Carlton Gibson <carlton@…>):
In [changeset:"29845ecf6999e9e31b56a82229d885f8dfb99e04" 29845ecf]:
{{{
#!CommitTicketReference repository=""
revision="29845ecf6999e9e31b56a82229d885f8dfb99e04"
Refs #25791 -- Added get_dirs() method to cached template loader.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25791#comment:20>