pin python version for mypy checks

[pmeier]

Without this option, mypy uses the current interpreter version. This can lead some issues like the one fixed in this PR: hashlib only got type annotations starting in Python 3.8. Since our CI workflow runs on 3.7, the default behavior of mypy is to accept everything in case no annotations are present. Thus, CI was green in #5717 (and for example in my local setup), whereas users running Python >= 3.8 will see an error.

With this patch, mypy now always uses the most up-to-date annotations of the standard library regardless of the interpreter version.

[datumbox]

The type_check_python job it self runs on Python 3.7 at the moment. Does it make sense to change the version of the job to 3.10 or you think it's totally fine to leave as is?

[pmeier]

I would keep it 3.7 to make sure contributors can develop with >= 3.7 and are not required to have a higher Python version as base.

[NicolasHug]

From the docs https://mypy.readthedocs.io/en/stable/command_line.html#cmdoption-mypy-python-version

This flag will attempt to find a Python executable of the corresponding version to search for PEP 561 compliant packages. If you’d like to disable this, use the --no-site-packages flag (see Import discovery for more details).

Do you know how that works in practice? I wonder how that can actually work if torchvision isn't installed in 3.10

[pmeier]

Do you know how that works in practice?

I don't, sorry. What I get from this is that will actually run mypy with a 3.10 executable if it finds one. In my local setup (and CI as well) it doesn't and so just runs with the regular interpreter. This seems to work out fine.

Note that you posted the docs for the command line option. The docs for the configuration file do not mention this behavior.

[NicolasHug]

So I assume that if mypy cannot find a 3.10 executable, it won't be able to properly type check code like that of notusedforsecurity, unless it comes from a package that is PEP 561 compliant (i.e. it distributes typing stubs)?

I assume that's fine.

But what a F***ing mess :(

[pmeier]

I've asked for clarification. I'll hold the merge until someone got back to me.

[NicolasHug]

Did you have a chance to make progress on this @pmeier ? It'd be nice to fix this for those of us who develop on >= 3.8

[pmeier]

I didn't get a definitive answer, but all my local tests suggest that the wording is just wrong. Even if you have for example a 3.7 and 3.10 interpreter on path and python_version = 3.10 set, if invoked from the 3.7 interpreter, mypy will stick with it. Thus, we should be good here.

[NicolasHug]

Approving. But this is the kind of problems we could avoid having.

[pmeier]

While thinking about #5942, I realized that pinning python_version as suggested here is only sufficient for the standard library. Unpinned third-party packages can still create errors on different Python versions.

For example, as of numpy==1.22.0 the padding_mode parameter of np.pad requires the Literal["constant", "edge", ...] annotation whereas before str was fine. Since numpy==1.22.0 was only released for Python >= 3.8 this cannot be detected if our mypy workflow runs on Python 3.7.

[NicolasHug]

Yes, supporting type annotations adds another layer of dependency issues.
Would you have any suggestion @pmeier ?

[pmeier]

Right now the only case we have as reference is a BC change: using Literal with only string literals is recognized as subset of str

from typing_extensions import Literal


def np_pad_lt_38(padding_mode: str):
    pass


def np_pad_ge_38(padding_mode: Literal["foo", "bar", "baz"]):
    pass


padding_mode: Literal["foo", "bar"] = "foo"

np_pad_lt_38(padding_mode)
np_pad_ge_38(padding_mode)

This is why our current annotation from #5301 also works on Python 3.7. That being said, I have no idea if projects commit to BC of annotations.

---

IMO the best we can do is too run our workflow on 3.10 to cover all Python versions. At the same time we should still set python_version = 3.10. Contributors developing on < 3.10 might still see a different behavior between local runs and CI, but at least it will be limited to third party libraries.