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

Add type annotations #558

Open
mthuurne opened this issue Mar 16, 2023 · 33 comments
Open

Add type annotations #558

mthuurne opened this issue Mar 16, 2023 · 33 comments
Milestone

Comments

@mthuurne
Copy link
Contributor

mthuurne commented Mar 16, 2023

Problem

When type-checking a model that uses InheritanceManager using mypy, the following error is displayed:

app/models.py:39: error: Could not resolve manager type for
"app.models.MyModel.objects"  [django-manager-missing]
        objects = InheritanceManager()
        ^

According to this Stack Overflow post, the origin of this problem is the django-stubs mypy plugin being unable to find InheritanceManager because django-model-utils does not provide type annotations.

According to the linked post, no actual annotations are required, just a marker that the package is annotated. But I think real annotations would be good to have anyway, both for users and developers of django-model-utils.

I am working on adding annotations to django-model-utils. I don't know if I'll have enough time to completely annotate it, but I'll share what I have in any case, probably tomorrow.

Environment

  • Django Model Utils version: 4.3.1
  • Django version: 3.2
  • Python version: 3.9
  • Other libraries used, if any: django-stubs 1.16.0
@mthuurne
Copy link
Contributor Author

The work-in-progress can be found in this branch.

Given the relatively low line count, I thought I would be able to annotate all of django-model-utils quickly, but it is quite complex code that does things that are not easy to accommodate in the type system.

The following commits might be worth reviewing already, as they don't contain type annotations themselves, but are code cleanups necessary to make mypy accept the code:

@mthuurne
Copy link
Contributor Author

I don't know yet when I'll have time to finish the annotations, but I'll submit the fixes as separate PRs, both to make sure they can be included sooner and to simplify the review of the annotations PR later.

@Mostaard
Copy link

Hi! I was wondering whether you have made any progress on this? I would like to use types too. Can I help?

@mthuurne
Copy link
Contributor Author

I haven't made progress since last year. I'm still waiting for my cleanups PRs #566 and #567 to be reviewed.

@foarsitter
Copy link
Contributor

@mthuurne both PR's you are mentioning are merged. Are you willing to take the further lead on typing?

@mthuurne
Copy link
Contributor Author

I'll pick it up again.

I split off one cleanup that can already be applied: #591.

@mthuurne
Copy link
Contributor Author

mthuurne commented Mar 22, 2024

More cleanups: #594, #595, #596.

@mthuurne
Copy link
Contributor Author

Even more cleanups: #597, #598.

I also updated the branch on top of the latest master.

@foarsitter
Copy link
Contributor

Type annotations isn't a job that has to be done in one haul but you are getting pretty far. By any change, is there a way to make smaller chunks out of the branch?

@foarsitter foarsitter added this to the 5.0.0 milestone Mar 23, 2024
@mthuurne
Copy link
Contributor Author

Yes, there should be ways to carve it up into multiple PRs. I prefer to first annotate everything in a branch though before submitting the annotations themselves, as that can uncover mistakes made earlier. In particular, having the unit tests annotated functions as a consistency check for the annotations in the code under test.

Something that might be possible to split off already is adding mypy to CI. I'll have a look at that later today.

@foarsitter
Copy link
Contributor

Can you open a draft PR from the annotations branch so we can stay in touch with your progres? If we first merge all the low hanging fruit we may split up the remaining issues into sub branches when needed. Played around with the generic tests of the Tracker and there may be some challenges that aren't blocking features to me.

@mthuurne
Copy link
Contributor Author

I could do that, but I'm still force-pushing after squashing corrections, so that would make the PR rather messy. If you don't mind that, I'll create a draft PR.

@mthuurne
Copy link
Contributor Author

mthuurne commented Mar 25, 2024

I made the PR to add mypy to CI: #601.

I put several discussion points in the PR comment, mostly about how to handle various dependencies.

@foarsitter
Copy link
Contributor

I could do that, but I'm still force-pushing after squashing corrections, so that would make the PR rather messy. If you don't mind that, I'll create a draft PR.

I don't mind, notifications for drafts is opt-in isn't it?

@mthuurne
Copy link
Contributor Author

I don't mind, notifications for drafts is opt-in isn't it?

Yes, but I couldn't create a draft PR from the start: I didn't see any checkbox on the submission form and starting the title with "Draft:" like in GitLab didn't work. So if you want to skip notifications, you'll have to unsubscribe from the PR, I'm afraid.

In any case, what I was worried about is that comments on changes would become orphans as new versions are force-pushed.

@foarsitter
Copy link
Contributor

It is behind the dropdown:
image

Thanks for your draft, a lot to learn for me about typing!

@mthuurne
Copy link
Contributor Author

Here is a new series of cleanups that can be merged separately: #604, #605, #606, #607.

@foarsitter
Copy link
Contributor

With 4.5.0 released we can merge our more impactful changes 🎉

@mthuurne
Copy link
Contributor Author

mthuurne commented Apr 9, 2024

Another small cleanup that can be merged: #610.

I'm still working on completing the annotations, but it's progressing more slowly as the low-hanging fruit has already been picked.

PR #601 could already be reviewed and merged: it would at least ensure that new developments do not regress the type checking.

@foarsitter
Copy link
Contributor

I like your progress @mthuurne

If you are running out of time or want to subdivide it into smaller issues we can merge it as is and take that as starting point. Just let me know.

@mthuurne
Copy link
Contributor Author

One thing to decide is how to deal with mixins in the test suite. Unfortunately, there is no good way to handle mixins in Python's typing system yet.

The problem with mixins is that they have expectations on what is available on self, coming from a base class that they will eventually be mixed in with. However, there is no way to tell a type checker like mypy what the expected base class will be.

If there is only limited functionality used from the base class, I typically duplicate the annotation on the mixin, like the _db attribute in this example:

class SoftDeletableManagerMixin(Generic[ModelT]):
    """
    Manager that limits the queryset by default to show only not removed
    instances of model.
    """
    _queryset_class = SoftDeletableQuerySet

    _db: str | None

    ...

If a lot of functionality is used from the base class, I typically give the mixin an explicit base class during type checking, but not at runtime:

if TYPE_CHECKING:
    MixinBase = TestCase
else:
    MixinBase = object

class FieldTrackerMixin(MixinBase):
    ...

This can become quite tricky though, as you have to be careful to not break the method resolution order during type checking. Also there is the django-stubs mypy plugin, which executes code while TYPE_CHECKING is True and will therefore receive an impure mixin.

A full example of this is 869c6a6, where I had to do some code surgery to get the type annotation overrides and method resolution order correct.

In the particular case of unit tests, the main reason why mixins require special attention is because unittest.TestCase or django.test.TestCase is used to get access to methods like assertEqual(). It is possible to sidestep this issue by using pytest's augmented assert statements instead and having test functions rather than test methods.

I used the pytest assert style in parts of 80dda9f.

What would be the preferred way forward:

  • duplicate the signatures for all used assertX() methods in each mixin?
  • use the fake base class approach for all mixins?
  • convert to pytest-style assertions everywhere?
  • convert to pytest-style assertions only where needed?

Personally I prefer the pytest style even when not dealing with mixins, but I can't speak for all developers who will work on this code in the future.

@mthuurne
Copy link
Contributor Author

Another small cleanup: #611. This one is backwards incompatible, but that means now is a good time to do that, right?

@foarsitter
Copy link
Contributor

Yes, 5.0 will be full of breaking changes :)

@mthuurne
Copy link
Contributor Author

Note that #541 is still an open issue. For now, I'll annotate the current behavior and drop the "fix" I had for this issue, since it probably wasn't the right fix.

@mthuurne
Copy link
Contributor Author

mthuurne commented Apr 16, 2024

More cleanups: #612, #613, #614

@mthuurne
Copy link
Contributor Author

The draft PR #603 passed CI checks for the first time.

There is still work to do though:

  • two errors are reported when the mypy cache isn't empty; I suspect this is an issue in the django-stubs mypy plugin
  • add py.typed marker and start checking applications against the annotated library
  • check for type arguments that are not forwarded properly, like model type falling back to Any
  • check whether the annotations can be done in a cleaner way
  • split up the changes in a way that allows reviewing and merging in chunks

@foarsitter
Copy link
Contributor

  • check whether the annotations can be done in a cleaner way
  • split up the changes in a way that allows reviewing and merging in chunks

Those last two bullets aren't required (anymore) from my perspective.

@gmcrocetti
Copy link
Contributor

Do you guys believe an extra pair of hands would help in closing this issue and releasing 5.x ? @foarsitter @mthuurne

@mthuurne
Copy link
Contributor Author

mthuurne commented May 11, 2024

The bulk of the work has been done: everything is covered in annotations. Now it just needs some verification and cleanups.

Ways to help would be:

  • install the code from Add type annotations #603 and use mypy to type check your own application that uses django-model-utils against it
  • review the type annotations: check whether they make sense and whether there might be a simpler way to achieve the same effect

@mthuurne
Copy link
Contributor Author

Note to anyone testing the new annotations: do not use pip install -e, as the django-stubs mypy plugin does not see modules installed in that way. This results in a "Could not resolve manager type" error when using InheritanceManager.

Using just pip install <path> or pip install <url> works fine.

@foarsitter
Copy link
Contributor

If we are in the testing phase I would like to release a beta, what do you people think?

@gmcrocetti
Copy link
Contributor

I agree @foarsitter , let's make a release candidate 🚀 .

@mthuurne
Copy link
Contributor Author

mthuurne commented Jun 13, 2024

#603 is ready for merging into a beta / preview release.

Some things to discuss before doing a production release:

Use typing.Self?

Like Django itself, django-model-utils uses a lot of mix-ins. Currently, I have annotated mix-in methods that return an instance of the concrete class using the mix-in to return the matching concrete class from django-model-utils. For example:

class InheritanceQuerySetMixin(Generic[ModelT]):

    def select_subclasses(self, *subclasses: str | type[models.Model]) -> InheritanceQuerySet[ModelT]:
        ...

class InheritanceQuerySet(InheritanceQuerySetMixin[ModelT], QuerySet[ModelT]):
    ...

However, it is also possible for user code to combine InheritanceQuerySetMixin with their own class and that is in fact the entire reason for having mix-ins in the first place. In that case, the returned instance will be of the user class and the annotation doesn't reflect that.

This can be solved using typing.Self and that is also how django-stubs handles situations like this. However, typing.Self was only introduced in Python 3.11, so until 3.10 support is dropped we'd have to import Self from typing-extensions instead, adding a dependency.

As django-stubs relies on typing-extensions already, this dependency must be installed for type checking anyway, but my question to the django-model-utils maintainers is whether it would be acceptable to have it as a runtime dependency. If not, it is possible to work around it by importing it only when typing.TYPE_CHECKING is True, but that will add some noise to the code.

How to communicate the required version of django-stubs?

I filed a handful of PRs on django-stubs to add or fix support for things that django-model-utils uses. These have been included in the 5.0 release of django-stubs.

While some of these changes are needed only internally (for example, by the unit tests), others might be required for the public interface to be accepted by mypy. So in practice, I think we can only rely on django-model-utils type checking to work with a recent enough version of django-stubs. How do we communicate this to the end users?

One option would be to include it in the documentation, but this is manual work, which means it's easy to forget to update it, both when making changes to django-model-utils and at the end user's side when they upgrade django-model-utils.

What I think would be the proper solution would be to add a dependency extra for type checking that depends on the minimum version of django-stubs that we know works correctly.

Convert unit tests to pytest style?

Currently the unit tests use the unittest class-based approach to organizing test code. In particular the tests/test_fields/test_field_tracker.py test module uses a complex inheritance structure. While I got that module to be accepted by mypy, the result isn't pretty and liberally uses # type: ignore overrides. Besides making the tests harder to read, this undermines the purpose of having annotated unit tests, which is to have code that verifies whether the annotations in the public interfaces (the code under test) make sense.

I think that using pytest's native style, with test functions rather than methods, parameter decorators rather than overriding fields, and fixtures to control the setup and teardown might help to clean this up. However, that means converting a lot of test code, the maintainers having to become familiar with a new style and probably adding pytest-django as a test dependency because some of Django's test helpers only work with unittest.TestCase out of the box. So that's not something that I should be deciding on my own.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants