Implement support for `<QuerySet>.as_manager()` #1025

flaeppe · 2022-06-27T12:09:45Z

I've reorganised and extended dynamic manager class generation to support <QuerySet>.as_manager().

This also renames the dynamically generated manager types to align with what those class names will be during runtime, set by Django. See here: https://github.com/django/django/blob/b2eff16806057095c7dd3daa9402ad615e51627f/django/db/models/manager.py#L109-L110
That means that reveal_type(<Model>.<manager>) should output the same type name as type(<Model>.<manager>) (unless there's collisions etc etc.)

Related issues

Closes: #324

sobolevn

Looks good to me. Awesome work.
But, I want to have another look from someone :)

README.md

flaeppe · 2022-06-28T08:22:59Z

Did you have anyone in mind for a second pair of eyes? Or did you mean for me to ask someone?

sobolevn · 2022-06-28T11:27:12Z

@mkurnikov or @intgr or @syastrov

intgr · 2022-06-29T10:46:07Z

Sorry, mypy plugin code is entirely foreign to me and I don't have time right now to delve into it.

ljodal · 2022-06-29T12:37:46Z

Seems like you managed to implement support for creating these managers inline, does that mean that it's technically possible to support Manager.from_queryset(QuerySet) inline as well?

Oh and I'd be happy to review this, but I'm kinda swamped for the next few days so I might not get to it until Friday.

flaeppe · 2022-06-29T12:51:07Z

Seems like you managed to implement support for creating these managers inline, does that mean that it's technically possible to support Manager.from_queryset(QuerySet) inline as well?

Oh and I'd be happy to review this, but I'm kinda swamped for the next few days so I might not get to it until Friday.

Yes, it should be technically possible (I'm assuming class level/definition, when you say inline) as long as we're not talking about the "full" call Manager.from_queryset(QuerySet)(). But I opted out of supporting it here, since I thought it felt a bit strange. Reasoning being that unwrapping the non-dynamic version would look like something below:

class MyModel(models.Model):
    class ManagerFromMyQuerySet(...):
        ...
    
    objects = ManagerFromMyQuerySet()

Felt more natural to keep force it outside of the model class definition.

ljodal · 2022-06-29T13:09:21Z

Seems like you managed to implement support for creating these managers inline, does that mean that it's technically possible to support Manager.from_queryset(QuerySet) inline as well?
Oh and I'd be happy to review this, but I'm kinda swamped for the next few days so I might not get to it until Friday.

[...] as long as we're not talking about the "full" call Manager.from_queryset(QuerySet)(). [...]

This is what I wanted. All our models currently do objects = MyManager.from_queryset(MyQuerySet)(), so I'm having to go through all of them and move the manager definitions outside of the models, which feels a bit pointless, but oh well.

I'd forgotten about that extra call to initialise the model. I assume that causes mypy to not call get_dynamic_class_hook on the plugin and that's the problem with supporting that? Wonder if it would be possible to still support this by using different hooks. Something to dive into when I have time I guess

flaeppe · 2022-06-29T15:38:52Z

... I assume that causes mypy to not call get_dynamic_class_hook on the plugin and that's the problem with supporting that? ...

Yes, exactly, for clarity, the reason it wont work with inlining the full call, seen below, is that it wont trigger the get_dynamic_class_hook

class MyModel(models.Model):
    objects = Manager.from_queryset(MyQuerySet)()

... This plugin hook is called for every assignment to a simple name where right hand side is a function call ...

Ref: https://mypy.readthedocs.io/en/stable/extending_mypy.html#current-list-of-plugin-hooks (docs for get_dynamic_class_hook)

The more fortunate case for .as_manager is that it returns a manager instance, while .from_queryset returns a manager type.

sobolevn · 2022-06-30T07:21:17Z

There are conflicts right now 😉

flaeppe · 2022-06-30T08:10:18Z

There are conflicts right now 😉

Hehe, yeah. Think it's resolved now

mypy_django_plugin/transformers/managers.py

ljodal

The changes look very nice, I've just added some minor nits here and there. I am however seeing this when running this on our largest Django project at work:

project/users/forms/normal.py: error: INTERNAL ERROR: maximum semantic analysis iteration count reached

I'm not sure what's causing this (we're subclassing some Django builtin forms etc), but I assume it's a defer() call somewhere. I can dig into that tomorrow if you need help reproducing it.

mypy_django_plugin/main.py

ljodal · 2022-06-30T20:19:58Z

mypy_django_plugin/transformers/managers.py

+    # type won't be defined on variable level.
+    var = Var(
+        name=ctx.name,
+        type=TypeType(Instance(new_manager_info, [AnyType(TypeOfAny.from_omitted_generics)])),


Why is this Any? Shouldn't we take the model from either the queryset or the manager (and potentially validate that they match)?

I'm not sure, I'm kinda split on if we should display a model here. Since this is when we're outside of a model class definition, e.g.

MyManager = Manager.from_queryset(MyQuerySet)

What I'm thinking about here is what the manager type is during runtime, model won't be set until the model metaclass is called. Which is basically inside model class definition. And, while I have never used it like that, same manager could be used on multiple models..

But perhaps you're right, maybe we should require model arguments for both queryset and manager to match. Since we're basically merging those two arguments into one and the model argument exists for manager and queryset types.

Also, I think AddManagers will change the model argument if called on a different model? But perhaps that should also change?

I was thinking it would make sense to inspect the generics of the manager and querysets (if possible). If they are different an error message should probably be emitted and in other case use the class from either?

And I'd think that if AddManagers sees a model argument that's not compatible with the one on the manager then an error should be emitted?

I'm okay with not adding this in this PR, but that's at least how I would expect this to behave. I see that it's really an edge case and probably not a big problem.

Yeah, I've been looking in to exactly that. And it seems possible if we for .from_queryset traverse .bases (as those are types.Instance, we'll get access to Instance.args) and compare model arguments, then on mismatch emit an error.

This should probably happen for both .from_queryset and AddManagers, as you say.

There's a case left to decide on, when model arguments aren't populated for Queryset or Manager. Either:

We emit an error and enforce population of the model arg or,

We then allow it to be any model, thus just populate it with whatever we got

I'm leaning towards the latter, since one can switch on enforcing via mypy config: disallow_any_generics = true.

For the first case, we could then establish some sort of "auto-fill". Depending on if 1 of the model arguments are populated, if both are missing .from_queryset results in ManagerFromQueryset[Any] while AddManagers just populates it with the model it's processing.

This is basically the table:

Manager and queryset model argument populated -> error if mismatch for both .from_queryset and AddManagers

Only manager model argument populated -> Populate queryset model argument with manager model argument

Only queryset model argument populated -> Populate manager model argument with queryset model argument

No argument populated -> .from_queryset sets Any and AddManagers populates with the model it's processing.

I think this aligns with when using Django builtins e.g. BaseManager.from_queryset(CustomQuerySet), since otherwise we'll enforce people to write custom managers to populate the argument, even though they only have/need a queryset defined. Like this:

class SuperFluousManager(BaseManager[MyModel]): ...

What do you think?

Yeah, that sounds exactly like what I'd expect 👍

I've been looking in to this for a bit and think we should push for it to a future PR, as it's quite an isolated addition but generates quite a bit of lines. And the changes we have here are already large enough.

I've pushed progress to a PR here: flaeppe#29

mypy_django_plugin/transformers/managers.py

flaeppe · 2022-07-01T07:00:39Z

I'm not sure what's causing this (we're subclassing some Django builtin forms etc), but I assume it's a defer() call somewhere. I can dig into that tomorrow if you need help reproducing it.

Thanks for the review. It'd be very helpful if you could find a repro, I have no idea where to start looking for it really.

ljodal · 2022-07-01T09:36:58Z

I dug a bit into the mypy code yesterday and from what I could see the problem isn't guaranteed to be in the file it reports, so I think this is related to something else. The curious thing is that we don't have any .as_manager() calls in our codebase anymore, so either it's from a dependency or it's related to the other changes in this PR.

I'll try to extract just the "refactoring" bit of this PR and see if it reproduces with just that at least, but I'm struggling to understand what causes it. It does happen during processing of the majority of our models though.

Running mypy -v shows Processing SCC of size 748, where SCC stands for strongly connected cluster or something along those lines and 748 is the number of modules it's processing. So it might be a symptom of our codebase being highly entangled (which it is 😅), but the cluster size is the same before this patch so it hasn't changed that.

ljodal · 2022-07-01T13:00:43Z

I commented out the new create_new_manager_class_from_as_manager_method method in main.py and it still crashes, so the problem is somehow related to the refactoring of the from_queryset logic 🤔 Guess I'll have to try to step through that code and see where it goes wrong

flaeppe · 2022-07-02T05:33:39Z

Interesting, could it be related to the new bailing out when .from_queryset isn't called at module level?

django-stubs/mypy_django_plugin/transformers/managers.py

Lines 274 to 277 in c232226

    
           elif semanal_api.is_class_scope(): 
        
               # We force `.from_queryset` to be called _outside_ of a class body. So we'll 
        
               # skip doing any work if we're inside of one.. 
        
               return

ljodal · 2022-07-02T20:47:35Z

I'm pretty sure it's not related to that. I've had that line there unrelated to this and that worked fine. I'll try to pick the PR apart and test it bit by bit next week to see if I can figure it out.

ljodal · 2022-07-03T10:46:11Z

I think it might be related to the name changes for generated managers. I tried to do something similar while working on #1045 and quickly got the same problem in a test repo. Should probably be solvable, but maybe we could try without those changes and do it in a separate PR?

flaeppe · 2022-07-03T11:29:41Z

I think it might be related to the name changes for generated managers. I tried to do something similar while working on #1045 and quickly got the same problem in a test repo. Should probably be solvable, but maybe we could try without those changes and do it in a separate PR?

Sure, we could drop the name change.

I ran in to it too when fiddling with flaeppe#29

Got stuck in AddManagers with a metadata key that didn't match the runtime manager name for a dynamic manager.

I think you should get a trace to the model definition it gets stuck on? Anything special happening with it?

ljodal · 2022-07-03T16:39:06Z

I think you should get a trace to the model definition it gets stuck on? Anything special happening with it?

Yeah, I get a trace, but it ends in a random form class (for our user model, which I guess subclasses a few django.contrib.auth forms). It's literally 100s of lines long, if not 1000s, and I couldn't find anything useful there. I tried to check if any single model was processed more than 20 times (which appears to be the iteration limit) in a row and that wasn't the case. Max was 10

ljodal · 2022-07-03T16:46:46Z

Yeah, it's related to the manager name. Changing that logic back fixes the issue. I wonder if it might be because the name already exists in the module?

diff --git a/mypy_django_plugin/transformers/managers.py b/mypy_django_plugin/transformers/managers.py
index 6aa00c8..3e9b8fe 100644
--- a/mypy_django_plugin/transformers/managers.py
+++ b/mypy_django_plugin/transformers/managers.py
@@ -318,20 +318,20 @@ def create_new_manager_class_from_from_queryset_method(ctx: DynamicClassDefConte
     symbol_kind = semanal_api.current_symbol_kind()
     # Annotate the module variable as `<Variable>: Type[<NewManager[Any]>]` as the model
     # type won't be defined on variable level.
-    var = Var(
-        name=ctx.name,
-        type=TypeType(Instance(new_manager_info, [AnyType(TypeOfAny.from_omitted_generics)])),
-    )
-    var.info = new_manager_info
-    var._fullname = f"{semanal_api.cur_mod_id}.{ctx.name}"
-    var.is_inferred = True
+    # var = Var(
+    #     name=ctx.name,
+    #     type=TypeType(Instance(new_manager_info, [AnyType(TypeOfAny.from_omitted_generics)])),
+    # )
+    # var.info = new_manager_info
+    # var._fullname = f"{semanal_api.cur_mod_id}.{ctx.name}"
+    # var.is_inferred = True
     # Note: Order of `add_symbol_table_node` calls matter. Case being if
     # `ctx.name == new_manager_info.name`, then we'd like the type to override the
     # `Var`, so the `Var` won't exist in the end.
-    assert semanal_api.add_symbol_table_node(ctx.name, SymbolTableNode(symbol_kind, var, plugin_generated=True))
+    # assert semanal_api.add_symbol_table_node(ctx.name, SymbolTableNode(symbol_kind, var, plugin_generated=True))
     # Insert the new manager dynamic class
     assert semanal_api.add_symbol_table_node(
-        new_manager_info.name, SymbolTableNode(symbol_kind, new_manager_info, plugin_generated=True)
+        ctx.name, SymbolTableNode(symbol_kind, new_manager_info, plugin_generated=True)
     )

flaeppe · 2022-07-03T17:10:51Z

... I wonder if it might be because the name already exists in the module?

Yep, that might be it. I added a test which didn't resolve that properly.

flaeppe · 2022-07-03T17:13:52Z

I added a test for that for .as_manager, but forgot about doing it for .from_queryset

https://github.com/flaeppe/django-stubs/blob/c2322264035aa951e51e28906c3322711da1c3d8/tests/typecheck/managers/querysets/test_as_manager.yml#L60-L80

flaeppe · 2022-07-03T19:54:56Z

@ljodal I pushed an update resolving name collisions with .from_queryset. Do you have a possibility to test with that? See if it helps?

ljodal · 2022-07-03T20:00:36Z

Still crashing :(

sobolevn · 2022-08-26T16:32:32Z

Can you please rebase this? I am open to get it merged

flaeppe · 2022-08-29T05:44:10Z

I think there's some rework to get done here before merging is possible. Gonna have to get to that first, not sure when I'll be able to though. (I think it's #1045 we have to align changes here with)

orsinium · 2022-09-16T11:40:32Z

Is there anything I can help with to have it merged? About 30% of mypy violations we have in our code base are caused by this issue 🙃 We use custom QuerySet manager a lot.

orsinium · 2022-09-27T07:01:46Z

@flaeppe the PR seems quite clean and seemingly solving the issue. I can try the patch on our code base if you want to be double sure, though. What kind of rework do you see as blocking? Maybe, I can help with that. And if it's just a matter of rebasing, I can do that for you too.

flaeppe · 2022-09-27T19:45:25Z

@orsinium the changes here depends the version previous to #1045. Basically .from_queryset(...) and .as_manager() shares(d) some helpers/functionality to annotate types. What I did here was to extract the common bits so that it could be reused to annotate both methods.

Although #1045 had to adjust the same logic. So the pieces I extracted conflicts with #1045, basically they look different now. So one kind of have to go find them again and see if/how it can be extracted/shared.

I'll have a look now to see if I'll figure it out

orsinium · 2022-09-28T06:28:17Z

Thank you! Lemmino if you need any help.

flaeppe · 2022-09-28T20:19:24Z

@sobolevn I've rebased and aligned the implementation here with Add support for inline from_queryset in model classes #1045 (as I've spoken of earlier)
@orsinium It would be very helpful if you try this branch out on your project(s) (I use .from_queryset in my projects, so I don't really have much test surface for this implementation)
@ljodal It would be really great if you could have a second (read 42:nd) look here, see if you find any discrepancies from my realignment with Add support for inline from_queryset in model classes #1045 or whatnot

ljodal

Looks good 👍 I've tested it on our codebase and it introduces no new errors. Also converted a few Manager.from_queryset(MyQuerySet) calls and saw no new errors

ljodal · 2022-09-29T09:06:33Z

tests/typecheck/managers/querysets/test_as_manager.yml

+-   case: handles_call_outside_of_model_class_definition
+    main: |
+        from myapp.models import MyModel, MyModelManager
+        reveal_type(MyModelManager)  # N: Revealed type is "myapp.models.ManagerFromModelQuerySet[Any]"


I guess this really should produce a Manager[MyModel], but if I remember correctly you were thinking of fixing that in a separate PR?

Yeah, exactly. I opened a branch for that in my fork, it's quite a bit of code to get it up and running I think. Probably more suitable for a subsequent PR I'd say

ljodal · 2022-09-29T10:01:33Z

mypy_django_plugin/transformers/managers.py

+    manager_base.metadata.setdefault("from_queryset_managers", {})
+    # The `__module__` value of the manager type created by Django's
+    # `.from_queryset` is `django.db.models.manager`. But we put new type(s) in the
+    # module currently being processed, so we'll map those together through metadata.
+    runtime_fullname = ".".join(["django.db.models.manager", manager_name])
+    manager_base.metadata["from_queryset_managers"][runtime_fullname] = fullname


Shouldn't these be under the django key?

Yeah, probably. They weren't previously so I kept them the same. Will probably enforce cache clear to work properly. But that might be completely fine too.

sobolevn

Thank you so much!
I propose to merge this PR and adjust any parts later on.

orsinium · 2022-09-29T12:09:16Z

It would be very helpful if you try this branch out on your project(s)

Tested the new version, and it works. For posterity, I faced a few issues along the way, but I'm not sure if they are related to this PR:

The minimal version for mypy in constraints might be wrong, I had to upgrade to the latest version.

pyproject.toml:1: error: Error importing plugin "mypy_django_plugin.main": cannot import name 'has_placeholder' from 'mypy.semanal_shared'

There are bunch of new errors related to settings:

'Settings' object has no attribute 'PAYMENTS_SERVICE_BASE_URL'  [misc]
Import cycle from Django settings module prevents type inference for 'POD'  [misc]

But again, this can be unrelated, I'll investigate it later.

sobolevn · 2022-09-29T12:10:32Z

The minimal version for mypy in constraints might be wrong

I've bumped it recently, please check if that's now correct.

orsinium · 2022-09-29T12:17:58Z

I've bumped it recently, please check if that's now correct.

You're right, the constraint is correct. I think I've messed up with environments somehow. It failed with mypy 0.961 but the constraint is already mypy>=0.980. So, that's on my side.

ljodal · 2022-09-29T12:20:08Z

There are bunch of new errors related to settings:

'Settings' object has no attribute 'PAYMENTS_SERVICE_BASE_URL'  [misc]
Import cycle from Django settings module prevents type inference for 'POD'  [misc]

This one is due to this: #1163. Basically you most likely import something in the settings file that imports django.conf.settings

sobolevn reviewed Jun 27, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

flaeppe mentioned this pull request Jun 29, 2022

Problem with manager created using .from_queryset() and imported from other module #1032

Closed

flaeppe force-pushed the fix/type-queryset-as-manager branch 2 times, most recently from ad71834 to 086301f Compare June 30, 2022 07:53

sobolevn reviewed Jun 30, 2022

View reviewed changes

mypy_django_plugin/transformers/managers.py Outdated Show resolved Hide resolved

mypy_django_plugin/transformers/managers.py Outdated Show resolved Hide resolved

ljodal reviewed Jun 30, 2022

View reviewed changes

flaeppe mentioned this pull request Jul 2, 2022

Emit error when manager model argument is incorrect flaeppe/django-stubs#29

Closed

ljodal mentioned this pull request Jul 3, 2022

Add support for inline from_queryset in model classes #1045

Merged

Implement support for <QuerySet>.as_manager()

29b5c5b

flaeppe force-pushed the fix/type-queryset-as-manager branch from d927b96 to 29b5c5b Compare September 28, 2022 20:09

flaeppe requested review from ljodal and sobolevn and removed request for ljodal and sobolevn September 28, 2022 20:19

flaeppe added 2 commits September 28, 2022 22:36

fixup! Implement support for <QuerySet>.as_manager()

2d9b5a8

fixup! fixup! Implement support for <QuerySet>.as_manager()

6c50925

ljodal approved these changes Sep 29, 2022

View reviewed changes

sobolevn approved these changes Sep 29, 2022

View reviewed changes

sobolevn merged commit 54d5835 into typeddjango:master Sep 29, 2022

This was referenced Sep 29, 2022

WIP: QuerySet.as_manager() #272

Closed

Fixes for Manager.from_queryset(), support for QuerySet.as_manager() #290

Closed

Add support for QuerySet.as_manager() #265

Closed

orsinium mentioned this pull request Oct 3, 2022

Disable fallback to runtime types for Django settings #1163

Merged

flaeppe deleted the fix/type-queryset-as-manager branch July 30, 2024 21:39

Implement support for <QuerySet>.as_manager() #1025

Implement support for <QuerySet>.as_manager() #1025

Conversation

flaeppe commented Jun 27, 2022

Related issues

sobolevn left a comment

Choose a reason for hiding this comment

flaeppe commented Jun 28, 2022

sobolevn commented Jun 28, 2022

intgr commented Jun 29, 2022

ljodal commented Jun 29, 2022 • edited Loading

flaeppe commented Jun 29, 2022 • edited Loading

ljodal commented Jun 29, 2022

flaeppe commented Jun 29, 2022 • edited Loading

sobolevn commented Jun 30, 2022 • edited Loading

flaeppe commented Jun 30, 2022

ljodal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flaeppe Jul 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flaeppe Jul 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flaeppe commented Jul 1, 2022

ljodal commented Jul 1, 2022

ljodal commented Jul 1, 2022 • edited Loading

flaeppe commented Jul 2, 2022 • edited Loading

ljodal commented Jul 2, 2022

ljodal commented Jul 3, 2022

flaeppe commented Jul 3, 2022

ljodal commented Jul 3, 2022

ljodal commented Jul 3, 2022

flaeppe commented Jul 3, 2022

flaeppe commented Jul 3, 2022

flaeppe commented Jul 3, 2022

ljodal commented Jul 3, 2022

sobolevn commented Aug 26, 2022

flaeppe commented Aug 29, 2022 • edited Loading

orsinium commented Sep 16, 2022

orsinium commented Sep 27, 2022

flaeppe commented Sep 27, 2022

orsinium commented Sep 28, 2022

flaeppe commented Sep 28, 2022

ljodal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sobolevn left a comment

Choose a reason for hiding this comment

orsinium commented Sep 29, 2022

sobolevn commented Sep 29, 2022

orsinium commented Sep 29, 2022

ljodal commented Sep 29, 2022

Implement support for `<QuerySet>.as_manager()` #1025

Implement support for `<QuerySet>.as_manager()` #1025

ljodal commented Jun 29, 2022 •

edited

Loading

flaeppe commented Jun 29, 2022 •

edited

Loading

flaeppe commented Jun 29, 2022 •

edited

Loading

sobolevn commented Jun 30, 2022 •

edited

Loading

flaeppe Jul 1, 2022 •

edited

Loading

flaeppe Jul 1, 2022 •

edited

Loading

ljodal commented Jul 1, 2022 •

edited

Loading

flaeppe commented Jul 2, 2022 •

edited

Loading

flaeppe commented Aug 29, 2022 •

edited

Loading