Opened 8 years ago

Closed 5 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: master
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 5 years ago.
a fix as discussed with russ
10498-2.diff (2.8 KB) - added by Claude Paroz 5 years ago.
Updated to latest trunk (r17503)
bench.txt (7.3 KB) - added by Anssi Kääriäinen 5 years ago.
djangobench log between 1.3.1 and r17672
ticket_10498_alt.diff (4.8 KB) - added by Anssi Kääriäinen 5 years ago.
Alternate way to fix Promise object handling

Download all attachments as: .zip

Change History (19)

comment:1 Changed 8 years ago by Malcolm Tredinnick

Triage Stage: UnreviewedAccepted

comment:2 Changed 8 years ago by liangent

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 Changed 6 years ago by Chris Beaven

Severity: Normal
Type: Bug

comment:4 Changed 5 years ago by Jonas Obrist

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

comment:5 Changed 5 years ago by Jonas Obrist

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.

Changed 5 years ago by Jonas Obrist

Attachment: 10498.diff added

a fix as discussed with russ

comment:6 Changed 5 years ago by Jonas Obrist

Has patch: set
Keywords: dceu2011 added

Changed 5 years ago by Claude Paroz

Attachment: 10498-2.diff added

Updated to latest trunk (r17503)

comment:7 Changed 5 years ago by Claude Paroz

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 Changed 5 years ago by Jannis Leidel

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 Changed 5 years ago by Anssi Kääriäinen

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 5 years ago by Anssi Kääriäinen (previous) (diff)

Changed 5 years ago by Anssi Kääriäinen

Attachment: bench.txt added

djangobench log between 1.3.1 and r17672

comment:10 Changed 5 years ago by Anssi Kääriäinen

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.

Changed 5 years ago by Anssi Kääriäinen

Attachment: ticket_10498_alt.diff added

Alternate way to fix Promise object handling

comment:11 Changed 5 years ago by Claude Paroz

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 Changed 5 years ago by Anssi Kääriäinen

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 Changed 5 years ago by Jonas Obrist

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

comment:15 Changed 5 years ago by Jannis Leidel

Resolution: fixed
Status: reopenedclosed

In [17698]:

(The changeset message doesn't reference this ticket)

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