Code

Opened 5 years ago

Closed 2 years ago

#10498 closed Bug (fixed)

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

Reported by: mtredinnick Owned by: ojii
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 ojii 3 years ago.
a fix as discussed with russ
10498-2.diff (2.8 KB) - added by claudep 2 years ago.
Updated to latest trunk (r17503)
bench.txt (7.3 KB) - added by akaariai 2 years ago.
djangobench log between 1.3.1 and r17672
ticket_10498_alt.diff (4.8 KB) - added by akaariai 2 years ago.
Alternate way to fix Promise object handling

Download all attachments as: .zip

Change History (19)

comment:1 Changed 5 years ago by mtredinnick

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 5 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 3 years ago by SmileyChris

  • Severity set to Normal
  • Type set to Bug

comment:4 Changed 3 years ago by ojii

  • Easy pickings unset
  • Owner changed from nobody to ojii
  • UI/UX unset

comment:5 Changed 3 years ago by ojii

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 3 years ago by ojii

a fix as discussed with russ

comment:6 Changed 3 years ago by ojii

  • Has patch set
  • Keywords dceu2011 added

Changed 2 years ago by claudep

Updated to latest trunk (r17503)

comment:7 Changed 2 years ago by claudep

  • Triage Stage changed from Accepted to Ready 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 2 years ago by jezdez

  • Resolution set to fixed
  • Status changed from new to closed

In [17641]:

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

comment:9 Changed 2 years ago by akaariai

  • Cc anssi.kaariainen@… added
  • Resolution fixed deleted
  • Severity changed from Normal to Release blocker
  • Status changed from closed to reopened

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.

Version 0, edited 2 years ago by akaariai (next)

Changed 2 years ago by akaariai

djangobench log between 1.3.1 and r17672

comment:10 Changed 2 years ago by akaariai

  • Triage Stage changed from Ready for checkin to Design 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 2 years ago by akaariai

Alternate way to fix Promise object handling

comment:11 Changed 2 years ago by claudep

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 2 years ago by akaariai

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 2 years ago by ojii

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

comment:15 Changed 2 years ago by jezdez

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.