Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#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 Alex Hill)

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 Alex Hill, 9 years ago

Description: modified (diff)

comment:2 by Collin Anderson, 9 years ago

Cc: cmawebsite@… added
Version: 1.8alpha1master

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.

Last edited 9 years ago by Collin Anderson (previous) (diff)

comment:3 by Alex Hill, 9 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 be field, 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 Collin Anderson, 9 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 Alex Hill, 9 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 deprecate add_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 document lazy_related_operation as well.

Now wishing this had come up a bit earlier and snuck into 1.8!

Cheers,
Alex

Last edited 9 years ago by Alex Hill (previous) (diff)

comment:6 by Anssi Kääriäinen, 9 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 Alex Hill, 9 years ago

Hi Anssi,

Thanks for your feedback.

I have made some changes to the patch, notably:

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 Alex Hill, 9 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

Last edited 9 years ago by Alex Hill (previous) (diff)

comment:9 by Simon Charette, 9 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 Alex Hill, 9 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 Alex Hill, 9 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 Simon Charette, 9 years ago

We're talking about deprecating the class_prepared signal altogether (#24313) so this should closed in the process.

comment:13 by Alex Hill, 9 years ago

That sounds sensible. Thanks for bringing that ticket to my attention.

comment:14 by Anssi Kääriäinen, 9 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 Markus Holtermann, 9 years ago

Hey Anssi,

as part of #24225 and #24264 I re-implemented the model reloading process in PR 4097. For now it recursively looks at a model's references, removes all of them from the app registry and adds them again. In case that might be worth knowing.

comment:16 by Simon Charette <charette.s@…>, 9 years ago

In 9239f1dda7b94f53d21efb8b5e4d056e24f4e906:

Refs #24215 -- Prevented pending lookup pollution by abstract models.

comment:17 by Tim Graham, 9 years ago

Triage Stage: UnreviewedAccepted

comment:18 by Alex Hill, 9 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 of Apps.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 with partial before it's passed to Apps.lazy_model_operation.

comment:19 by Alex Hill, 9 years ago

Description: modified (diff)

comment:20 by Alex Hill, 9 years ago

I've rebased this on master - new pull request at https://github.com/django/django/pull/4130

comment:21 by Alex Hill, 9 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

Last edited 9 years ago by Alex Hill (previous) (diff)

comment:22 by Tim Graham, 9 years ago

Patch needs improvement: set

PR #4122 is merged. I left some minor comments on #4130, but suggest we wait until #4241 to avoid creating more work for Anssi.

comment:23 by Alex Hill, 9 years ago

Patch needs improvement: unset

PR4241 is in and I've rebased on top of that and squashed.

comment:24 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: newclosed

In 720ff74:

Fixed #24215 -- Refactored lazy model operations

This adds a new method, Apps.lazy_model_operation(), and a helper function,
lazy_related_operation(), which together supersede add_lazy_relation() and
make lazy model operations the responsibility of the App registry. This
system no longer uses the class_prepared signal.

comment:25 by Tim Graham <timograham@…>, 9 years ago

In 25c157e4:

Refs #24215 -- Improved error message for unhandled lazy model operations.

comment:26 by Tim Graham <timograham@…>, 9 years ago

In 7506616f:

Refs #24215 -- Fixed Python 3.5 compatiblity for unhandled lazy ops error.

comment:27 by Tim Graham <timograham@…>, 7 years ago

In b2ffbb00:

Refs #24215 -- Removed add_lazy_relation() per deprecation timeline.

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