Skip to content

pin python version for mypy checks #5755

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

pin python version for mypy checks #5755

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2c6b802
upgrade Python version of the mypy CI workflow
pmeier Apr 6, 2022
5a6d633
Revert "upgrade Python version of the mypy CI workflow"
pmeier Apr 6, 2022
6eb16aa
pin python version for mypy checks
pmeier Apr 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions mypy.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
[mypy]

files = torchvision
python_version = 3.10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

show_error_codes = True
pretty = True
allow_redefinition = True
Expand Down
2 changes: 1 addition & 1 deletion torchvision/datasets/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def calculate_md5(fpath: str, chunk_size: int = 1024 * 1024) -> str:
# Setting the `usedforsecurity` flag does not change anything about the functionality, but indicates that we are
# not using the MD5 checksum for cryptography. This enables its usage in restricted environments like FIPS. Without
# it torchvision.datasets is unusable in these environments since we perform a MD5 check everywhere.
md5 = hashlib.md5(**dict(usedforsecurity=False) if sys.version_info >= (3, 9) else dict())
md5 = hashlib.md5(**dict(usedforsecurity=False) if sys.version_info >= (3, 9) else dict()) # type: ignore[arg-type]
with open(fpath, "rb") as f:
for chunk in iter(lambda: f.read(chunk_size), b""):
md5.update(chunk)
Expand Down