[Django] #22841: ModelChoiceField does not make it easy to reuse querysets
Django
---------------------------------------+------------------------
Reporter: mjtamlyn | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
---------------------------------------+------------------------
`ModelChoiceField` is designed to aggressively ensure that the queryset is
always copied and executed anew each time. This is generally a good idea,
but it has performance implications, especially where formsets of
modelforms are concerned. The situation is not restricted to formsets
however, there are other use cases where you may already have the executed
queryset you need for the form within that request/response cycle.
Here's a simple example of a view and form with the API I might like to
see.
{{{
# views.py
def book_create(request):
categories = Category.objects.all()
if request.method == 'POST':
form = BookForm(data=request.POST, categories=categories)
if form.is_valid():
form.save()
return HttpResponseRedirect(reverse('book_list'))
else:
form = BookForm(categories=categories)
context = {
'categories': categories,
'form': form,
}
return render('book_form.html', context)
# forms.py
class BookForm(forms.ModelForm):
class Meta:
model = Book
fields = ['name', 'category']
def __init__(self, categories, **kwargs):
super(BookForm, self).__init__(**kwargs)
self.fields['category'].evaluated_queryset = categories
}}}
So we have a view to create a book, but that view has the list of
categories in the context as it also includes a by-category navigation in
a sidebar. As a result, in order to render the view we currently have to
execute `Category.objects.all()` twice - once to render the navigation and
once for the form.
I have introduced a new proposed API to the `ModelChoiceField`
(`form.fields['category']` in the example), currently called
`evaluated_queryset`. This will be used by the `ModelChoiceIterator`
*without* calling `.all()` on it, allowing the same queryset cache to be
used twice within the view.
----
The current "best approach" for doing this that I've found looks as
follows:
{{{
class BookForm(forms.ModelForm):
# ...
def __init__(self, categories, **kwargs):
super(BookForm, self).__init__(**kwargs)
iterator = ModelChoiceIterator(self.fields['category'])
choices = [iterator.choice(obj) for obj in categories]
self.fields['category'].choices = choices
}}}
Whilst this is functional, it is not a particularly nice API. If we are
happy with it as the correct pattern, we should document it, but at
present `ModelChoiceIterator` is not documented, and it probably shouldn't
be.
----
Possible more general improvements which become possible with a feature
like this:
- Automatic sharing of querysets between identical forms in a formset
- Similarly, if the queryset has been executed then we can check inside it
instead of doing the additional `.get()` query on data validation. This
has a small performance gain on write in certain circumstances - in
particular where you have a formset with 10+ fields, loading the full
queryset once will be more efficient than doing 10 `.get()` queries.
- Inlines and list editable in the admin could use this feature for
significant performance improvements
--
Ticket URL: <https://code.djangoproject.com/ticket/22841>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: mjtamlyn | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bmispelon):
* stage: Unreviewed => Accepted
Comment:
+1
--
Ticket URL: <https://code.djangoproject.com/ticket/22841#comment:1>
Django
Reporter: mjtamlyn | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by loic84):
* cc: loic84 (added)
Comment:
+1.
I'm not sure if we document that form instances should be short-lived
(i.e. the span of a request), but if we don't we should and fixing this
ticket would make it a requirement.
--
Ticket URL: <https://code.djangoproject.com/ticket/22841#comment:2>
Django
Reporter: mjtamlyn | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by kunitoki):
i encounter this in every app i'm coding that has lot of foreign keys.
looking at the produced sql queries there are tons of equals queries just
to populate the same identical dropdowns. should definately be addressed
as it is a huge performance bottleneck !
+1
--
Ticket URL: <https://code.djangoproject.com/ticket/22841#comment:3>
Django
Reporter: mjtamlyn | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by tomek):
+1
I see this is quite naive solution, but couldn't we just eliminate
''all()'' like that?
{{{
#!python
class ModelChoiceIterator(object):
def __iter__(self):
if self.field.empty_label is not None:
yield ("", self.field.empty_label)
if self.field.cache_choices:
if self.field.choice_cache is None:
self.field.choice_cache = [
self.choice(obj) for obj in self.queryset.all()
]
for choice in self.field.choice_cache:
yield choice
else:
for obj in self.queryset.all():
yield self.choice(obj)
}}}
{{{
#!python
class ModelChoiceIterator(object):
def __iter__(self):
if self.field.empty_label is not None:
yield ("", self.field.empty_label)
if self.field.cache_choices:
if self.field.choice_cache is None:
self.field.choice_cache = [
self.choice(obj) for obj in self.queryset.all()
]
for choice in self.field.choice_cache:
yield choice
else:
try:
for obj in self.queryset:
yield self.choice(obj)
except TypeError:
for obj in self.queryset.all():
yield self.choice(obj)
}}}
As I said, naive... so I'll be glad if someone points out why not this
way.
--
Ticket URL: <https://code.djangoproject.com/ticket/22841#comment:4>
Django
Reporter: mjtamlyn | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by robin):
* cc: robinchew@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/22841#comment:5>
Django
Reporter: mjtamlyn | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by mdentremont):
* cc: mattdentremont@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/22841#comment:6>
Django
Reporter: Marc Tamlyn | Owner: Michael Vasiliou
Type: New feature | Status: assigned
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Michael Vasiliou):
* owner: nobody => Michael Vasiliou
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/22841#comment:7>
Django
Reporter: Marc Tamlyn | Owner: (none)
Type: New feature | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* cc: Carlton Gibson (added)
* owner: Michael Vasiliou => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/22841#comment:8>
Django
Reporter: Marc Tamlyn | Owner: (none)
Component: Forms | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by powderflask):
+1
I run into this a fair bit with ModelFormsets that include
ModelChoiceFields. Work arounds are clunky and largely undocumented.
Thanks to anyone who has a clever idea.
--
Ticket URL: <https://code.djangoproject.com/ticket/22841#comment:9>
Django
Reporter: Marc Tamlyn | Owner: (none)
Component: Forms | Version: master
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: new => closed
* resolution: => wontfix
Comment:
I gave a talk on this exact topic for DjangoCon Europe 2020. It's only 20
minutes. I show examples for a DRF serializer form, a regular form with
django-filter and a FormSet via the admin using `list_editable`
The video is here: [https://www.youtube.com/watch?v=e52S1SjuUeM Choose and
Choose Quickly: Optimizing ModelChoiceField]
* We can't drop the `all()` calls, since they're explicitly there to avoid
reusing cached data. (From the talk: ''"There's only one thing worse that
slow data, and that's wrong data."'')
* I don't think an additional `evaluated_queryset` attribute is necessary
because (at any place you have that code in hand) setting `choices`
directly is no more work (certainly not enough to merit extra API)
* Setting `choices` is exactly the intended API here. There's not an exact
optimization pattern documented but, `choices` has been ≈forever, and
`ModelChoiceIterator` has been documented since
5da85ea73724d75e609c5ee4316e7e5be8f17810.
TBH I don't think one needs to bring `ModelChoiceIterator` into play. The
example I use in the talk is just:
{{{
author_choices =
list(forms.ModelChoiceField(Author.objects.all()).choices)
}}}
Because I'm doing this outside of the form context, in order to provide to
the form later:
{{{
self.fields["author"].choices = author_choices
}}}
The example in the description, purely inside `BookForm.__init__()` could
well just be:
{{{
self.fields['category'].choices = list(self.fields['category'].choices)
}}}
You evaluate once, setting the output for reuse later.
--
Ticket URL: <https://code.djangoproject.com/ticket/22841#comment:10>