Skip to content

Conversation

vfdev-5
Copy link
Collaborator

@vfdev-5 vfdev-5 commented Aug 25, 2022

and passing locally on python 3.9 without errors like:

# mypy --config-file mypy.ini
torchvision/datasets/utils.py:68: error: Argument 1 to "md5" has incompatible type "**Dict[str, bool]"; expected
"Union[bytes, Union[bytearray, memoryview, array[Any], mmap, _CData, PickleBuffer]]"  [arg-type]
        md5 = hashlib.md5(**dict(usedforsecurity=False) if sys.version_info >= (3, 9) else dict())

@NicolasHug
Copy link
Member

IIRC this only fails on 3.10. @pmeier had an attempt to fix this at #5755 but we weren't too enthusiastic about the solution

@vfdev-5
Copy link
Collaborator Author

vfdev-5 commented Aug 25, 2022

@NicolasHug yes, @pmeier pointed out to me his PR.

Let me recap the findings from my local tests using conda env on macosx :

code python version mypy check
main branch 3.8 failure with md5 = hashlib.md5(**dict(usedforsecurity=False) if sys.version_info >= (3, 9) else dict())
main branch 3.9 failure with md5 = hashlib.md5(**dict(usedforsecurity=False) if sys.version_info >= (3, 9) else dict())
main branch 3.10 failure with md5 = hashlib.md5(**dict(usedforsecurity=False) if sys.version_info >= (3, 9) else dict())
This PR 3.8 passing
This PR 3.9 passing
This PR 3.10 passing

mypy version: mypy 0.971 (compiled: yes)

@vfdev-5 vfdev-5 marked this pull request as ready for review August 25, 2022 12:34
Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

LGTM! It simplifies the code as well while not specifying Python to 3.10 as the previous attempt.

@datumbox datumbox requested a review from NicolasHug August 25, 2022 12:40
@pmeier
Copy link
Collaborator

pmeier commented Aug 25, 2022

It seems that mypy indeed recognizes the sys.version_info branch: https://mypy-play.net/?mypy=latest&python=3.10&gist=3f2a763676db35f08960e3bad08b62f3

while not specifying Python to 3.10 as the previous attempt.

My PR is not primarily about fixing this issue, but rather making these issues visible in CI. The change made in this PR cannot be verified by CI, but has to be done manually. Meaning, if someone introduces a change like I did in #5717, we will not know until someone on Python >= 3.7 is getting errors on main.

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

mypy will always surprise me at how it behaves so differently on 2 pieces of code which look like the exact same thing externally...

Thanks @vfdev-5, I agree we can merge as this issue has been an inconvenience for local development

@vfdev-5 vfdev-5 merged commit f82a467 into pytorch:main Aug 25, 2022
@vfdev-5 vfdev-5 deleted the try-fix-mypy-calculate_md5 branch August 25, 2022 13:59
@github-actions
Copy link

Hey @vfdev-5!

You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py

facebook-github-bot pushed a commit that referenced this pull request Aug 30, 2022
Reviewed By: NicolasHug

Differential Revision: D39131019

fbshipit-source-id: 926d5ad1055dc9e95c5f9f42382a506e1b604383
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants