Skip to content

Improves type-checking guards. #43339

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

Jump to bottom
Closed
andyljones wants to merge 1 commit into from
Closed

Improves type-checking guards. #43339

andyljones wants to merge 1 commit into from

Conversation

@andyljones
Copy link
Contributor

@andyljones andyljones commented Aug 20, 2020 •
edited
Loading

PR #38157 fixed type checking for mypy by including if False guards on some type-checker-only imports. However other typecheckers - like pyright - will respect this logic and ignore the imports. Using if TYPE_CHECKING instead means both mypy and pyright will work correctly.

For background, an example of where the current code fails is if you make a file tmp.py with the contents

import torch
torch.ones((1,))

Then pyright tmp.py --lib will fail with a "ones" is not a known member of module error. This is because it can't find the _VariableFunctions.pyi stub file, as pyright respects the if False logic. After adding the TYPE_CHECKING guard, all works correctly.

Credit to @erictraut for suggesting the fix.

@dr-ci
Copy link

dr-ci bot commented Aug 20, 2020 •
edited
Loading

💊 CI failures summary and remediations

As of commit 5951221 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 13 times.

@andyljones andyljones mentioned this pull request Aug 20, 2020
Closed
@andyljones
Copy link
Contributor Author

andyljones commented Aug 20, 2020

The failed tests all seem to be of the form

Incompatible import of "set_flush_denormal" (imported name has type "Callable[[Arg(bool, 'mode')], bool]", local name has type "Callable[[Arg(bool, 'arg')], bool]")  [misc]

where the imported signature has a different parameter name from the local one, or of the form

Incompatible import of "namedtuple_values_indices" (imported name has type "Type[torch._C._VariableFunctions.namedtuple_values_indices]", local name has type "Type[torch._C.namedtuple_values_indices]")  [misc]

where _VariableFunctions.namedtuple has been collapsed down to namedtuple.

Searching for the names, I think the problem is a mismatch between torch/_C/__init__.pyi.in and tools/pyi/gen_pyi.py? And looking at the blames, the relevant lines of each file haven't been touched in a few months, so I think this is a genuine error being surfaced?

I think I can fix the parameter name mismatches easily enough, but the namedtuple chunk of gen_pyi.py is pretty intimidating. I'd appreciate it if someone with working knowledge of it could make the fix there.

@ngimel ngimel requested a review from ezyang August 25, 2020 04:31
@ngimel ngimel added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Aug 25, 2020
@ezyang
Copy link
Contributor

ezyang commented Aug 25, 2020 •
edited
Loading

Thanks @andyljones. I'd advise marking these # type: ignore for now so we can get this in. (And file an issue for it.)

@andyljones
Improves type-checking guards.
5951221 
PR pytorch#38157 fixed type checking for mypy by including `if False` guards
on some type-checker-only imports. However other typecheckers - like
pyright - will respect this logic and ignore the imports. Using
the TYPE_CHECKING var instead means both mypy and pyright will work.
@andyljones andyljones mentioned this pull request Aug 26, 2020
Open
@andyljones
Copy link
Contributor Author

andyljones commented Aug 26, 2020

I'd advise marking these # type: ignore for now so we can get this in. (And file an issue for it.)

Done & done.

facebook-github-bot
facebook-github-bot reviewed Aug 26, 2020
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ezyang
Copy link
Contributor

ezyang commented Sep 3, 2020

Pretty weird internal error that is blocking this land:

RuntimeError: Expected to not find "\n" but found it
  %14 : Double(2:1, requires_grad=1, device=cpu) = aten::tanh(%13) # /mnt/xarfuse/uid-30041/8add5f6c-seed-nspid4026533393-ns-4026533383/test_jit.py:1012:0
  %17 : Double(2:1, requires_grad=1, device=cpu) = aten::add(%14, %14, %6) # /mnt/xarfuse/uid-30041/8add5f6c-seed-nspid4026533393-ns-4026533383/test_jit.py:1012:0
  %27 : Double(2:1, requires_grad=1, device=cpu) = aten::add(%13, %17, %6) # /mnt/xarfuse/uid-30041/8add5f6c-seed-nspid4026533393-ns-4026533383/test_jit.py:1013:0
  return (%27)
From CHECK-NEXT: return
  File "/usr/local/fbcode/platform007/lib/python3.7/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/usr/local/fbcode/platform007/lib/python3.7/unittest/case.py", line 628, in run
    testMethod()
  File "/mnt/xarfuse/uid-30041/8add5f6c-seed-nspid4026533393-ns-4026533383/test_jit.py", line 1021, in test_cse
    .run(str(g))

@ezyang
Copy link
Contributor

ezyang commented Sep 3, 2020

nvm, it passed when i ran it locally

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Sep 3, 2020

@ezyang merged this pull request in 24ca6aa.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@ezyang ezyang ezyang approved these changes

@rgommers rgommers rgommers approved these changes

Assignees

No one assigned

Labels

Merged open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@andyljones @ezyang @facebook-github-bot @rgommers @ngimel @pytorchbot @mruberry