CI: Freeze DRF commit used in typecheck_tests and types-requests/types-urllib3

[intgr]

• Freeze DRF git commit hash used in CI.
• Freeze versions of types-requests and types-urllib3 since they are already incompatible with mypy 0.991.

FYI @christianbundy.

As discussed in #339 (comment), using the latest master branch state in typecheck_tests frequently causes CI failures to pop up in unrelated pull requests.

I've looked at each error, and checked if it's something we're missing in stubs or a false positive. Usually it's a false positive, or some new feature that has been merged in DRF but will be in the next version (e.g. #338).

But it's annoying and confusing, especially for new contributors, when CI checks start to fail due to unrelated changes. So I've been thinking that we may want to freeze the git commit hash that we check (like is already done in django-stubs).

But the downside is that then we would have to periodically update this hash, something that's been a bit neglected in django-stubs.

[intgr]

Sigh! There are regressions from types-urllib3 and types-requests too! Scary how fragile Python typing still is.

requests-stubs/sessions.pyi:80: error: Type application has too many types (1 expected) (diff)
requests-stubs/sessions.pyi:81: error: Type application has too many types (1 expected) (diff)
requests-stubs/sessions.pyi:99: error: Type application has too many types (1 expected) (diff)
requests-stubs/sessions.pyi:106: error: Type application has too many types (1 expected) (diff)
urllib3-stubs/fields.pyi:6: error: Type application has too many types (1 expected) (diff)

EDIT: Reported to upstream: python/typeshed#9690

[intgr]

Latest types-requests and types-urllib3 are already incompatible with mypy 0.991.

I think it's a good idea to keep these pinned in any case. Dependabot will keep them up to date.

[mschoettle]

Definitely a good idea to pin those dependencies in general

[intgr]

@mschoettle Any thoughts on this?

[christianbundy]

I don't have perms to meaningfully approve this, but I agree that this is a problem and your patch looks like a great solution. ✅

[christianbundy]

LGTM, this is 1 week old: encode/django-rest-framework@22d206c

[mschoettle]

@mschoettle Any thoughts on this?

Thanks for asking!

To be honest, I am not too familiar with the whole typeshed stuff and the purpose of the typecheck_tests script. I am generally fine with it but a bit wary with these kind of manual changes that are then required. I'll give you my general thoughts/questions:

If the commit is pinned, would this prevent these kind of errors to ever show up?

Ideally, they would appear somewhere on here. I assume they would now surface in the dependabot PR to update the third-party types (now that they are pinned), right?

If that is the case, would that solve the problem without having to pin the commit?

Another thing I am wondering: Can a new PR introduce changes that cause type errors that would not be found because another commit is checked against?

Hope it's helpful and the questions are based on my limited understanding of how it is setup :)

[mschoettle]

Definitely a good idea to pin those dependencies in general

[intgr]

I am not too familiar with the whole typeshed stuff and the purpose of the typecheck_tests script.

Ah, for background: typecheck_tests.py runs in CI. It makes a git checkout of the django-rest-framework (DRF) project and then runs mypy over its test suite together with our stubs. The idea is that DRF test suite provides some example code that we can verify our type stubs against.

Since upstream's test suite doesn't have proper type hints, there are bound to be lots of false positives where mypy's inference doesn't guess the intent of the developer, or hacks were made for testing purposes. So we have a massive IGNORED_ERRORS dict containing the false positives:

https://github.com/typeddjango/djangorestframework-stubs/blob/master/scripts/typecheck_tests.py#L41-L45

Since we took the latest master state of DRF repo, as upstream adds new tests, new false positives would appear at a random moment in some djangorestframework-stubs pull request and we'd have to look into them and usually add new ignores (e.g. #323, #311, #303, #273).

[intgr]

There are two changes in this PR. One is pinning types-requests and types-urllib3, that's just incidental, because those packages just now happened to break compatibility with mypy 0.991 and would break our CI. Dependabot will update them and they aren't a concern.

The significant part is is freezing the git hash in typecheck_tests.py. I doubt we can make Dependabot update this.

[christianbundy]

I doubt we can make Dependabot update this.

I think you're right, but we can build a simple GitHub Action that runs ~weekly to update this and open a PR, similar to Dependabot. Very happy to help with this if it's a high priority, and we could reuse it in django-stubs.

[intgr]

Very happy to help with this if it's a high priority, and we could reuse it in django-stubs.

That would be helpful, though I wouldn't say it's high priority.

In the longer term I'd hope to see mypy_primer integrated, which handles false positives far better because it only reports diffs between mypy results (before and after PR changes). With that, I would suggest dropping typecheck_tests entirely, so we can get rid of the burden of maintaining these ignores. I've opened an issue at typeddjango/django-stubs#1362 to discuss that.

[mschoettle]

Thanks for the additional background, @intgr!

And apologies. I misread the code in the typecheck script and thought that the DRF-stubs project is checked out when in fact it is DRF.

I am wondering if the pinned DRF version in requirements.txt could be used. Then the tag associated with that version number could be checked out. That way it is tied to the declared dependency and when a PR is raised to change the version, any new false positives would appear at that point. This would extend the functionality added in this PR so suitable for its own PR.

[intgr]

Thanks for the reviews!