Re: Ticket #17093 -- quarantine global state in django.template
350 views
Skip to first unread message
Carl Meyer
Jan 28, 2013, 1:48:31 PM1/28/13
to django-d...@googlegroups.com
Hello Christopher,
Since I opened that ticket and sent you down this rabbit-hole, I can at
least offer some thoughts :-) In full disclosure, I should also say that
since opening the ticket I've had doubts about whether this change
(although clearly an improvement in the abstract) is worth the effort
and disruption to the codebase, especially given the existence of
Jinja2, a standalone (and global-state-free) implementation of a
template language quite similar to Django's (but much faster).
I don't want to discourage you from working on the project if you find
it interesting and informative, and it may well be that the results are
worth the effort and end up being merged into Django; but it is also
possible that we look at the patch and say "sorry, that's just too much
disruption for the benefits" - so you should be prepared for that
possibility. I really don't have a good sense of where the other core
devs stand on how much this type of deep refactoring is worth.
On 01/28/2013 09:30 AM, Christopher Medrela wrote:
> 4. What I did so far? I created base.TemplateEngine class and
> base.default_engine global instance. Some functions (mostly from loader
> module: get_template, find_template, select_templates and from base
> module: add_to_builtins, get_library, compile_string) delegates to
> appropriate methods of default_engine. I also partially rewrote tests
> (only tests/regressiontests/templates/tests.py) so they don't use
> default_engine directly.
That's an impressive amount of progress! I've looked through the code a
bit; it's obviously not complete, but the direction looks right to me.
> 5. The one issue is that the engine is needed everywhere. For example,
> consider creating base.Template instance. At the time of creation the
> template source is being compiled. The template may load a library, so
> the compilation cannot be done without list of all libraries defined in
> the engine. To sum up, we cannot just type Template(source), because
> this depends on template engine. I solved this issue by passing engine
> instance to Template constructor. In order not to break everything, I
> renamed Template class to _Template and created function called Template
> that does "return _Template(default_engine, **kwargs)".
Yes, I'd expect that in this approach most other classes will need a
reference the engine to do their work. I think this should be accepted
and just done thoroughly. I think your backwards-compatibility approach
with Template is generally correct, though I'm not sure _Template is a
good name for what in the long run ought to be the primary public
implementation of the class.
> 6. The same issue occures when you consider including another template.
> loader_tags.IncludeNode needs to be able to load another template. I
> resolved this by passing an engine to base.Parser constructor. A parser
> passes itself to constructors of nodes, so IncludeNode can load another
> template in this way: "parser.engine.find_template(...)".
Makes sense to me.
> 7. Then there is the issue of loaders. At the first, I thought that
> passing the engine will be not necessary if I change behaviour of
> loader.BaseLoader.load_template method so it will always return source
> of template instead of Template instance. So I did it for BaseLoader.
> Few days ago I did the same for loaders.cached.Loader, but I realised
> that this change means no caching at all -- the loader cached compiled
> templates. Now it caches nothing. The current architecture let us to
> cache template sources but not compiled templates.
>
> 8. There are two solutions. First, we can just pass the engine to every
> loader. Second, we can remove cache-loader and implement caching inside
> TemplateEngine. Both solutions are ugly. Do you see any better one?
I think loaders will need to have access to the template engine that is
using them (though for backwards-compatibility they may need to default
to default_engine if not given another engine).
> 9. Now, there is another issue. Changing API. It's obvious that changing
> base.Parser signature is OK since it's an internal structure. It's also
> easy to notice that base.Template is used everywhere so we cannot change
> API of Template. But there are many places where it's unclear if a
> method or class is public (it means that we promise we won't change its
> signature) or internal. For example, I don't know how much we can change
> loaders. I don't know if Django developers types "isinstance(obj,
> Template)" -- it won't work know. I don't know if I can change signature
> of SsiNode as I did. By the way, I think that we should make clear
> distinction between these two methods by using underscore convention.
Historically Django's codebase hasn't made much use of initial
underscores. Instead, the practice has been that backwards compatibility
promises apply only to documented classes and methods. So I think you
can use the documentation as your guide here; if something is not
documented, generally it can be changed.
> 10. As I said I'm trying to rewrite tests from tests.py file so they
> won't use the default engine. There left some sources of dependency on
> default engine. One of them are include tags in tests which load
> templates at the time of importing module [3] (or the source of problems
> is in base.Library.inclusion_tag?). Another one are template.response
> module [4] and all other test files (i. e. callables.py, custom.py). I
> didn't dig into these issues deeply yet since I wanted to publish my
> results and take some feedback.
For the first case, I would say those tests should simply be changed to
not load templates when the module is imported.
As for TemplateResponse, it would need to use default_engine, though I
suppose it could sprout an additional instantiation parameter to accept
an alternative engine.
> 11. A small issue is test
> regressiontests.views.tests.debug.DebugViewTests.test_template_loader_postmortem.
> The problem is in method
> django.views.debug.ExceptionReporter.get_traceback_data. It uses
> template_source_loaders global state which was removed. We can just use
> default_engine. I wonder when a Django developer uses a custom template
> engine instead of the global default one then they will get faulty error
> message. Is it important or negligible?
It looks to me on brief inspection like the TemplateDoesNotExist
exception instance would need to carry with it the necessary debugging
info that is currently pulled out of the global template_source_loaders.
> 12. As I said there is another source of global-state -- django
> settings. I think we can deal with that by passing all necessary
> settings (like TEMPLATE_DEBUG, INSTALLED_APPS etc.) to TemplateEngine
> constructor. It means that we won't be able to create the default engine
> at the module-level since settings are not configured at the time of
> importing template package. We will have to make instancing default
> engine lazy. It means that there should be get_default_engine() function
> which creates the default engine at the very first call. I wonder if we
> will fall into circular imports hell or other problems...
I think you're right that either instantiation or internal
initialization of the default_engine will need to be lazy so that
settings are not accessed at module-scope. This is irritating, but doable.
> 13. At the end I would like to split the template package into new
> django-independent package (called i. e. 'template_engine') and
> django-specific package (named 'templates') which defines default_engine
> and all django-specific tags and filters. By django-independent I mean
> not depending on settings neigher other django modules excluding utils.
> I won't split tests in the same way - I think there is no need for that.
This makes sense in terms of code hygiene, though if you're thinking of
the "template_engine" as something that is likely to see significant use
outside of Django, my feeling is that the existence of Jinja2 makes this
unlikely.
> 14. Is this plan good? What issues do you see? Could you look at my work
> and review it (I wonder especially if I'm using a python feature not
> supported by python version supported by Django?).
Best way to notice this is to fix up the test suite into a running
state, and run it with the supported Python versions (minimum is 2.6
currently, the more likely area for problems is Python 3, since Django
master now supports Python 3).
If you have some
> experience at splitting Django into smaller parts, please share it with
> me. Tell me what I'm doing wrong (and what's good). If you are looking
> for having this ticket done, please tell me what is more important and
> what is negligible, what API do you expect the template engine? If you
> were working at the ticket, how would you complete the ticket? I hope
> that we can make Django better.
The approach you're taking is very much the approach I'd envisioned when
I opened the ticket, and it seems like you're thinking through all the
potential issues carefully; I don't see any obvious problems in your
approach.
As I said above, I've since felt like this problem is perhaps more work
than it is worth at this point in Django's evolution (for myself,
anyway), but I won't speak for anyone else on that score, others may
feel differently.
Carl
Since I opened that ticket and sent you down this rabbit-hole, I can at
least offer some thoughts :-) In full disclosure, I should also say that
since opening the ticket I've had doubts about whether this change
(although clearly an improvement in the abstract) is worth the effort
and disruption to the codebase, especially given the existence of
Jinja2, a standalone (and global-state-free) implementation of a
template language quite similar to Django's (but much faster).
I don't want to discourage you from working on the project if you find
it interesting and informative, and it may well be that the results are
worth the effort and end up being merged into Django; but it is also
possible that we look at the patch and say "sorry, that's just too much
disruption for the benefits" - so you should be prepared for that
possibility. I really don't have a good sense of where the other core
devs stand on how much this type of deep refactoring is worth.
On 01/28/2013 09:30 AM, Christopher Medrela wrote:
> 4. What I did so far? I created base.TemplateEngine class and
> base.default_engine global instance. Some functions (mostly from loader
> module: get_template, find_template, select_templates and from base
> module: add_to_builtins, get_library, compile_string) delegates to
> appropriate methods of default_engine. I also partially rewrote tests
> (only tests/regressiontests/templates/tests.py) so they don't use
> default_engine directly.
That's an impressive amount of progress! I've looked through the code a
bit; it's obviously not complete, but the direction looks right to me.
> 5. The one issue is that the engine is needed everywhere. For example,
> consider creating base.Template instance. At the time of creation the
> template source is being compiled. The template may load a library, so
> the compilation cannot be done without list of all libraries defined in
> the engine. To sum up, we cannot just type Template(source), because
> this depends on template engine. I solved this issue by passing engine
> instance to Template constructor. In order not to break everything, I
> renamed Template class to _Template and created function called Template
> that does "return _Template(default_engine, **kwargs)".
Yes, I'd expect that in this approach most other classes will need a
reference the engine to do their work. I think this should be accepted
and just done thoroughly. I think your backwards-compatibility approach
with Template is generally correct, though I'm not sure _Template is a
good name for what in the long run ought to be the primary public
implementation of the class.
> 6. The same issue occures when you consider including another template.
> loader_tags.IncludeNode needs to be able to load another template. I
> resolved this by passing an engine to base.Parser constructor. A parser
> passes itself to constructors of nodes, so IncludeNode can load another
> template in this way: "parser.engine.find_template(...)".
Makes sense to me.
> 7. Then there is the issue of loaders. At the first, I thought that
> passing the engine will be not necessary if I change behaviour of
> loader.BaseLoader.load_template method so it will always return source
> of template instead of Template instance. So I did it for BaseLoader.
> Few days ago I did the same for loaders.cached.Loader, but I realised
> that this change means no caching at all -- the loader cached compiled
> templates. Now it caches nothing. The current architecture let us to
> cache template sources but not compiled templates.
>
> 8. There are two solutions. First, we can just pass the engine to every
> loader. Second, we can remove cache-loader and implement caching inside
> TemplateEngine. Both solutions are ugly. Do you see any better one?
I think loaders will need to have access to the template engine that is
using them (though for backwards-compatibility they may need to default
to default_engine if not given another engine).
> 9. Now, there is another issue. Changing API. It's obvious that changing
> base.Parser signature is OK since it's an internal structure. It's also
> easy to notice that base.Template is used everywhere so we cannot change
> API of Template. But there are many places where it's unclear if a
> method or class is public (it means that we promise we won't change its
> signature) or internal. For example, I don't know how much we can change
> loaders. I don't know if Django developers types "isinstance(obj,
> Template)" -- it won't work know. I don't know if I can change signature
> of SsiNode as I did. By the way, I think that we should make clear
> distinction between these two methods by using underscore convention.
Historically Django's codebase hasn't made much use of initial
underscores. Instead, the practice has been that backwards compatibility
promises apply only to documented classes and methods. So I think you
can use the documentation as your guide here; if something is not
documented, generally it can be changed.
> 10. As I said I'm trying to rewrite tests from tests.py file so they
> won't use the default engine. There left some sources of dependency on
> default engine. One of them are include tags in tests which load
> templates at the time of importing module [3] (or the source of problems
> is in base.Library.inclusion_tag?). Another one are template.response
> module [4] and all other test files (i. e. callables.py, custom.py). I
> didn't dig into these issues deeply yet since I wanted to publish my
> results and take some feedback.
For the first case, I would say those tests should simply be changed to
not load templates when the module is imported.
As for TemplateResponse, it would need to use default_engine, though I
suppose it could sprout an additional instantiation parameter to accept
an alternative engine.
> 11. A small issue is test
> regressiontests.views.tests.debug.DebugViewTests.test_template_loader_postmortem.
> The problem is in method
> django.views.debug.ExceptionReporter.get_traceback_data. It uses
> template_source_loaders global state which was removed. We can just use
> default_engine. I wonder when a Django developer uses a custom template
> engine instead of the global default one then they will get faulty error
> message. Is it important or negligible?
It looks to me on brief inspection like the TemplateDoesNotExist
exception instance would need to carry with it the necessary debugging
info that is currently pulled out of the global template_source_loaders.
> 12. As I said there is another source of global-state -- django
> settings. I think we can deal with that by passing all necessary
> settings (like TEMPLATE_DEBUG, INSTALLED_APPS etc.) to TemplateEngine
> constructor. It means that we won't be able to create the default engine
> at the module-level since settings are not configured at the time of
> importing template package. We will have to make instancing default
> engine lazy. It means that there should be get_default_engine() function
> which creates the default engine at the very first call. I wonder if we
> will fall into circular imports hell or other problems...
I think you're right that either instantiation or internal
initialization of the default_engine will need to be lazy so that
settings are not accessed at module-scope. This is irritating, but doable.
> 13. At the end I would like to split the template package into new
> django-independent package (called i. e. 'template_engine') and
> django-specific package (named 'templates') which defines default_engine
> and all django-specific tags and filters. By django-independent I mean
> not depending on settings neigher other django modules excluding utils.
> I won't split tests in the same way - I think there is no need for that.
This makes sense in terms of code hygiene, though if you're thinking of
the "template_engine" as something that is likely to see significant use
outside of Django, my feeling is that the existence of Jinja2 makes this
unlikely.
> 14. Is this plan good? What issues do you see? Could you look at my work
> and review it (I wonder especially if I'm using a python feature not
> supported by python version supported by Django?).
Best way to notice this is to fix up the test suite into a running
state, and run it with the supported Python versions (minimum is 2.6
currently, the more likely area for problems is Python 3, since Django
master now supports Python 3).
If you have some
> experience at splitting Django into smaller parts, please share it with
> me. Tell me what I'm doing wrong (and what's good). If you are looking
> for having this ticket done, please tell me what is more important and
> what is negligible, what API do you expect the template engine? If you
> were working at the ticket, how would you complete the ticket? I hope
> that we can make Django better.
The approach you're taking is very much the approach I'd envisioned when
I opened the ticket, and it seems like you're thinking through all the
potential issues carefully; I don't see any obvious problems in your
approach.
As I said above, I've since felt like this problem is perhaps more work
than it is worth at this point in Django's evolution (for myself,
anyway), but I won't speak for anyone else on that score, others may
feel differently.
Carl
Aymeric Augustin
Jan 28, 2013, 3:18:43 PM1/28/13
to django-d...@googlegroups.com
Hi Christopher,
Some feedback below.
2. This package has two kinds of global state. The first one are global attributes defined at module level of template package. These are base.builtins, base.libraries, loader.template_source_loaders and base.templatetags_modules. The second one are dependencies on settings and some other module-level attributes in template package like base.invalid_var_format_string (this is cache; it's a flag which means "is there '%s' in settings.TEMPLATE_STRING_IF_INVALID?") or context._standard_context_processors, which is cache set in context.get_standard_processors function which transform data from settings in some way. I decided to focus on the first kind of global state at first.
If I understand correctly, once your work is complete, you could listen to changes to a bunch of settings, and just re-instanciate the default template engine when any of them is changed.
5. The one issue is that the engine is needed everywhere. For example, consider creating base.Template instance. At the time of creation the template source is being compiled. The template may load a library, so the compilation cannot be done without list of all libraries defined in the engine. To sum up, we cannot just type Template(source), because this depends on template engine. I solved this issue by passing engine instance to Template constructor. In order not to break everything, I renamed Template class to _Template and created function called Template that does "return _Template(default_engine, **kwargs)".
Maybe a stupid question — couldn't you add an optional 'default_engine' keyword argument to the Template class, and if it isn't provided, use the default template engine?
9. Now, there is another issue. Changing API. It's obvious that changing base.Parser signature is OK since it's an internal structure. It's also easy to notice that base.Template is used everywhere so we cannot change API of Template. But there are many places where it's unclear if a method or class is public (it means that we promise we won't change its signature) or internal. For example, I don't know how much we can change loaders. I don't know if Django developers types "isinstance(obj, Template)" -- it won't work know. I don't know if I can change signature of SsiNode as I did. By the way, I think that we should make clear distinction between these two methods by using underscore convention.
As explained by Carl, documented APIs are public, other APIs are private, and Django avoids the underscore convention.
Then there's the gray area of things that aren't formally documented but are known to be used in the wild. Fortunately the template engine is mostly preserved from this problem.
11. A small issue is test regressiontests.views.tests.debug.DebugViewTests.test_template_loader_postmortem. The problem is in method django.views.debug.ExceptionReporter.get_traceback_data. It uses template_source_loaders global state which was removed. We can just use default_engine. I wonder when a Django developer uses a custom template engine instead of the global default one then they will get faulty error message. Is it important or negligible?
There's no such thing as a custom template engine right now. It's very important that the template engine's public APIs keep behave exactly like they do currently; you can deal with specific bits of Django that won't work with custom template engines later on.
14. Is this plan good? What issues do you see? Could you look at my work and review it (I wonder especially if I'm using a python feature not supported by python version supported by Django?)
You must write compatible code for Python 2 and 3. Quick tips:
- have a look at https://docs.djangoproject.com/en/dev/topics/python3/
- make sure all files have `from __future__ import unicode_literals` at the top; add it (and run tests) if it's missing
- don't put any u prefixes before your string literals
--
Aymeric.
ptone
Jan 28, 2013, 7:55:58 PM1/28/13
to django-d...@googlegroups.com
On Monday, January 28, 2013 9:30:44 AM UTC-8, Christopher Medrela wrote:
Hello everybody.
1. I'm working on ticket #17093 [1]. You can see progress of work at github [2]. It's about rewritting django.template so it won't use global state. I wrote this post to tell you what I did so far, what issues I deal with and to discuss them. I hope that we will gain valuable experience in splitting Django into smaller parts.
While I agree with Carl that there is some danger of this being a change that flirts with "is it worth it" in the specific sense - there is a certain value in exploring the process of making Django less monolithic, and in the long run I think Django HAS to go that way. So bravo for taking this on. I've only skimmed the code so far - but will try to make a couple comments.
3. The goal is to write TemplateEngine class to encapsulate global state. For backward-compatibility, there will be one global instance of TemplateEngine and all functions like loader.get_template should delegate to the instance. Tests need to be rewritten.
A global instance is probably unavoidable, but by itself doesn't result in less global state - it just wraps it all in one global object. Functions that need Engine state should be passed an engine instance explicitly wherever possible - not importing default_engine at module level.
4. What I did so far? I created base.TemplateEngine class and base.default_engine global instance. Some functions (mostly from loader module: get_template, find_template, select_templates and from base module: add_to_builtins, get_library, compile_string) delegates to appropriate methods of default_engine. I also partially rewrote tests (only tests/regressiontests/templates/tests.py) so they don't use default_engine directly.
Since your TemplateEngine class represents the current snapshot of features you may want to make rename your current TemplateEngine to BaseTemplateEngine, and then call TemplateEngineWithBuiltins just TemplateEngine.
Any internal method that uses default_engine needs to allow for an explicit engine instance, or be deprecated with a suggestion to use an alternate API that can support an explicit engine. Anything much less is just shuffling chairs without actually removing the dependency on global state.
5. The one issue is that the engine is needed everywhere. For example, consider creating base.Template instance. At the time of creation the template source is being compiled. The template may load a library, so the compilation cannot be done without list of all libraries defined in the engine. To sum up, we cannot just type Template(source), because this depends on template engine. I solved this issue by passing engine instance to Template constructor. In order not to break everything, I renamed Template class to _Template and created function called Template that does "return _Template(default_engine, **kwargs)".
could this just be handled with a kwarg of engine=default_engine?
10. As I said I'm trying to rewrite tests from tests.py file so they won't use the default engine. There left some sources of dependency on default engine. One of them are include tags in tests which load templates at the time of importing module [3] (or the source of problems is in base.Library.inclusion_tag?). Another one are template.response module [4] and all other test files (i. e. callables.py, custom.py). I didn't dig into these issues deeply yet since I wanted to publish my results and take some feedback.
The shim point for all of these IMO should be your get_default_engine function - and although the following suggestion requires some more thought/discussion - the default_engine instance might as well live on the settings object, where the setting itself could be the default engine class. Killing the global state doesn't mean killing the state - but it is a process of gathering it from all the scattered places, and consolidating it into piles, then gather those piles into one (or very few) pile and creating that only in the entry point(s) so that it is in fact local state. If there is a candidate for the one pile, it is settings. But I'm waxing now ;-)
The testing environment is both a great design testbed, and eventual benefactor of reduced global state. It will help surface all the template related API that is making global assumptions, and those places will need to be modified in a way to take an explicit engine instance, using the default if one is not passed.
11. A small issue is test regressiontests.views.tests.debug.DebugViewTests.test_template_loader_postmortem. The problem is in method django.views.debug.ExceptionReporter.get_traceback_data. It uses template_source_loaders global state which was removed. We can just use default_engine. I wonder when a Django developer uses a custom template engine instead of the global default one then they will get faulty error message. Is it important or negligible?
12. As I said there is another source of global-state -- django settings. I think we can deal with that by passing all necessary settings (like TEMPLATE_DEBUG, INSTALLED_APPS etc.) to TemplateEngine constructor. It means that we won't be able to create the default engine at the module-level since settings are not configured at the time of importing template package. We will have to make instancing default engine lazy. It means that there should be get_default_engine() function which creates the default engine at the very first call. I wonder if we will fall into circular imports hell or other problems...
There are dragons lurking here for sure - I agree that all template related settings need to be passed and encapsulated in the engine.
This is an example of why you want to avoid the module level "default_engine" instance completely if at all possible
If the route was to consolidate global state onto settings (something that merits a whole different discussion) - then any internal reference to a template related setting should switch to retrieving it from a passed explicit engine instance, that would default to settings.default_engine
-Preston
Christopher Medrela
Feb 8, 2013, 11:59:58 AM2/8/13
to django-d...@googlegroups.com
Hello. I'm really astonished how fast I get feedback and I really appreciate that. I apologize that you had to wait for my reply so long, but I cannot push myself to stop programming and to share results. :)
I did some progress since creating this topic:
- Almost all tests doesn't rely on the default engine, including tests of custom tag tests. That excludes tests in response.py (this is problematic one) and loaders.py:RenderToStringTest which tests render_to_string function; this function doesn't delegate to TemplateEngine instance in a simple way, so it should be tested.
- The template engine is passed to loaders now, so I was able to restore caching in cache-loader.
- I also did some small things like python 3.x support, making default_engine lazy (by using get_default_engine instead of default_engine) and adding unicode_literals imports. By the way, I added also absolute_import to almost every file. Is that desirable?
I don't want to discourage you from working on the project if you find
it interesting and informative, and it may well be that the results are
worth the effort and end up being merged into Django; but it is also
possible that we look at the patch and say "sorry, that's just too much
disruption for the benefits" - so you should be prepared for that
possibility.
I would like to avoid the situation like that, so what is the source of that disruption, apart from backward incompatibility?
I really don't have a good sense of where the other core
devs stand on how much this type of deep refactoring is worth.
I would like also to know the opinion of other core devs, so... Core devs, is it worth?
> 14. Is this plan good? What issues do you see? Could you look at my work
> and review it (I wonder especially if I'm using a python feature not
> supported by python version supported by Django?).
Best way to notice this is to fix up the test suite into a running
state, and run it with the supported Python versions (minimum is 2.6
currently, the more likely area for problems is Python 3, since Django
master now supports Python 3).
That's the approach I adopted -- red-green refactor loop. I tested under 2.7.3 and 3.2.3 python, all tests pass.
> 7. Then there is the issue of loaders. At the first, I thought that
> passing the engine will be not necessary if I change behaviour of
> loader.BaseLoader.load_template method so it will always return source
> of template instead of Template instance. So I did it for BaseLoader.
> Few days ago I did the same for loaders.cached.Loader, but I realised
> that this change means no caching at all -- the loader cached compiled
> templates. Now it caches nothing. The current architecture let us to
> cache template sources but not compiled templates.
>
> 8. There are two solutions. First, we can just pass the engine to every
> loader. Second, we can remove cache-loader and implement caching inside
> TemplateEngine. Both solutions are ugly. Do you see any better one?
I think loaders will need to have access to the template engine that is
using them (though for backwards-compatibility they may need to default
to default_engine if not given another engine).
There was chicken and egg situation. You had to provide list of loaders to create an template engine instance and to create a loader you had to provide an engine. I solved this issue by introducing two-step creating engine. First you create an template engine, then create a loader (and pass on the engine), eventually call engine.set_loaders(...). This is not the simplest one, but it works. It also made possible solving problem with custom inclusion tags.
> 3. The goal is to write TemplateEngine class to encapsulate global state. For backward-compatibility, there will be one global instance of TemplateEngine and all functions like loader.get_template should delegate to the instance. Tests need to be rewritten.
A global instance is probably unavoidable, but by itself doesn't result in less global state - it just wraps it all in one global object. Functions that need Engine state should be passed an engine instance explicitly wherever possible - not importing default_engine at module level.
I agree that it doesn't mean less global state. But there is good global state and bad global state. For example, we could make DjangoApplication class to avoid global state but there is no need. Global settings is OK since we don't need the ability to create more than one django application instance. Even tests doesn't require more than one instance at the same time, so we can just reinitialize the application when there is need for that. That's done by change_settings decorator and it works well. And there is bad global state, like current template system. I think that Preston Holmes made it plain so I will cite him:
The shim point for all of these IMO should be your get_default_engine function - and although the following suggestion requires some more thought/discussion - the default_engine instance might as well live on the settings object, where the setting itself could be the default engine class. Killing the global state doesn't mean killing the state - but it is a process of gathering it from all the scattered places, and consolidating it into piles, then gather those piles into one (or very few) pile and creating that only in the entry point(s) so that it is in fact local state. If there is a candidate for the one pile, it is settings. But I'm waxing now ;-)
> 2. This package has two kinds of global state. The first one are global attributes defined at module level of template package. These are base.builtins, base.libraries, loader.template_source_loaders and base.templatetags_modules. The second one are dependencies on settings and some other module-level attributes in template package like base.invalid_var_format_string (this is cache; it's a flag which means "is there '%s' in settings.TEMPLATE_STRING_IF_INVALID?") or context._standard_context_processors, which is cache set in context.get_standard_processors function which transform data from settings in some way. I decided to focus on the first kind of global state at first.
You've listed every cache that depends on a setting. It would be interesting to add a listener for the setting_changed signal to reset these caches when appropriate. This is only used in tests; one isn't supposed to change settings at runtime.
If I understand correctly, once your work is complete, you could listen to changes to a bunch of settings, and just re-instanciate the default template engine when any of them is changed.
I didn't say that, but it sounds like a good idea. I tried to achieve that but I fell into circular-imports problem. The problem occures when django/template/__init__.py is being executed. It imports django.template.base [1]. django/template/base.py should import django.test.signals.setting_changed since setting_changed is necessary to create an receiver ("@receiver(setting_changed)"). Then django/test/__init__.py imports django.test.client.Client [2] and django/test/client.py imports django.template.TemplateDoesNotExist [3], but this exception is still undeclared since we suspended parsing django/template/base.py at the beginning where imports exist.
I think that there is an underlying problem which is that client module should not know what exceptions are declared in the template package. What can we do? Since the client module uses the exception in one place only [4], we can import the exception dynamically just before this place. I did that but another circular-imports situation appeared. As I said, there is django.test.client.Client import in django/test/__init__.py [2]. django/test/client.py imports django.test.utils.ContextList [5]. Finally django/test/utils.py imports django.template.loader [6]. And django/template/loader.py imports from django.template.base [7] which parsing was suspended. I didn't play this game any longer since it looks like endless game.
> 10. As I said I'm trying to rewrite tests from tests.py file so they
> won't use the default engine. There left some sources of dependency on
> default engine. One of them are include tags in tests which load
> templates at the time of importing module [3] (or the source of problems
> is in base.Library.inclusion_tag?). Another one are template.response
> module [4] and all other test files (i. e. callables.py, custom.py). I
> didn't dig into these issues deeply yet since I wanted to publish my
> results and take some feedback.
For the first case, I would say those tests should simply be changed to
not load templates when the module is imported.
As for TemplateResponse, it would need to use default_engine, though I
suppose it could sprout an additional instantiation parameter to accept
an alternative engine.
I tried to do this, but there is a problem with pickling template.SimpleTemplateResponse. An instance of the class should be pickable, but this means that an template engine should be pickable also. I don't think it's possible since the engine has the list of tempalte loaders which are not pickable, as far as I know.
> 5. The one issue is that the engine is needed everywhere. For example, consider creating base.Template instance. At the time of creation the template source is being compiled. The template may load a library, so the compilation cannot be done without list of all libraries defined in the engine. To sum up, we cannot just type Template(source), because this depends on template engine. I solved this issue by passing engine instance to Template constructor. In order not to break everything, I renamed Template class to _Template and created function called Template that does "return _Template(default_engine, **kwargs)".
Maybe a stupid question — couldn't you add an optional 'default_engine' keyword argument to the Template class, and if it isn't provided, use the default template engine?
> 13. At the end I would like to split the template package into new
> django-independent package (called i. e. 'template_engine') and
> django-specific package (named 'templates') which defines default_engine
> and all django-specific tags and filters. By django-independent I mean
> not depending on settings neigher other django modules excluding utils.
> I won't split tests in the same way - I think there is no need for that.
This makes sense in terms of code hygiene, though if you're thinking of
the "template_engine" as something that is likely to see significant use
outside of Django, my feeling is that the existence of Jinja2 makes this
unlikely.
Before yours feedback I thought that the ticket is about rewriting template system to make possible to use it outside Django while refactoring it. That affected some decision I made, i. e. separating Template delegation-function and _Template class -- we could not pass an engine to old Template class and use default_engine if the argument is omitted since it means that Template class would need to reference to default_engine. My goal was to separate django-specific things (like new Template delegation-function) from django-independent ones (like new _Template class) and it wouldn't be possible with old Template class. I didn't know about Jinja2 and that it's more suitable for use outside Django. So it seems like the only goal is refactoring -- we need to quarantize global state, but not dependencies on settings. We also don't need to split the template system into django-dependent and django-independent parts.
Now I have an idea that we can refactor the template system so it will be possible to easily write and use other template systems, for example Mako. So anyone will be able to write "custom_engine = MakoTemplateEngine()" in settings (or anywhere else). Next, we can type "render_to_response(engine=custom_engine, ...)" or something like that. Adding support for Mako templates solves many problems of current template system in the best way -- by reusing existing solutions. It solves problem of low speed of django templates and it makes easier to switch do Django for people developing projects based on Mako templates. If we refactor the code properly, class-based views will work with Mako templates. That would be huge advantage over existing solutions like django-mako. I think that could be a good idea for GSoC this year. Do you think so?
Carl Meyer
Feb 8, 2013, 12:36:41 PM2/8/13
to django-d...@googlegroups.com
Hello Christopher,
On 02/08/2013 09:59 AM, Christopher Medrela wrote:
> I don't want to discourage you from working on the project if you find
>
> it interesting and informative, and it may well be that the results are
>
> worth the effort and end up being merged into Django; but it is also
>
> possible that we look at the patch and say "sorry, that's just too much
>
> disruption for the benefits" - so you should be prepared for that
>
> possibility.
>
>
> I would like to avoid the situation like that, so what is the source of
> that disruption, apart from backward incompatibility?
There are always risks associated with major codebase churn - both
unanticipated backwards-incompatibilities, and introducing new bugs. We
have to make sure the benefits of the change outweigh those risks.
> I agree that it doesn't mean less global state. But there is good global
> state and bad global state. For example, we could make DjangoApplication
> class to avoid global state but there is no need. Global settings is OK
> since we don't need the ability to create more than one django
> application instance. Even tests doesn't require more than one instance
> at the same time, so we can just reinitialize the application when there
> is need for that. That's done by change_settings decorator and it works
> well. And there is bad global state, like current template system.
...
state" is so clear, or even exists. A major part of the motivation for
#17093 was that it could be a step along the path towards a
global-state-free Django (using something like a DjangoApplication or
ApplicationConfig object), which would allow running multiple Django
sites in a single process (which is indeed something that some people,
though not most, need). This is mentioned in the ticket description.
If we are not working towards that larger goal (and I'd love to hear
other opinions on whether we should be or not), and external library use
of Django templates is not a strong motivator, and the testing issues
are manageable via override_settings, then I think the rationale for
#17093 begins to look rather weak. No-global-state would be a better
from-scratch design, certainly, but will it bring enough concrete
benefit now to be worth the transition?
> Now I have an idea that we can refactor the template system so it will
> be possible to easily write and use other template systems, for example
> Mako. So anyone will be able to write "custom_engine =
> MakoTemplateEngine()" in settings (or anywhere else). Next, we can type
> "render_to_response(engine=custom_engine, ...)" or something like that.
...
for refactoring the template engine.
Carl
On 02/08/2013 09:59 AM, Christopher Medrela wrote:
> I don't want to discourage you from working on the project if you find
>
> it interesting and informative, and it may well be that the results are
>
> worth the effort and end up being merged into Django; but it is also
>
> possible that we look at the patch and say "sorry, that's just too much
>
> disruption for the benefits" - so you should be prepared for that
>
> possibility.
>
>
> I would like to avoid the situation like that, so what is the source of
> that disruption, apart from backward incompatibility?
unanticipated backwards-incompatibilities, and introducing new bugs. We
have to make sure the benefits of the change outweigh those risks.
> I agree that it doesn't mean less global state. But there is good global
> state and bad global state. For example, we could make DjangoApplication
> class to avoid global state but there is no need. Global settings is OK
> since we don't need the ability to create more than one django
> application instance. Even tests doesn't require more than one instance
> at the same time, so we can just reinitialize the application when there
> is need for that. That's done by change_settings decorator and it works
> well. And there is bad global state, like current template system.
> I didn't know about Jinja2 and that it's more suitable for use outside
> Django. So it seems like the only goal is refactoring -- we need to
> quarantize global state, but not dependencies on settings. We also don't
> need to split the template system into django-dependent and
> django-independent parts.
I'm not sure the distinction between "good global state" and "bad global
> Django. So it seems like the only goal is refactoring -- we need to
> quarantize global state, but not dependencies on settings. We also don't
> need to split the template system into django-dependent and
> django-independent parts.
state" is so clear, or even exists. A major part of the motivation for
#17093 was that it could be a step along the path towards a
global-state-free Django (using something like a DjangoApplication or
ApplicationConfig object), which would allow running multiple Django
sites in a single process (which is indeed something that some people,
though not most, need). This is mentioned in the ticket description.
If we are not working towards that larger goal (and I'd love to hear
other opinions on whether we should be or not), and external library use
of Django templates is not a strong motivator, and the testing issues
are manageable via override_settings, then I think the rationale for
#17093 begins to look rather weak. No-global-state would be a better
from-scratch design, certainly, but will it bring enough concrete
benefit now to be worth the transition?
> Now I have an idea that we can refactor the template system so it will
> be possible to easily write and use other template systems, for example
> Mako. So anyone will be able to write "custom_engine =
> MakoTemplateEngine()" in settings (or anywhere else). Next, we can type
> "render_to_response(engine=custom_engine, ...)" or something like that.
> If we refactor the code properly, class-based views will work with Mako
> templates. That would be huge advantage over existing solutions like
> django-mako. I think that could be a good idea for GSoC this year. Do
> you think so?
This definitely would bring some value; it's the best rationale I see
> templates. That would be huge advantage over existing solutions like
> django-mako. I think that could be a good idea for GSoC this year. Do
> you think so?
for refactoring the template engine.
Carl
Yo-Yo Ma
Feb 9, 2013, 12:19:24 AM2/9/13
to django-d...@googlegroups.com
For anyone developing a SaaS that allows users to create templates through the UI (e.g., a hosted CMS), separately initializable template system is paramount, giving the ability to modify multiple settingsin the template system that are in effect during template rendering, including loaders, template builtins, etc. This way, an application's primary UI can use one set of settings, and the settings for users' templates.
Note: by "settings", I don't necessarily mean settings.py, just template settings, even of that means instantiating a template system on the fly.
Christopher Medrela
Feb 21, 2013, 10:16:09 AM2/21/13
to django-d...@googlegroups.com
Hello. I would like to report progress of my work (not too much new things, rather cleaning and polishing), discuss some issues and propose an idea for next Google Summer Of Code.
Progres from last time:
- Solved problem with TemplateResponse -- it receives optional keyword argument called `engine` (if omitted, then it calls get_default_engine). During pickling the engine is lost. During repickling get_default_engine() is called.
- Dropped idea of invalidating caches: base.invalid_var_format_string and context._standard_context_processors. These settings are used in about a dozen places in tests and it's not really necessary now.
- Renamed _Template back to Template. The Template class accepts optional keyword argument called `engine`. If omitted, then it calls get_default_engine().
I've also rebased commits so the branch forks from updated master. I've reordered and squashed commits and rewritten description of commits so all work ended in less then twenty well-described commits.
I tried to make all tests green before next weekend (Django Sprint is coming!), but I failed. There are some failing tests (exactly 450 xD); they are heisenbugs -- they appear only when you run all tests; if you run only one test suit, then they disappear. I guess that the problem is that the default_engine should be invalidated between some tests. I also bisected commits and I found out that the commit introducing failures is [1]. I will try to fix these problems before the sprint on Saturday, because the current state is almost ready to review (and to merge, I hope) and I would like to have something that can be reviewed at the weekend.
I will sum up what I've done. I've not introduced new features. I've gathered all pieces of global state into one (the default_engine). I've introduced TemplateEngine class and I've rewritten most tests so they create their own TemplateEngine instances instead of using the global one. I tried not to introduce backward incompatibilities.
I've rewritten history so in order not to break existing links I won't use ticket_17093 branch any more. Instead, see ticket_17093v2 branch [1].
OK, I've changed my mind. I agree that the goal of refactoring is to introduce something like DjangoApplication class whose instances will own components like default_engine. So everything will be global-state-free. However, it would be annoying if you had to pass the application instance everywhere and type "trains" like "request.application.query(MyModel).objects.all()". It would break backward compability in almost every line of code. So there must be one global instance of DjangoApplication.
OK, let mi introduce the idea for Google Some Of Code of this year. The idea is stolen from Pyramid [3]. The main new concept is a renderer. A renderer may be current TemplateEngine as well as JSON serializer or mako renderer. When I'm typing a view I would like to write "render(req, {'key':'this is response to AJAX request'}, engine='json')". When I'm writing a class-based view then I want to be able to write:
class MyView(TemplateView):
renderer = 'json'
# template_name not specified since it doesn't make sence for json serializer
def get_context_data(self, **kwargs):
return {'desc':'my response to AJAX request'}
It should be also possible to easily write your own renderer. How it will work and what issues will we have to deal with?
First of all, it's necessary to split current `template` package into `renderer` package and `template` package to keep things separated. The renderer package will contain the base interface of a renderer. I'm not sure if it will contain also loaders, so it will be possible to reuse existing loaders to load for example mako templates. It will also contain the global register of all renderers and new function `get_renderer`. By default, the register will contain `default` renderer which will be, of course, current default_engine.The register can be changed via settings -- we will introduce new setting called `RENDERERS` and it will be "RENDERERS = {'default':'django.template.TemplateEngine'}" by default. All other components of Django should use only functions in `renderer` package. That's second huge task.
I'm going to participate in Django Sprint in Krakow (Poland) this weekend. I would like to talk about it to other participants. Sorry for writing poor English but I hurry really before the spring.
Tom Christie
Feb 22, 2013, 9:13:50 AM2/22/13
to django-d...@googlegroups.com
Hi Christopher,
> OK, let mi introduce the idea for Google Some Of Code of this year. The idea is stolen from Pyramid [3]. The main new concept is a renderer. A renderer may be current TemplateEngine as well as JSON serializer or mako renderer.
I can't really comment on if it'd be appropriate for GSoC or not, but it'd probably be worth you taking a look at Django REST framework's renderers, which do pretty much exactly what you described.
Simple example of rendering to JSON:
class MyView(APIView):
renderer_classes = (JSONRenderer,)
def get(self, request, *args, **kwargs):
return Response({'desc':'my response to AJAX request'})
Rendering to HTML, using template & template context.
class MyView(APIView):
renderer_classes = (HTMLTemplateRenderer,)
def get(self, request, *args, **kwargs):
return Response({'desc':'my response to AJAX request'}, template_name='example.html')
There's a bit more to it than that, but I thought I'd mention it since it's so similar to the API you outlined.
Christopher Medrela
Feb 28, 2013, 4:51:51 PM2/28/13
to django-d...@googlegroups.com
Hello, I made all tests green finally (all commits are tested under Python 2.7.3 and 3.2.3). The problem that took a few days to solve was that the default engine should be recomputed after changing TEMPLATE_LOADERS setting since its _template_source_loaders internal needs to be recalculated. I tried to solve it by adding receiver in template/base.py and then circular-imports problem occured. I tried to solve it (that took a few days). Fortunately, I asked apollo13 and he helped me and told me that I should put the receiver to test/signals.py so I did it.
I also updated master which is important since the hierarchy of test files was restructured. I kept old branch and created new called ticket_17093v3 [new-branch]. The patch is ready to first review (however it still lack documentation).
After gathering all pieces of global state into one, I started to do some clean up. I noticed that the cache loader unnecessary procrastinates resolving loaders list, so I moved the resolving to __init__.
Now there is still a lot of mess in the template system. The mess is about passing on origin of template and some APIs. For example, TemplateEngine.find_template and BaseLoader.load_template return tuple instead of a template. The second element of the tuple is display_name which is necessary to compute origin of template (and the origin is computed in many places! [computing-origin]). The first element may be not a Template instance but... its source.
My plan is that TemplateEngine.find_template and BaseLoader.load_template will return only a Template instance (and no display_name!). The Template will be created in BaseLoader.load_template and that will be place for computing origin of template. That will be much more simpler API than the current one. I tried to do this, but there are some problems.
The main problem is somehow buggy (in my opinion) behaviour of TemplateEngine.find_template [buggy-find-template]. If a loader raises TemplateDoesNotExist, then we catch this exception and check next loader. However, this is not what should be done. Look at the case when the template exists, but it includes a missing template. Including missing template raises the exception and it shouldn't be caught by find_template. The problem occured in many tickets [tickets] but the patches didn't solve the problem -- they're only a workarounds in my opinion.
I think that find_template should distinguish why the TemplateDoesNotExist was raised (because the current template is missing or because the current template exists but it includes some missing templates). The simplest solution is to ensure that the TemplateDoesNotExist is created with template name as its argument (with the exception of TemplateEngine.select_template case). Then find_template should check if the template name in exception is equal to `name` argument. If it's not true then reraise the exception loudly. Otherwise make it silent.
I partially did the changes (however, I didn't pushed it into the branch), and it works. My question is if the changes of API are acceptable since they are backward incompatible. The changes includes: TemplateDoesNotExist (added template_name argument in constructor), TemplateEngine.find_template and BaseLoader.load_template (and its subclasses)(changed return values).
Now I would like to ask about GSOC. Last time I told about my idea of introducing rendererers. Now I think that it doesn't introduce many improvements -- now you can use Django REST Framework (as Tom said) for JSON or you can just write your own render_to_response delegating to mako template system. So I dropped this idea. Now I wonder what are priorities?
- Continuing refactoring (and adding new features) of template system. Better error reporting. Logging invalid variable lookups. Compiling Django templates into python functions so they will render faster.
- Refactoring another part of Django. Is there any wiki page about how Django should be refactored and what principles should I base on when I restructure Django?
- Improving error reporting.
- Revamping of validation functionality.
- Test framework cleanup. Speeding up tests. Splitting long tests migrated from doctests. Testing Django application "in a void" (independently of Django project and everything else). Running all tests against different databases backends in sequence?
Please tell me which of these are most interesting for you (and which one have the greates chance to be a GSoC project). Which ones are important and which ones negligible? The list of ideas from 2012 doesn't contain information which ones are priorities. I would like to know to focus on the most important first.
[computing-origin] Run `git grep -n make_origin`
[buggy-find-template] https://github.com/chrismedrela/django/blob/ticket_17093v3/django/template/base.py#L1362
Reply all
Reply to author
Forward
0 new messages