Re: Automatic prefetching in querysets

2017-09-12 Thread Gordon Wrigley
This received some positive responses, so to help move the conversation along I have created a ticket and pull request. https://code.djangoproject.com/ticket/28586 https://github.com/django/django/pull/9064 Regards G On Tue, Aug 15, 2017 at 10:44 AM, Gordon Wrigley

Re: Automatic prefetching in querysets

2017-08-30 Thread Patryk Zawadzki
W dniu środa, 16 sierpnia 2017 14:48:54 UTC+2 użytkownik Luke Plant napisał: > > I completely agree that visibility of this problem is a major issue, and > would really welcome work on improving this, especially in DEBUG mode. One > option might be a method that replaces lazy loads with

Re: Automatic prefetching in querysets

2017-08-21 Thread Shai Berger
On Monday 21 August 2017 18:44:35 Tobias McNulty wrote: > On Sat, Aug 19, 2017 at 1:10 PM, Luke Plant wrote: > > This could work something like the way that ForeignKey `on_delete` works > > - you have options that are enumerated as constants, but in reality they > > are

Re: Automatic prefetching in querysets

2017-08-21 Thread Tobias McNulty
On Sat, Aug 19, 2017 at 1:10 PM, Luke Plant wrote: > This could work something like the way that ForeignKey `on_delete` works - > you have options that are enumerated as constants, but in reality they are > classes that embody the strategy to be used. We could have

Re: Automatic prefetching in querysets

2017-08-19 Thread Luke Plant
This could work something like the way that ForeignKey `on_delete` works - you have options that are enumerated as constants, but in reality they are classes that embody the strategy to be used. We could have something similar - `on_missing_relation=FETCH | WARN | ERROR | IGNORE ... ` Luke

Re: Automatic prefetching in querysets

2017-08-19 Thread Todor Velichkov
+1 to just add the queryset method it gives a fine granular level of control. Moreover when not used we could display warnings which can help people detect these O(n) query cases as Tom Forbes initially suggested. On Friday, August 18, 2017 at 9:08:11 AM UTC+3, Anssi Kääriäinen wrote: > > On

Re: Automatic prefetching in querysets

2017-08-18 Thread Collin Anderson
I like that idea - keep it a private API for now and make it a public API once people have used it a little bit. On Fri, Aug 18, 2017 at 4:01 AM, Shai Berger wrote: > On Friday 18 August 2017 09:08:11 Anssi Kääriäinen wrote: > > Maybe we should just add the queryset method.

Re: Automatic prefetching in querysets

2017-08-18 Thread Shai Berger
On Friday 18 August 2017 09:08:11 Anssi Kääriäinen wrote: > Maybe we should just add the queryset method. This is the smallest atomic > task that can be done. Even if there's only the queryset method available, > it's possible to enable prefetches per model by using a Manager. > I disagree on

Re: Automatic prefetching in querysets

2017-08-18 Thread Anssi Kääriäinen
On Thursday, August 17, 2017 at 11:30:45 PM UTC+3, Aymeric Augustin wrote: > > Hello, > > > On 17 Aug 2017, at 14:48, Luke Plant > wrote: > > > > On 16/08/17 23:17, Aymeric Augustin wrote: > >> It should kick in only when no select_related or prefetch_related is in >

Re: Automatic prefetching in querysets

2017-08-17 Thread Aymeric Augustin
Hello, > On 17 Aug 2017, at 14:48, Luke Plant wrote: > > On 16/08/17 23:17, Aymeric Augustin wrote: >> It should kick in only when no select_related or prefetch_related is in >> effect, to avoid interfering with pre-existing optimizations. It's still >> easy to

Re: Automatic prefetching in querysets

2017-08-17 Thread Collin Anderson
"turn it on/off per model" - I wonder if we just have a custom manager/mixin for the per model case. objects = AutoPrefetchRelatedManager(). The manager can return a queryset with prefetch_related(auto=True) or whatever already applied. On Thu, Aug 17, 2017 at 1:33 PM, Andrew Godwin

Re: Automatic prefetching in querysets

2017-08-17 Thread Andrew Godwin
Just some quick thoughts, as most of the points I would bring up have been discussed: - This should definitely be opt-in for the first release, it's too big a change to make opt-out straight away. - You should be able to turn it on/off per model, probably in Meta, not in a setting, and obviously

Re: Automatic prefetching in querysets

2017-08-17 Thread Malcolm Box
Hi, I think there's a potential to make this opt-in, and improve the out-of-box experience. Summarising the discussion, it seems that the rough consensus is that if we were building the ORM from scratch, then this would be entirely sensible behaviour (with necessary per-QS ways to disable) -

Re: Automatic prefetching in querysets

2017-08-17 Thread Gordon Wrigley
I'm not advocating either way on this, but it's probably worth noting that the context manager proposal is compatible with a queryset level optin/out and an object level optout. So you could for example have prefetch_related(auto=None) which can be explicitly set to True / False and takes it's

Re: Automatic prefetching in querysets

2017-08-16 Thread Josh Smeaton
I think there's a lot right with your suggestions here Shai. It delivers better default behaviour for new projects, does not affect existing deployments, and seems pretty easy to enable/disable selectively at any level of the stack. My only concern would be libraries leaning on this behaviour

Re: Automatic prefetching in querysets

2017-08-16 Thread Shai Berger
First of all, I think making the auto-prefetch feature available in some form is a great idea. As an opt-in, I would have made use of it on several occasions, and cut days of optimization work to minutes. It's true that this wouldn't achieve the best possible optimization in many cases, but it

Re: Automatic prefetching in querysets

2017-08-16 Thread Gordon Wrigley
Regarding 2, it does work for reverse one-to-one relations. On Wed, Aug 16, 2017 at 9:17 PM, Aymeric Augustin < aymeric.augus...@polytechnique.org> wrote: > On 15 Aug 2017, at 11:44, Gordon Wrigley wrote: > > I'd like to discuss automatic prefetching in querysets.

Re: Automatic prefetching in querysets

2017-08-16 Thread Aymeric Augustin
> On 15 Aug 2017, at 11:44, Gordon Wrigley wrote: > > I'd like to discuss automatic prefetching in querysets. Specifically > automatically doing prefetch_related where needed without the user having to > request it. Hello, I'm rather sympathetic to this proposal.

Re: Automatic prefetching in querysets

2017-08-16 Thread 'Tom Evans' via Django developers (Contributions to Django itself)
Is this opt-{in,out} considered to be a global flag, meant to be toggled on or off depending on whether it is an "expert" working on the project or not? I don't think that would be a good idea, almost all of our projects have a mix of skill levels, and people move from team to team on a regular

Re: Automatic prefetching in querysets

2017-08-16 Thread Gordon Wrigley
> It's also likely to require a significant patch The guts of it are actually surprisingly clean and small. That's probably a testament to some of the good decisions in the ORM architecture. Covering one2one in both directions, FK's in the forward direction, ignoring tests and various optin/out

Re: Automatic prefetching in querysets

2017-08-16 Thread Tom Forbes
Some quicker changes that have been brought up here could be implemented in the interim, like adding intelligent warnings. Someone brought up a great point about how the ORM hides most SQL terminology from the user, but still requires the knowledge of the difference between prefetch and select

Re: Automatic prefetching in querysets

2017-08-16 Thread Marc Tamlyn
As an opt in feature, it does sound quite interesting. If over the course of a few releases, it proves to be reliable and we don't get tons of support requests of it either not working, or causing the opposite problem, we can consider moving it to an opt out feature. This makes it easy for people

Re: Automatic prefetching in querysets

2017-08-16 Thread Luke Plant
Hi Josh, On 16/08/17 02:26, Josh Smeaton wrote: I believe we should be optimising for the **common** use case, not expecting everyone to be experts with the ORM. > If I can come up with a single example where it would significantly decrease performance (either memory usage or speed)

Re: Automatic prefetching in querysets

2017-08-16 Thread Anssi Kääriäinen
On Wednesday, August 16, 2017 at 2:27:11 PM UTC+3, Josh Smeaton wrote: > > It won't affect experienced users. They'll read the release notes, see >> that this change has been implemented, and either go and delete a bunch of >> prefetch_related() calls, grumble a bit and turn auto-prefetch off

Re: Automatic prefetching in querysets

2017-08-16 Thread Adam Johnson
> > I wouldn't want is to give free optimisations to non-ORM pros > Something like 99% of django projects are by non-ORM pros, it can be easy to forget that on a mailing list of experts. On 16 August 2017 at 12:27, Josh Smeaton wrote: > It won't affect experienced

Re: Automatic prefetching in querysets

2017-08-16 Thread Josh Smeaton
> > It won't affect experienced users. They'll read the release notes, see > that this change has been implemented, and either go and delete a bunch of > prefetch_related() calls, grumble a bit and turn auto-prefetch off globally > or just file it away as another fact they know about the

Re: Automatic prefetching in querysets

2017-08-15 Thread Alexander Hill
I think this is an excellent suggestion. It seems generally accepted in this thread that although there are cases where this would hurt performance, it would on average solve more problems than it creates. The debate seems to be more whether or not it's "right" for the ORM to behave in this

Re: Automatic prefetching in querysets

2017-08-15 Thread Cristiano Coelho
I would rather have warnings as well, adding more magical behavior is bad and might even degrade performance on some cases, automatically selecting a bunch of data that "might" be used is bad, and specially considering how slow python is, accidentally loading/building 1k+ objects when maybe

Re: Automatic prefetching in querysets

2017-08-15 Thread Curtis Maloney
The 2 goals of a famework: - protect you from the tedious things - protect you from the dangerous things N+1 queries would be in the 'dangerous' category, IMHO, and 'detecting causes of N+1 queries' is in the 'tedious'. If we can at least add some DEBUG flagged machinery to detect and warn

Re: Automatic prefetching in querysets

2017-08-15 Thread Josh Smeaton
I believe we should be optimising for the **common** use case, not expecting everyone to be experts with the ORM. > If I can come up with a single example where it would significantly decrease performance (either memory usage or speed) compared to the default (and I'm sure I can), then I would

Re: Automatic prefetching in querysets

2017-08-15 Thread Anthony King
Automatically prefetching is something I feel should be avoided. A common gripe I have with ORMs is they hide what's actually happening with the database, resulting in beginners-going-on-intermediates building libraries/systems that don't scale well. We have several views in a dashboard, where a

Re: Automatic prefetching in querysets

2017-08-15 Thread Adam Johnson
> > I agree with Marc here that the proposed optimizations are 'magical'. I > think when it comes to optimizations like these you simply cannot know in > advance whether doing extra queries is going to a be an optimization or a > pessimization. I think Django automatically fetching data from the

Re: Automatic prefetching in querysets

2017-08-15 Thread Adam Johnson
> > Adam/Gordon, I'm interested in hearing how these changes led you to > discovering stale prefetches? We removed all the manual prefetches in admin get_queryset() methods, added the auto_prefetch_related call, and regenerated the performance records from django-perf-rec tests that hit the

Re: Automatic prefetching in querysets

2017-08-15 Thread Sean Brant
I wonder if a solution similar to [1] from the rails world would satisfy this request. Rather then doing anything 'magical' we instead log when we detect things like accessing a related model that has not been pre-fetched. [1] https://github.com/flyerhzm/bullet On Tue, Aug 15, 2017 at 5:14 PM,

Re: Automatic prefetching in querysets

2017-08-15 Thread Luke Plant
I agree with Marc here that the proposed optimizations are 'magical'. I think when it comes to optimizations like these you simply cannot know in advance whether doing extra queries is going to a be an optimization or a pessimization. If I can come up with a single example where it would

Re: Automatic prefetching in querysets

2017-08-15 Thread Josh Smeaton
I'm in favour of *some* automated way of handling these prefetches, whether it's opt-in or opt-out there should be some mechanism for protection. Preferably with loud logging that directs users to the source of the automated hand-holding so they have an opportunity to disable or fine tune the

Re: Automatic prefetching in querysets

2017-08-15 Thread Adam Johnson
I'm biased here, Gordon was my boss for nearly three years, 2014-2016. I'm in favour of adding the auto-prefetching, I've seen it work. It was created around midway through last year and applied to our application's Admin interface, which was covered in tests with *django-perf-rec*. After adding

Re: Automatic prefetching in querysets

2017-08-15 Thread Gordon Wrigley
The warnings you propose would certainly be an improvement on the status quo. However for that to be a complete solution Django would also need to detect places where there are redundant prefetch_relateds. Additionally tools like the Admin and DRF would need to provide adequate hooks for

Re: Automatic prefetching in querysets

2017-08-15 Thread Gordon Wrigley
I didn't answer your questions directly. Sorry for the quoting but it's the easiest way to deal with a half dozen questions. > How would possible prefetches be identified? Wherever we currently automatically fetch a foreign key value. > What happens when an initial loop in a view requires one

Re: Automatic prefetching in querysets

2017-08-15 Thread Tom Forbes
Exploding query counts are definitely a pain point in Django, anything to improve that is definitely worth considering. They have been a problem in all Django projects I have seen. However I think the correct solution is for developers to correctly add select/prefetch calls. There is no general

Re: Automatic prefetching in querysets

2017-08-15 Thread Gordon Wrigley
In my current version each object keeps a reference to a WeakSet of the results of the queryset it came from. This is populated in _fetch_all and if it is populated then ForwardManyToOneDescriptor does a prefetch across all the objects in the WeakSet instead of it's regular fetching. On Tue, Aug

Re: Automatic prefetching in querysets

2017-08-15 Thread Collin Anderson
Hi Gordon, How is it implemented? Does each object keep a reference to the queryset it came from? Collin On Tue, Aug 15, 2017 at 2:44 PM, Gordon Wrigley wrote: > Sorry maybe I wasn't clear enough about the proposed mechanism. > > Currently when you dereference a

Re: Automatic prefetching in querysets

2017-08-15 Thread Gordon Wrigley
Sorry maybe I wasn't clear enough about the proposed mechanism. Currently when you dereference a foreign key field on an object (so 'choice.question' in the examples above) if it doesn't have the value cached from an earlier access, prefetch_related or select_related then Django will

Re: Automatic prefetching in querysets

2017-08-15 Thread Marc Tamlyn
Hi Gordon, Thanks for the suggestion. I'm not a fan of adding a layer that tries to be this clever. How would possible prefetches be identified? What happens when an initial loop in a view requires one prefetch, but a subsequent loop in a template requires some other prefetch? What about nested