Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AssertionError raised when calling Manager.from_queryset() #905

Closed
RJPercival opened this issue Mar 31, 2022 · 17 comments · Fixed by #1731
Closed

AssertionError raised when calling Manager.from_queryset() #905

RJPercival opened this issue Mar 31, 2022 · 17 comments · Fixed by #1731
Labels
bug Something isn't working

Comments

@RJPercival
Copy link
Contributor

RJPercival commented Mar 31, 2022

Bug report

What's wrong

When the argument to models.Manager.from_queryset() is a type alias, the following AssertionError is raised:

version: 0.942
Traceback (most recent call last):
  File "mypy/semanal.py", line 5220, in accept
  File "mypy/nodes.py", line 1155, in accept
  File "mypy/semanal.py", line 2091, in visit_assignment_stmt
  File "mypy/semanal.py", line 2377, in apply_dynamic_class_hook
  File "/Users/robpercival/.virtualenvs/kraken-core/lib/python3.8/site-packages/mypy_django_plugin/transformers/managers.py", line 173, in create_new_manager_class_from_from_queryset_method
    assert isinstance(derived_queryset_info, TypeInfo)
AssertionError:

If it is a class with a type argument, it instead raises here:

version: 0.942
Traceback (most recent call last):
  File "mypy/semanal.py", line 5220, in accept
  File "mypy/nodes.py", line 1155, in accept
  File "mypy/semanal.py", line 2091, in visit_assignment_stmt
  File "mypy/semanal.py", line 2377, in apply_dynamic_class_hook
  File "/Users/robpercival/.virtualenvs/kraken-core/lib/python3.8/site-packages/mypy_django_plugin/transformers/managers.py", line 148, in create_new_manager_class_from_from_queryset_method
    assert isinstance(passed_queryset, NameExpr)
AssertionError:

How is that should be

It should support both type aliases and classes with type arguments.

System information

  • OS: macOS 12.3
  • python version: 3.8.3
  • django version: 3.2.10
  • mypy version: 0.942
  • django-stubs version: 1.10.1
  • django-stubs-ext version: 0.4.0
@RJPercival RJPercival added the bug Something isn't working label Mar 31, 2022
@felixmeziere
Copy link

Same bug here, blocking me completely

@felixmeziere
Copy link

felixmeziere commented Jun 24, 2022

@flaeppe I see that you have been working a bit on progressing the work on querysets and managers recently, thanks for this!
With the recent release though I get warnings everywhere and can't type as well as I used to be able to (with tricks, obviously). It's impossible to get a properly typed manager that can chain calls of custom methods with filters, annotate etc. this is a big impediment for us. Please let me know if I can help :)
Thanks again for the work!

@flaeppe
Copy link
Member

flaeppe commented Jun 25, 2022

@felixmeziere Yes I have been doing some stuff around querysets.

These kind of asserts are there as there's loads of different ways to use types. I've tried to build support for the most obvious happy case(s) first, when I've implemented something.

But I can understand there's limitations in it.

Best kind of issues are those opened also having a minimal repro, either inlined in description or paired with a PR. As long as one can figure one out, of course.

@flaeppe
Copy link
Member

flaeppe commented Jun 25, 2022

It's impossible to get a properly typed manager that can chain calls of custom methods with filters, annotate etc. this is a big impediment for us

I haven't had many issues at all recently with this. I'm curious what kind of problem you're seeing and when

@felixmeziere
Copy link

felixmeziere commented Jun 29, 2022

@flaeppe sorry for the delayed response, I've been battling with this trying to apply the docs as well as possible (and other custom techniques to try and make it work) for a few days. I could write an issue but it seems really hard to describe this in detail (there are many different problems that arise depending on which way I try to get things to work, describing each way and the corresponding problems with an example codebase is going to be very long and difficult).

Before I might potentially do that, is there any chance you could jump on a screenshare call with me for like 15mn to discard the option that I might have missed entirely how it's supposed to work? That'd be amazing! ❤️

@flaeppe
Copy link
Member

flaeppe commented Jun 29, 2022

@felixmeziere the base line is that we expect the call to .from_queryset to look something like:

class MyQuerySet(models.QuerySet["MyModel"]):
    ...
    
MyManager = models.Manager.from_queryset(MyQuerySet)
class MyModel(models.Model):
    objects = MyManager()

Though, one could hit the first assert when doing something like this

class MyQuerySet(models.QuerySet["MyModel"]):
    ...

X = MyQuerySet
MyManager = models.Manager.from_queryset(X)

The interesting part, for me, is e.g. how you pass the queryset variable to .from_queryset. Or if you're doing something with the models.Manager end, that is specific. Personally in my code, in almost all cases, I'm doing MyManager = models.Manager.from_queryset(MyQuerySet).

I'm interested in what kind of use case you involve .from_queryset in? I'm looking for examples really.

@felixmeziere
Copy link

felixmeziere commented Jun 30, 2022

@flaeppe Manager methods return a queryset.
Let's say I define:

class HostingAdvertManager(models.Manager[HostingAdvert]):
    def filter_by_is_active(self) -> QuerySet[HostingAdvert]:
        return self.filter(onboarding_status='DONE', is_cool=True)

    def annotate_with_is_available(self) -> QuerySet[WithAnnotations[HostingAdvert, TypedDict(...)]]:
        return self.annotate(is_available=Exists(...))

class HostingAdvert(models.Model):
    ...

    objects = HostingAdvertManager()

    ...

then, once I have called one of this manager's method e.g. queryset = HostingAdvert.objects.filter(is_active=True), the variable queryset I obtain is just a QuerySet[HostingAdvert], not the manager anymore, so I cannot chain the custom manager methods I have defined, am I wrong?

For example I cannot do HostingAdvert.objects.filter_by_is_active().annotate_with_is_available() and not even queryset = HostingAdvert.objects.filter(something=True).filter_by_is_active() isn't it?

That's why I have to first define the queryset and then derive the manager from it, so that I can chain the methods, or have I missed something big (I wish 😋)?
In a basic way I could do:

class HostingAdvertQuerySet(models.QuerySet[HostingAdvert]):
    def filter_by_is_active(self) -> HostingAdvertQuerySet:
        return self.filter(onboarding_status='DONE', is_cool=True)

    def annotate_with_is_available(self) -> HostingAdvertQuerySet:
        return self.annotate(is_available=Exists(...))

HostingAdvertManager = models.Manager.from_queryset(HostingAdvertQuerySet)

class HostingAdvert(models.Model):
    ...

    objects = HostingAdvertManager()

    ...

But this doesn't allow to chain methods with annotations for example, it's too naive, we need a typevar

So I can do:

_HostingAdvertTypeVar = TypeVar("_HostingAdvertTypeVar", bound=HostingAdvert)

class HostingAdvertQuerySet(models.QuerySet[_HostingAdvertTypeVar]):
    def filter_by_is_active(self) -> HostingAdvertQuerySet[_HostingAdvertTypeVar]:
        return self.filter(onboarding_status='DONE', is_cool=True)

    def annotate_with_is_available(self) -> HostingAdvertQuerySet[WithAnnotations[_HostingAdvertTypeVar, TypedDict(...)]]:
        return self.annotate(is_available=Exists(...))

HostingAdvertManager = models.Manager.from_queryset(HostingAdvertQuerySet[HostingAdvert])

class HostingAdvert(models.Model):
    ...

    objects = HostingAdvertManager()

    ...
.

but then mypy breaks altogether which what this issue points out.

I can fix the problem by doing:

_HostingAdvertTypeVar = TypeVar("_HostingAdvertTypeVar", bound=HostingAdvert)

class GenericHostingAdvertQuerySet(models.QuerySet[_HostingAdvertTypeVar]):
    def filter_by_is_active(self) -> HostingAdvertQuerySet[_HostingAdvertTypeVar]:
        return self.filter(onboarding_status='DONE', is_cool=True)

    def annotate_with_is_available(self) -> HostingAdvertQuerySet[WithAnnotations[_HostingAdvertTypeVar, TypedDict(...)]]:
        return self.annotate(is_available=Exists(...))

class HostingAdvertQuerySet(GenericHostingAdvertQuerySet[HostingAdvert]):
    pass

HostingAdvertManager = models.Manager.from_queryset(HostingAdvertQuerySet)

class HostingAdvert(models.Model):
    ...

    objects = HostingAdvertManager()

    ...
.

But then when defining things this way, I lose the annotations typing when I do HostingAdvert.objects.annotate_with_is_available()[0]: the type I get is just HostingAdvert, not WithAnnotations[HostingAdvert, TypedDict(...)] (this is due to how WithAnnotations works, I investigated, it can't handle TypeVars I think at the moment). And when I reveal_type of HostingAdvert.objects.annotate_with_is_available().filter_by_is_active()[0] I get type Any, although HostingAdvert.objects.annotate_with_is_available().filter_by_is_active() is typed correctly as HostingAdvertQuerySet[WithAnnotations[HostingAdvert, TypedDict(...)]] !!!!

So I can do:

_HostingAdvertTypeVar = TypeVar("_HostingAdvertTypeVar", bound=HostingAdvert)

class GenericHostingAdvertQuerySet(models.QuerySet[_HostingAdvertTypeVar]):
    def filter_by_is_active(self) -> HostingAdvertQuerySet[HostingAdvert]:
        return self.filter(onboarding_status='DONE', is_cool=True)  # type: ignore[return-value]

    def annotate_with_is_available(self) -> HostingAdvertQuerySet[WithAnnotations[HostingAdvert, TypedDict(...)]]:
        return self.annotate(is_available=Exists(...)) # type: ignore[return-value]

class HostingAdvertQuerySet(GenericHostingAdvertQuerySet[HostingAdvert]):
    pass

HostingAdvertManager = models.Manager.from_queryset(HostingAdvertQuerySet)

class HostingAdvert(models.Model):
    ...

    objects = HostingAdvertManager()

    ...
.

in this case, HostingAdvert.objects.annotate_with_is_available().filter_by_is_active()[0] gives me the right type and annotate_with_is_available as well, but 1) it does not make ANY sense to have the typevar up there but not use it inside the class, however that does make things work (this is completely random...) 2) I still can't chain the calls, for example several consequent annotation calls, and 3) I have to add all the # type: ignore[return-value] because of course the return type GenericHostingAdvertQuerySet[_HostingAdvertTypeVar] doesn't match the declared return type GenericHostingAdvertQuerySet[HostingAdvert]

This is super intricate to explain with the corresponding mypy errors it would be much easier just easier to show you!.

What I had before was a setup where I would declare things all myself and it used to work quite well for me (plus I was getting all the autocomplete of my IDE which is an extra productivity and readability benefit). I wish I could at least continue to do that but I can't because this new version of django-stubs overwrites all my uses of QuerySet, Manager etc to force this new pattern that does not work for me, I'm frankly desperate at this stage.

Here is a gist of the pattern I used to have before this django-stubs version (ugly but worked like a charm, waiting for better):

_HostingAdvertTypeTagVar = TypeVar("_HostingAdvertTypeTagVar", bound="HostingAdvert")


class HostingAdvertQuerySet(QuerySet[_HostingAdvertTypeTagVar]):
    def filter_by_is_active(self) -> HostingAdvertQuerySet[_HostingAdvertTypeVar]:
        return self.filter(onboarding_status='DONE', is_cool=True)

    def annotate_with_is_available(self) -> HostingAdvertQuerySet[WithAnnotations[HostingAdvertt, TypedDict(...)]]: # Does not allow to chain two annotation calls because WithAnnotations doesn't hande TypeVars, it would be AMAZING if it could.
        return self.annotate(is_available=Exists(...))

class ManagerFromHostingAdvertQuerySetType(models.Manager[_HostingAdvertTypeVar]):
    def filter_by_is_active(self) -> HostingAdvertQuerySet[_HostingAdvertTypeVar]:
        ...
    def annotate_with_is_available(self) -> HostingAdvertQuerySet[WithAnnotations[HostingAdvertt, TypedDict(...)]]:
        ...

class HostingAdvert(models.Model):
    ...

    objects: ManagerFromHostingAdvertQuerySetType[HostingAdvert] = HostingAdvertQuerySet[
        "HostingAdvert"
    ].as_manager()  # type:ignore[assignment]

    ...

How can we move forward with this issue?

@flaeppe
Copy link
Member

flaeppe commented Jun 30, 2022

@flaeppe Manager methods return a queryset. Let's say I define:

class HostingAdvertManager(models.Manager[HostingAdvert]):
    def filter_by_is_active(self) -> QuerySet[HostingAdvert]:
        return self.filter(onboarding_status='DONE', is_cool=True)

    def annotate_with_is_available(self) -> QuerySet[WithAnnotations[HostingAdvert, TypedDict(...)]]:
        return self.annotate(is_available=Exists(...))

class HostingAdvert(models.Model):
    ...

    objects = HostingAdvertManager()

    ...

then, once I have called one of this manager's method e.g. queryset = HostingAdvert.objects.filter(is_active=True), the variable queryset I obtain is just a QuerySet[HostingAdvert], not the manager anymore, so I cannot chain the custom manager methods I have defined, am I wrong?

Right, the thing here is that you're returning django.db.models.QuerySet, which comes from Django. Your custom methods are not declared for that class. I normally work with querysets, but what I do there is to return my custom queryset instead of django.db.models.Queryset since I inherit from that. Not sure it'll work the same with managers, but applying it there then your custom methods will look like:

class HostingAdvertManager(models.Manager[HostingAdvert]):
    def filter_by_is_active(self) -> HostingAdvertManager:
        ...

That way you'll have access to your custom methods while chaining.

class HostingAdvertQuerySet(models.QuerySet[HostingAdvert]):
    def filter_by_is_active(self) -> QuerySet[HostingAdvert]:
        return self.filter(onboarding_status='DONE', is_cool=True)

    def annotate_with_is_available(self) -> QuerySet[WithAnnotations[HostingAdvert, TypedDict(...)]]:
        return self.annotate(is_available=Exists(...))

HostingAdvertManager = models.Manager.from_queryset(HostingAdvertQuerySet)

class HostingAdvert(models.Model):
    ...

    objects = HostingAdvertManager()

    ...

Using the same approach mentioned above for querysets:

class HostingAdvertQuerySet(models.QuerySet[HostingAdvert]):
    def filter_by_is_active(self) -> HostingAdvertQuerySet:
        ...

    def annotate_with_is_available(self) -> HostingAdvertQuerySet[WithAnnotations[HostingAdvert, TypedDict(...)]]:
        ...

HostingAdvertManager = models.Manager.from_queryset(HostingAdvertQuerySet)
class HostingAdvert(models.Model):
    ...

    objects = HostingAdvertManager()

    ...

I'm unfamiliar with using WithAnnotations, so I'm not really sure if or how that can work. In my experience, it has been quite unstable and I'd normally just work with returning HostingAdvertQuerySet even though having called .annotate.

In any case, this should allow you to chain Django's methods with your custom ones.

@felixmeziere
Copy link

@flaeppe sorry but I just amended my previous post for it to be much bigger and complete 😅 do you mind re-reading it and maybe amending yours if anything needs to change in it? (I'm reading yours in the meantime)

@felixmeziere
Copy link

felixmeziere commented Jun 30, 2022

@flaeppe one point regarding your answer:

you have written:

class HostingAdvertManager(models.Manager[HostingAdvert]):
    def filter_by_is_active(self) -> HostingAdvertManager:
        ...

This allows me to chain things in terms of typing (but without typevar so it's limited for annotations anyway), but not at runtime: python will crash here! Because the methods of a manager (when you do self.filter()) do not return another instance of the manager, they return the Queryset attached to the manager (returned by the get_queryset() method of the manager btw), that does not have the custom methods. Then, when you call .filter after that, it is indeed the Queryset that keeps being returned. Basically a manager is only used for the first call of the chain, then we exit ManagerLand and enter QuerySetLand

@flaeppe
Copy link
Member

flaeppe commented Jun 30, 2022

I'd type it up like this, hopefully that'll get you something to work with, typing wise.

class HostingAdvertQuerySet(models.QuerySet[HostingAdvert]):
    def filter_by_is_active(self) -> HostingAdvertQuerySet:
        return self.filter(onboarding_status='DONE', is_cool=True)

    def annotate_with_is_available(self) -> HostingAdvertQuerySet:
        return self.annotate(is_available=Exists(...))


HostingAdvertManager = models.Manager.from_queryset(HostingAdvertQuerySet)
class HostingAdvert(models.Model):
    ...

    objects = HostingAdvertManager()

    ...

Regarding the WithAnnotations, and getting that to work, I wouldn't know unfortunately.

@flaeppe
Copy link
Member

flaeppe commented Jun 30, 2022

@flaeppe one point regarding your answer:

you have written:

class HostingAdvertManager(models.Manager[HostingAdvert]):
    def filter_by_is_active(self) -> HostingAdvertManager:
        ...

This allows me to chain things in terms of typing (but without typevar so it's limited for annotations anyway), but not at runtime: python will crash here! Because the methods of a manager (when you do self.filter()) do not return another instance of the manager, they return the Queryset attached to the manager (returned by the get_queryset() method of the manager btw), that does not have the custom methods. Then, when you call .filter after that, it is indeed the Queryset that keeps being returned. Basically a manager is only used for the first call of the chain, then we exit ManagerLand and enter QuerySetLand

Yeah, I suspected that it could become difficult to chain between manager and queryset methods but wasn't sure.

@flaeppe
Copy link
Member

flaeppe commented Jun 30, 2022

How can we move forward with this issue?

@felixmeziere to summarise my interpretation of what you've written down here, this sounds more like issues/limitations with WithAnnotations rather than querysets/managers, although still related to querysets/managers since WithAnnotations depends on them. At least that's what I'd say.

@felixmeziere
Copy link

felixmeziere commented Jun 30, 2022

@flaeppe yes my issue boils down to: (but generally usage of TypeVar with this manager/queryset typing has a lot of bugs and also I don't get IDE autocomplete anymore that I used to have)

class HostingAdvertQuerySet(models.QuerySet[HostingAdvert]):
    def filter_by_is_active(self) -> HostingAdvertQuerySet[HostingAdvert]:
        return self.filter(onboarding_status='DONE', is_cool=True)

    def annotate_with_is_available(self) -> HostingAdvertQuerySet[WithAnnotations[HostingAdvert, TypedDict(...)]]:
        return self.annotate(is_available=Exists(...))

HostingAdvertManager = models.Manager.from_queryset(HostingAdvertQuerySet)

class HostingAdvert(models.Model):
    ...

    objects = HostingAdvertManager()

    ...


reveal_type(HostingAdvert.objects.filter_by_is_active().annotate(tonton="tata")) # revealed type is GenericHostingAdvertQuerySet[WithAnnotations[HostingAdvert, TypedDict(...)]])
reveal_type(HostingAdvert.objects.filter_by_is_active().annotate(tonton="tata"))[0] # lost the annotation!

and

_HostingAdvertTypeVar = TypeVar("_HostingAdvertTypeVar", bound=HostingAdvert)

class GenericHostingAdvertQuerySet(models.QuerySet[_HostingAdvertTypeVar]):
    def filter_by_is_active(self) -> HostingAdvertQuerySet[HostingAdvert]:
        return self.filter(onboarding_status='DONE', is_cool=True)  # type: ignore[return-value]

    def annotate_with_is_available(self) -> HostingAdvertQuerySet[WithAnnotations[HostingAdvert, TypedDict(...)]]:
        return self.annotate(is_available=Exists(...)) # type: ignore[return-value]

class HostingAdvertQuerySet(GenericHostingAdvertQuerySet[HostingAdvert]):
    pass

HostingAdvertManager = models.Manager.from_queryset(HostingAdvertQuerySet)

class HostingAdvert(models.Model):
    ...

    objects = HostingAdvertManager()

    ...


reveal_type(HostingAdvert.objects.filter_by_is_active().annotate(tonton="tata")) # revealed type is GenericHostingAdvertQuerySet[WithAnnotations[HostingAdvert, TypedDict(...)]])
reveal_type(HostingAdvert.objects.filter_by_is_active().annotate(tonton="tata"))[0] # revealed type is correct WithAnnotations[HostingAdvert, TypedDict(...)]] , but typing of the model is absurd and requires lots of type: ignores!

@ljodal hi! Would you any ideas on this issue maybe?

@ljodal
Copy link
Contributor

ljodal commented Jun 30, 2022

I have not used WithAnnotations, but my impression is that it's a recently added feature that might not be that mature yet. I've seen some issues with it, but we're not on the latest django-stubs yet, so that might be why. As for chaining WithAnnotations I don't think that'll work, but with the right types on your QuerySets you should be able to make the last WithAnnotations stick.


As for the other issues, I'm not sure, but I haven't seen the issues you describe here.

In our codebase we generally try to avoid custom managers when we can and we don't really have any generic querysets either, at least not the way you describe. I'm assuming you have sub-classes of HostingAdvert that you want to use with the same queryset?

We generally type our querysets like this. It works fine, but then the queryset is specific to one model:

class MyModelQueryset(QuerySet["MyModel"]):
    def is_active(self) -> "MyModelQuerySet":
        return self.filter(is_active=True)

I've not used typevars for the model type, but you might be able to get away with typing the self type:

M = TypeVar("M", bound=Model)

class GenericQuerySet(QuerySet):
    def is_active(self: GenericQuerySet[M]) -> GenericQuerySet[M]:
        return self.filter(is_active=True)

I think that should work on querysets (haven't tested it, so take it with a grain of salt). I suspect that the type will be wrong if this method is called on the manager though, but I'm not entirely sure. If so that should be fixable.

@felixmeziere
Copy link

@ljodal thanks a LOT for the "typing the self type" tip, I had never thought of this technique, I'm going to try it out todays see if it unlocks some stuff, have a good one :-)

@RJPercival
Copy link
Contributor Author

RJPercival commented Jul 7, 2022

I think you'll probably run into this bug if you try the "typing the self type" approach, unless that has been fixed recently: "SelfType is unbound" error with custom QuerySet definitions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging a pull request may close this issue.

4 participants