Type annotations for torchvision/utils.py

[pmeier]

Addresses #2025

[fmassa]

Looks great, thanks a lot!

I have a few comments.

Also, I was wondering if we should setup some CI to run mypy in the code (for the files that support type annotation). It would be as simple as writing something like

vision/.travis.yml

Lines 18 to 22 in 504d20c

	- env: LINT_CHECK
	python: "3.6"
	install: pip install flake8 typing
	script: flake8 .circleci
	after_success: []

in the travis CI, but installing mypy and running it in the files that have type annotation. Do you think you could do it in a follow-up PR?

[fmassa]

Also, test failures are related

TypeError: Optional[t] requires a single type. Got (<class 'str'>, <class '_io.FileIO'>).

[pmeier]

Could you link me to the failing test? I can't find it within circleci checks.

Edit: Found it in the Travis CI test. At least for me this is not shown within checks for this PR. Do you have an idea why?

Failure is already fixed through a commit addressing one of the review comments.

[pmeier]

Also, I was wondering if we should setup some CI to run mypy in the code (for the files that support type annotation).

Running mypy torchvision/utils.py right now results in two classes of errors:

1. torchvision/utils.py:108: error: Skipping analyzing 'PIL': found module but no type hints or library stubs
   Since Pillow does not provide annotations we either have to provide them ourselves or ignore PIL all together. While the first option sounds like a lot of work it really is not that much since we only need provide annotations for the stuff we are using. Here that would be the Image class as well as the fromarray and save method. I've that before in a project of mine if you want to have a look.
2. torchvision/ops/boxes.py:8: error: syntax error in type comment '(Tensor, Tensor, float)'
   I'm puzzled right now why mypy also checks torchvision/ops/*, but I think that makes it clear that a CI test as you are proposing will probably not work as intended until we reach a significant annotation coverage. I think including it into CI can help the reviewer that sense that he can see if there any conflicts within the processed file without locally checking out the PR. But if we do this we have to keep in mind that from now on all future PRs will potentially fail the CI checks until we finished annotating.

[fmassa]

@pmeier
1 -

Since Pillow does not provide annotations we either have to provide them ourselves or ignore PIL all together

Let's ignore PIL annotations (there is a way to tell mypy to ignore annotations from specific modules), and wait until PIL add type annotations to its functions

2 - I think we just need to properly make mypy ignore every folder / file except the ones that we want to test.

Additionally, instead of adding the mypy test in travis.yml, let's add it instead to CircleCI, similar to what we did in #2056. For some reason TravisCI has been unstable those past few days, so let's just finish moving away from it.

[pmeier]

I'll open a new PR for that and rebase this when the other is merged.

[fmassa]

Thanks a lot @pmeier !

[pmeier]

@fmassa I'm confused why you merged this. Does that mean you want me to continue without mypy running in CI or shall I hold further plans for type annotations until we figure that out?

[fmassa]

@pmeier this PR is very self-contained for now, and I think the next step should be to add CI for type annotations and targeting only this file -- this way we can have signal that our annotations are correct and compliant.

What do you think?