#24215 closed Cleanup/optimization (fixed)
Refactor of lazy model operations
Reported by: | Alex Hill | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | cmawebsite@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
I dealt with add_lazy_relation()
a few months ago working on Mezzanine and I thought it could use some detangling.
Now that the list of pending operations is stored in the Apps
class, it makes sense to put the related methods on that class as well. Running a function when a model is loaded seems an appropriate job for the app registry object.
I've introduced a more generic API, whereby a user-supplied function can be called once any number of models are ready, with those freshly-loaded models as its arguments (plus optional kwargs), a helper function for related models, and the old add_lazy_relation()
reimplemented in terms of the new API with a deprecation warning.
New pull request, now targeting master rather than 1.8, at https://github.com/django/django/pull/4130
Change History (27)
comment:1 by , 10 years ago
Description: | modified (diff) |
---|
comment:2 by , 10 years ago
Cc: | added |
---|---|
Version: | 1.8alpha1 → master |
comment:3 by , 10 years ago
Hey Collin, thanks for having a look at this. I'll try to explain myself a bit better.
add_lazy_relation
is essentially "do something with this related field and its two related models when both models are ready". That's just a special case of the more general "do something when all of these n models are ready", so I've reimplemented it as such.
The important part is waiting until models are loaded, and running some code at that time. So I've pulled that part out and made it as generic as possible: you provide a callback and any number of models or model names, and when they're all loaded, your code runs with access to the actual model classes.
I reckon that's a generally useful piece of functionality for Apps to provide. So that's the first part.
The second part is to introduce a new two-model wrapper function, lazy_related_operation
, to replace add_lazy_relation
itself. A wrapper function like this remains useful as a place to resolve "self" references and model names without app_labels, and for identifying the relevant apps object. There are a few reasons I'd like to deprecate add_lazy_relation
:
- it doesn't do what it says - it doesn't add lazy relations at all, the callback you supply does that (or doesn't).
- the argument order is inconsistent. it takes
class, field, related_class, callback
but the callback's signature has to befield, related_class, class
. The API introduced in this patch is consistent with most other partial/lazy functional APIs:function, *args, **kwargs
. - it still requires the field argument, which can be handled by a more general kwargs implementation (or simply by closing over it)
- it's inconsistent with the new underlying API
add_lazy_relation
is still there with its existing signature in case anybody's using it outside of Django, just reimplemented using the new API and a deprecation warning.
The test suite passes with this patch in place, but I'd gladly write some more tests that demonstrate the new API in a narrower way.
Alex
comment:4 by , 10 years ago
Hi Alex,
Do you have example of why you would need two models? What real-world problem are you trying to solve? :)
I think add_lazy_relations is one of those private APIs that may have a lot of use, so if we're going to change it I just want to make sure it's worth it. :)
Thanks,
Collin
comment:5 by , 10 years ago
Hey Collin,
Sorry about the delay, I didn't get an email notification of your comment. Thanks for replying.
I use it this in Mezzanine to wait on both a user model and a profile model which are both string references. That usage is more defensive than it is strictly necessary, but it demonstrates the point. I leave more creative uses to future generations. :)
I hear you about add_lazy_relation
being used outside of Django (I do it). If it is used a lot, I see that as a good reason to clean up and document the API, though with an extended deprecation timeline – pave the cowpaths. I've done a quick search on GitHub but most of the results are Django builds in other people's repos, so it's not all that helpful. Perhaps I should poll django-users to get a sense of how widely it's used?
I propose:
- Move the core generic functionality into the Apps registry and reimplement
add_lazy_relation
accordingly as in this PR. - Include
Apps.lazy_model_operation
in the public app registry API and document it here.
Then either:
- Scrap the new
lazy_related_operation
helper and don't deprecateadd_lazy_relation
at all – simply re-implement it while retaining the existing signature, or - Proceed as per my plan but push removal of
add_lazy_relation
back to the next LTS. Possibly go the whole hog and documentlazy_related_operation
as well.
Now wishing this had come up a bit earlier and snuck into 1.8!
Cheers,
Alex
comment:6 by , 10 years ago
I like the general idea of refactoring the add_lazy_relation functionality. I think we also want to guard against unhandled lazy relations, which seems to be missing from current code. So, +1 from me.
I'm not going to set this as accepted, as I'm not sure if we need this as public API (I'm not against that either, I just don't know).
comment:7 by , 10 years ago
Hi Anssi,
Thanks for your feedback.
I have made some changes to the patch, notably:
- lazy_related_operation now takes multiple related model arguments.
- I've simplified and removed some now-unnecessary type checks in RelatedField.contribute_to_class and create_many_to_many_intermediary_model (the latter now takes advantage of the first bullet point)
- I've made a change to how SQLite schema alterations work due to a bit of friction in the above. This is a small change in terms of code and I believe for the better, but will want some eyes on it.
Regarding unhandled relations: if someone waits for a model that doesn't exist the signal will never fire. I suppose at the end of the app registry's loading phase would be a good place to check for those. Issuing a warning might be the go, as it's always possible (though unlikely) that the user intends to define a model later...
While looking into this, ran into #21175. Currently abstract models don't fire class_prepared, so lazy operations involving abstract models remain unhandled. In practise it doesn't matter because their concrete children work fine, but it would be worth fixing.
Cheers,
Alex
comment:8 by , 10 years ago
Actually, it's a bit more complicated than just having abstract models fire class_prepared.
We try get_registered_model to see if a model is ready or not, which will always fail with abstract models because they don't appear in the registry. In the case that the abstract model really isn't ready, the callback will still fire when it's constructed (assuming #21175 is fixed), but only the first time.
Is there a reason the app registry doesn't keep track of abstract models?
Note these inconsistencies are present in Django now, they aren't introduced by this patch. Again in practise it doesn't really matter – in fact you get errors if some of the lazy operations are run with abstract models. However if we want to be able to provide warnings for unhandled lazy operations, we have to sort something out with abstract models or they'll raise false alarms.
Cheers,
Alex
comment:9 by , 10 years ago
As pointed out by Carl on the mailing list abstract model should only be considered as field and method placeholders.
The real issue here is related fields declared on abstract models that add themselves to the apps _pending_lookups
.
See this PR.
comment:10 by , 10 years ago
Yep, after my last post in django-developers I basically did the same thing as you in my pull request, just hadn't updated this ticket yet (see commit here). Good to see we're thinking along the same lines.
comment:11 by , 10 years ago
Incidentally, what do you think of #21175 - having abstract models fire class_prepared?
I'm thinking that in keeping with what we're talking about here, maybe they shouldn't.
comment:12 by , 10 years ago
We're talking about deprecating the class_prepared
signal altogether (#24313) so this should closed in the process.
comment:14 by , 10 years ago
I'm working on refactoring the way relation fields are implemented in Django (#24317). One part of the work is to store directly the remote parts of fields in the remote model's _meta. This isn't that complex to do when first creating the models, but when doing partial re-renders for migrations we need some way to re-fire the equivalent of class-prepared signal. The ticket contains a larger explanation of what I'm planning to do. The last point in the plan is the troublesome case. Ideas welcome!
comment:15 by , 10 years ago
comment:17 by , 10 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:18 by , 10 years ago
PR now in much better shape thanks to review from Collin, Anssi, Simon and Carl. Notably:
- I'm no longer using the
class_prepared
signal. Instead,Apps.do_pending_lookups
is called directly at the end ofApps.register_model
. Apps.lazy_model_operation
no longer accepts keyword arguments. Functions passed to it should expect a number of models as positional arguments, and nothing else.- The
lazy_related_operation
convenience wrapper still supports keyword arguments, but they're applied to the supplied function withpartial
before it's passed toApps.lazy_model_operation
.
comment:19 by , 10 years ago
Description: | modified (diff) |
---|
comment:20 by , 10 years ago
I've rebased this on master - new pull request at https://github.com/django/django/pull/4130
comment:21 by , 10 years ago
Current status of this issue: the main pull request, #4130 has been reviewed extensively but is waiting on another pull request, #4122, which makes some changes to SQLite schema alterations.
My last change to the SQLite schema alteration branch explicitly disables FK constraints during schema alterations in order to guarantee the expected behaviour. This change hasn't been reviewed yet; if anybody has a few minutes, I would really appreciate a review: https://github.com/django/django/pull/4122
Cheers,
Alex
comment:22 by , 10 years ago
Patch needs improvement: | set |
---|
comment:23 by , 10 years ago
Patch needs improvement: | unset |
---|
PR4241 is in and I've rebased on top of that and squashed.
Hi, Thanks for the patch. It's not super clear to me what problem you're trying to solve.
A test or simpler pull request could make it more clear.