Skip to content

Syntax error in type annotations #2061

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.

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
pmeier opened this issue Apr 3, 2020 · 5 comments
Closed

Syntax error in type annotations #2061

pmeier opened this issue Apr 3, 2020 · 5 comments

Comments

@pmeier
Copy link
Collaborator

pmeier commented Apr 3, 2020

In the light of first inline type annotations, a static type check via mypy was requested in #2034 (review).
Running mypy on the current torchvision results in syntax errors in several type annotations. For example:

torchvision/io/_video_opt.py:86: error: syntax error in type comment '(List[int])'

Since syntax errors are fatal they cannot be silenced even with ignore_errors set. Furthermore, mypy stops the checking after it encountered a module with syntax errors.

Thus, if we want to include mypy checks in CI and test if they work we need to either fix them or disable them temporarily and fix them later. Since they seem to be related to torch.jit, I have no idea how much work fixing them would be or how much damage temporarily disabling them will do.

@fmassa
Copy link
Member

fmassa commented Apr 3, 2020

I've noticed similar issues as well in the past, and I tried to fix some of those in #1696

We need those type annotations as those functions are being used with TorchScript, so we need to find a way to make them compliant with TorchScript and mypy.

cc @eellison for thoughts.

@pmeier
Copy link
Collaborator Author

pmeier commented Apr 3, 2020

You never merged the PR although @eellison approved it. Does that mean the way you did it does not work with TorchScript? If adding the return type as you did does indeed work with TorchScript could you rebase and merge your PR. I would take up the rest.

@fmassa
Copy link
Member

fmassa commented Apr 6, 2020

That PR had failing tests, and I never had the chance to get back to it and fix it so that it is compatible with mypy and torchscript.

@zhangguanheng66 are you still looking at fixing type annotations in #1696?

@zhangguanheng66
Copy link
Contributor

zhangguanheng66 commented Apr 6, 2020

I think the the output annotation Tuple[Tensor, Tensor] is not supported by torchscript. @eellison

It seems that a tuple of tensor is supported. For example, the multi_head_attention_forward function here

@pmeier
Copy link
Collaborator Author

pmeier commented May 17, 2020

This is fixed by #1696 and #2195.

@pmeier pmeier closed this as completed May 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants