ugettext_lazy changes (in case I get hit by a bus)
Malcolm Tredinnick
like ugettext_lazy() -- in particular -- is blowing up early in Django's
runtime. Of no interest at all to most people...
In r8120 (and against in r8127), I added a fairly big change to the
django.utils.functional.lazy() function (mostly provided by Jakub Wilk)
that drastically reduces memory usage for things like ugettext_lazy().
That's pretty important, since previously it was taking about 1.8 KB per
instance and now it takes around 140 bytes per instance for 1000
instances and it gets better the more you have. The reason is, we're
storing more methods on the class object (so they exist only once) and
less on the instances (one per instance).
Short version: for somebody writing an app with a lot of strings that
need lazy translation, this is a huge win. It also fixes another problem
with certain methods not being properly proxied by *_lazy() methods.
The drawback can be seen in r8127, r8132, r8138 (thanks, Jacob), where
we have to be careful accessing translated strings whilst models are
being imported. This isn't as bad as it sounds because it's *really
hard* for user code to inject itself at that point in the process, so
all the bugs are inside Django. The problem mostly pops up because the
verbose_name attribute on a model or a field can be marked for
translation. In fact, if you do nothing, verbose_name_plural will be
marked as a lazy translation by default. So if we have code that says:
if self.verbose_name: ...
or
self.verbose_name = self.verbose_name or ....
this will implicitly call bool() on self.verbose_name, which will
evaluate the lazy translation. This requires (knowing and) setting the
current locale. Which requires having all the apps imported, because it
needs access to all the translation strings. Since we're already
importing things, the wheels fall off. It doesn't look immediately clear
to me how to include that in the locking that goes on in
django.db.models.loader, but it also doesn't seem necessary. Those
particular pieces of idiom can be replaced by comparing for equality,
providing you know what the default is (which is how I screwed up r8132
and Jacob fixed it in r8138). So the second line above becomes
if self.verbose_name is not None:
self.verbose_name = ....
providing you know the default is None (adjust accordingly).
Pragmatically, the only real risks here are verbose_name and
verbose_name_plural and only inside places like
django/db/models/fields/* in code that contribute_to_class() might
execute. So I'm comfortable with just avoiding the implicit bool() call.
Anyway, that's why it's happened twice. I think we've got them all, but
I'll look again in a couple of days with fresh eyes to make sure
(anybody else wants to check, go ahead). We don't need to change every
single reference, just those accessed during app loading and that's a
really small set. The benefits of this change are huge (smaller memory
usage and better proxying of methods), so, in my opinion, it's well
worth working around the slight constraint during app loading.
Regards,
Malcolm
Manuel Saelices
in thing like models, if we use ugettext instead ugettext_lazy on load
time.
The simplest example in model file was:
from django.db import models
from django.utils.translation import ugettext as _
class FooModel(models.Model):
name = models.CharField(_(u'Name'), max_length=100)
Also, the error raised is not representative of real problem (change
ugettext to ugettext_lazy).
And I wonder... is not possible to mark act of finishing app loading
and check this mark in ugettext call? if this mark was not set, this
implies we are calling gettext in import time and raise an specific
error for that (or return a "ERROR_IN_GETTEXT_CALL_IN_IMPORT_TIME"
string ;).
Regards,
Manuel Saelices
Malcolm Tredinnick
On Tue, 2008-07-29 at 14:37 -0700, Manuel Saelices wrote:
> In my company we often are having problems with circular import error
> in thing like models, if we use ugettext instead ugettext_lazy on load
> time.
Well, to be fair, that's almost always going to be a bug in your code.
If the code is executed at import time, it shouldn't be using
ugettext(), since the output will possibly have to be in a different
locale at presentation time. But I understand your point; I've seen it,
too.
I gather from what you're writing in your email that you also realise
it's an error, but just in case you don't: "Don't do that!" :-)
[...]
> Also, the error raised is not representative of real problem (change
> ugettext to ugettext_lazy).
>
> And I wonder... is not possible to mark act of finishing app loading
> and check this mark in ugettext call? if this mark was not set, this
> implies we are calling gettext in import time and raise an specific
> error for that (or return a "ERROR_IN_GETTEXT_CALL_IN_IMPORT_TIME"
> string ;).
Quite possible. That is a good idea. I'm not sure whether failing
quietly (and just return the original string) or failing noisily
(because it's pretty much always an error) is the right thing to do. But
either way, the solution is a good one. We already have a way to tell if
all the apps are loaded.
Would you mind to open a ticket for this so I don't forget?
Regards,
Malcolm
Manuel Saelices
>
> Would you mind to open a ticket for this so I don't forget?
I think implementation is quite simple
Regards,
Manuel Saelices
>
> Regards,
> Malcolm
Malcolm Tredinnick
On Tue, 2008-07-29 at 14:57 -0700, Manuel Saelices wrote:
>
> >
> > Would you mind to open a ticket for this so I don't forget?
>
> Of course, I'll do after dinner, and I'll try to add a patch, because
> I think implementation is quite simple
Even better. I should have mentioned in my last mail that
django.db.models.loading.app_cache_ready() is the way to know when we're
"ready".
Regards,
Malcolm
Manuel Saelices
http://code.djangoproject.com/ticket/8033
I'll try to resolve as soon as possible (now is 1am in Spain)
Manuel Saelices
attached? I've found strange Apache (and maybe others production
servers) behaviour. It seems app_cache_ready doesnt work because
nobody calls to django.db.models.loading.get_apps() if you don't call
gettext.
Regards,
Manuel Saelices
Julien Demoor
User model at runtime in an application's __init__ module.
This worked well before r8120, but now it causes a lazy translation
string to be evaluated, followed by a recursive import and an
AttributeError in trans_real.translation().
As far as I can tell, this is only a problem because my application is
referred to by settings.INSTALLED_APPS with a dotted name
('myproject.myapp'), which is treated differently from simple names in
trans_real.py:
102 def translation(language):
...
133 def _fetch(lang, fallback=None):
...
178 for appname in settings.INSTALLED_APPS:
179 p = appname.rfind('.')
180 if p >= 0:
181 app = getattr(__import__(appname[:p], {}, {},
[appname[p+1:]]), appname[p+1:])
182 else:
183 app = __import__(appname, {}, {}, [])
It looks like ignoring AttributeError on line 181 would solve this
problem without side-effects - except allowing incorrect values in
settings.INSTALLED_APPS, but that's caught when model files are loaded.
Should I open a ticket?
Malcolm Tredinnick
On Wed, 2008-09-10 at 10:17 +0200, Julien Demoor wrote:
> I'm having a problem with the change in r8120, due to changing Django's
> User model at runtime in an application's __init__ module.
>
> This worked well before r8120, but now it causes a lazy translation
> string to be evaluated, followed by a recursive import and an
> AttributeError in trans_real.translation().
[...]
> Should I open a ticket?
It depends. Do you think we should fix the bug?
Note that the real bug is that a lazy translation is being accessed at
all at that point. That's the thing we have to fix. Calling translations
during app importing is always going to have various dependency
problems, not just the one you noticed.
Regards,
Malcolm