Opened 15 years ago

Closed 12 years ago

#10498 closed Bug (fixed)

Passing ugettext_lazy to related object's create() doesn't work

Reported by: Malcolm Tredinnick Owned by: Jonas Obrist
Component: Database layer (models, ORM) Version: dev
Severity: Release blocker Keywords: dceu2011
Cc: anssi.kaariainen@… Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

As originally reported in #10491, doing this appears to fail (unconfirmed by me at the moment):

user.message_set.create(message=ugettext_lazy(u'xxx')) 

It should work.

Attachments (4)

10498.diff (2.9 KB ) - added by Jonas Obrist 13 years ago.
a fix as discussed with russ
10498-2.diff (2.8 KB ) - added by Claude Paroz 12 years ago.
Updated to latest trunk (r17503)
bench.txt (7.3 KB ) - added by Anssi Kääriäinen 12 years ago.
djangobench log between 1.3.1 and r17672
ticket_10498_alt.diff (4.8 KB ) - added by Anssi Kääriäinen 12 years ago.
Alternate way to fix Promise object handling

Download all attachments as: .zip

Change History (19)

comment:1 by Malcolm Tredinnick, 15 years ago

Triage Stage: UnreviewedAccepted

comment:2 by liangent, 15 years ago

note that the problem is:

something like <django.utils.functional.__proxy__ object at 0x0110ED10> shows when i'm trying to access messages in template context messages.

so i think there's some connection with #10491 and i put it in #10491 at first.

comment:3 by Chris Beaven, 13 years ago

Severity: Normal
Type: Bug

comment:4 by Jonas Obrist, 13 years ago

Easy pickings: unset
Owner: changed from nobody to Jonas Obrist
UI/UX: unset

comment:5 by Jonas Obrist, 13 years ago

Possible solution: In django.db.models.base.Model.init, check if 'val's that are set onto the model are django.utils.functional.Promise instances, and if so, force_unicode them before setting the attribute.

by Jonas Obrist, 13 years ago

Attachment: 10498.diff added

a fix as discussed with russ

comment:6 by Jonas Obrist, 13 years ago

Has patch: set
Keywords: dceu2011 added

by Claude Paroz, 12 years ago

Attachment: 10498-2.diff added

Updated to latest trunk (r17503)

comment:7 by Claude Paroz, 12 years ago

Triage Stage: AcceptedReady for checkin

Tested that performance are not affected:

Running benchmarks: django_test_postgresql:many_to_one
Control: Django 1.4a1 (in git branch master)
Experiment: Django 1.4a1 (in git branch 10498)

Running 'django_test_postgresql' benchmark ...
Min: 0.245787 -> 0.249683: 1.0159x slower
Avg: 0.256881 -> 0.257157: 1.0011x slower
Not significant
Stddev: 0.00550 -> 0.00291: 1.8910x smaller (N = 20)

Running benchmarks: django_test_sqlite:many_to_one
Control: Django 1.4a1 (in git branch master)
Experiment: Django 1.4a1 (in git branch 10498)

Running 'django_test_sqlite' benchmark ...
Min: 0.053780 -> 0.053740: 1.0007x faster
Avg: 0.054773 -> 0.053980: 1.0147x faster
Not significant
Stddev: 0.00186 -> 0.00040: 4.6798x smaller (N = 20)

comment:8 by Jannis Leidel, 12 years ago

Resolution: fixed
Status: newclosed

In [17641]:

Fixed #10498 -- Fixed using ugettext_lazy values when creating model instances. Thanks to Claude Paroz and Jonas Obrist.

comment:9 by Anssi Kääriäinen, 12 years ago

Cc: anssi.kaariainen@… added
Resolution: fixed
Severity: NormalRelease blocker
Status: closedreopened

While working on #17030 I noticed the isinstance(val, Proxy) calls in the model.__init__. I did some benchmarking, and confirmed the patch applied in this ticket causes some performance regressions. Using djangobench:

Running 'query_all' benchmark ...
Min: 0.021516 -> 0.024808: 1.1530x slower
Avg: 0.026032 -> 0.030944: 1.1887x slower
Significant (t=-5.668286)
Stddev: 0.00516 -> 0.00696: 1.3479x larger (N = 100)

Running 'query_all_multifield' benchmark ...
Min: 0.043492 -> 0.053683: 1.2343x slower
Avg: 0.049113 -> 0.061782: 1.2580x slower
Significant (t=-11.048640)
Stddev: 0.00686 -> 0.00919: 1.3382x larger (N = 100)

When repeating the query_all_multifield the result seems to stay around 20% slower (mentioning this because for some reason or other djangobench hasn't been that reliable).

I am going to reopen and mark this ticket as a release blocker. The reason is that I think this should be reconsidered in the light of the 20% regression for fetching objects from the db. It seems pretty clear that the performance regression wasn't taken in account when doing the commit, so reconsidering this in light of the regression should be done before 1.4. I am not saying that this must be reverted, just that reconsidering the value of the patch with the new info available should be done.

My initial feeling is that the patch should be reverted, as using ugettext_lazy in models doesn't sound more important than the 20% regression. There are also a couple of other ways forward:

  • revert just the part of the commit which touches the args based setattr "fast path" loop. This leaves a bit of inconsistency, but args based __init__ + ugettext_lazy isn't that common really.
  • check isinstance(Promise) when saving the model. That way .create() would work with ugettext lazy objects. I think there is still an unfixed problem if you just assign a Promise object to some field, then save (no, I haven't tested this).

Attached is the full benchmark log. (Note: most of the other results aren't repeatable. url_resolving regression is real, though).

Last edited 12 years ago by Anssi Kääriäinen (previous) (diff)

by Anssi Kääriäinen, 12 years ago

Attachment: bench.txt added

djangobench log between 1.3.1 and r17672

comment:10 by Anssi Kääriäinen, 12 years ago

Triage Stage: Ready for checkinDesign decision needed

The attached patch does the Promise -> unicode conversion much later in the saving cycle. It fixes numerous other ways to pass Promise objects to the DB layer, and in addition the slowdown to model init is now removed.

by Anssi Kääriäinen, 12 years ago

Attachment: ticket_10498_alt.diff added

Alternate way to fix Promise object handling

comment:11 by Claude Paroz, 12 years ago

Sorry about my benchmarks in comment:7, I was probably not targeting enough the object creation part in the executed tests. I can confirm Anssi's benchmarks, even if for query_all tests, I only get about 7% slower results.

comment:12 by Anssi Kääriäinen, 12 years ago

It would be nice to have continuous benchmarking system...

The 5-20% slowdown is somewhat important to deal with, but now that I look at this, the _real_ reason to fix this in the subqueries module is that it is a better place to do the fix. It fixes some situations the original patch doesn't fix. I wonder if the proper place would be in database adapters actually. I don't think now is the time to do that change anyways.

My opinion is that the alternate patch should be committed both because it fixes a performance regression, and also because it fixes more cases to begin with.

comment:14 by Jonas Obrist, 12 years ago

The new patch looks good to me. Seems reasonable and djangobench reports it fixes the performance regression.

comment:15 by Jannis Leidel, 12 years ago

Resolution: fixed
Status: reopenedclosed

In [17698]:

Fixed #10498 (again) -- Made sure the improvements done in r17641 have a smaller impact on speed. Thanks to Anssi Kääriäinen for the patch and Jonas Obrist for reviewing.

Note: See TracTickets for help on using tickets.
Back to Top