Skip to content
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

Add type annotations to torch.overrides #48493

Closed
wants to merge 20 commits into from
Closed

Add type annotations to torch.overrides #48493

wants to merge 20 commits into from

Conversation

guilhermeleobas
Copy link
Collaborator

Fixes #48492

@guilhermeleobas guilhermeleobas added the module: typing Related to mypy type annotations label Nov 26, 2020
@guilhermeleobas guilhermeleobas self-assigned this Nov 26, 2020
@facebook-github-bot facebook-github-bot added cla signed oncall: distributed Add this issue/PR to distributed oncall triage queue labels Nov 26, 2020
@dr-ci
Copy link

dr-ci bot commented Nov 26, 2020 •
edited by facebook-github-bot
Loading

💊 CI failures summary and remediations

As of commit 3a123bc (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 to the (internal) Dr. CI Users group.

@codecov
Copy link

codecov bot commented Nov 26, 2020 •
edited
Loading

Codecov Report

Merging #48493 (948b88f) into master (7f3a407) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #48493      +/-   ##
==========================================
+ Coverage   80.66%   80.71%   +0.04%     
==========================================
  Files        1913     1904       -9     
  Lines      208058   206632    -1426     
==========================================
- Hits       167833   166775    -1058     
+ Misses      40225    39857     -368

@guilhermeleobas guilhermeleobas marked this pull request as ready for review November 27, 2020 00:26
@mruberry mruberry requested a review from ezyang November 27, 2020 06:39
@mruberry mruberry added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Nov 27, 2020
rgommers
rgommers requested changes Nov 27, 2020
View reviewed changes
Copy link
Collaborator

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks @guilhermeleobas. It would be good to not add too many unnecessary ignores, but just fix the missing annotations.

torch/overrides.py Outdated Show resolved Hide resolved
@rgommers rgommers changed the title Override Add type annotations to torch.overrides Nov 27, 2020
rgommers
rgommers reviewed Dec 1, 2020
View reviewed changes
torch/overrides.py Outdated Show resolved Hide resolved
hameerabbasi
hameerabbasi approved these changes Jan 18, 2021
View reviewed changes
Copy link
Collaborator

@hameerabbasi hameerabbasi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @guilhermeleobas!

torch/overrides.py Outdated
Tensor._grad.__get__: lambda self: -1,
Tensor._grad_fn.__get__: lambda self: -1,
Tensor.grad_fn.__get__: lambda self: -1,
Tensor._version.__get__: lambda self: -1,
Tensor._version.__get__: lambda self: -1, # type: ignore[attr-defined]
Copy link
Collaborator

Choose a reason for hiding this comment

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

If mypy doesn't recognize this, we should file a missing annotation report.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, Tensor._version is already annotated and this type: ignore can be safely removed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, in that case I'd expect all the X.__get__ ignores can be removed. Please do so.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

@hameerabbasi
Copy link
Collaborator

In general, this LGTM, but I'd double-check if the ignores in the diff are necessary.

facebook-github-bot
facebook-github-bot reviewed Jan 19, 2021
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 Jan 19, 2021

Looks like there are still some failures. Happy to merge once they're resolved.

@guilhermeleobas
Copy link
Collaborator Author

I don't know why but the facebook bot just closed this PR.

@hameerabbasi
Copy link
Collaborator

I don't know why but the facebook bot just closed this PR.

It got merged, you should make a follow-up PR.

@guilhermeleobas guilhermeleobas deleted the override branch January 20, 2021 14:47
@guilhermeleobas guilhermeleobas mentioned this pull request Jan 20, 2021
Closed
@facebook-github-bot
Copy link
Contributor

@ezyang merged this pull request in 2ace4fc.

@samestep
Copy link
Contributor

This seems to have broken the mypy test:

Traceback (most recent call last):
  File "test_type_hints.py", line 191, in test_run_mypy
    self.fail(f"mypy failed: {stdout} {stderr}")
AssertionError: mypy failed: torch/nn/modules/conv.py:1035: error: "Tensor" has no attribute "materialize"  [attr-defined]
torch/nn/modules/conv.py:1038: error: "Tensor" has no attribute "materialize"  [attr-defined]
Found 2 errors in 1 file (checked 1199 source files)

@guilhermeleobas
Copy link
Collaborator Author

@samestep, I have another PR opened fixing this:
#50813

@mrshenli
Copy link
Contributor

Hey @guilhermeleobas, this breaks several tests on master, which hides real test signals. Landing a new PR would take a few hours. To avoid blocking other developers, I am going to revert this one. Please reland with your fix. Thanks!

@facebook-github-bot
Copy link
Contributor

This pull request has been reverted by 1f5c3b3.

@guilhermeleobas guilhermeleobas restored the override branch January 20, 2021 17:50
@guilhermeleobas guilhermeleobas mentioned this pull request Jan 20, 2021
Closed
facebook-github-bot pushed a commit that referenced this pull request Jan 25, 2021
@guilhermeleobas @facebook-github-bot
Add type annotations to torch.overrides (#50824)
9dfbfe9 
Summary:
This is a follow up PR of #48493.

Fixes #48492

Pull Request resolved: #50824

Reviewed By: bdhirsh

Differential Revision: D26050736

Pulled By: ezyang

fbshipit-source-id: 049605fd271cff28c8b6e300c163e9df3b3ea23b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hameerabbasi hameerabbasi hameerabbasi approved these changes

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

@ezyang ezyang Awaiting requested review from ezyang

@rgommers rgommers Awaiting requested review from rgommers

Assignees

@guilhermeleobas guilhermeleobas

Labels
cla signed Merged module: typing Related to mypy type annotations oncall: distributed Add this issue/PR to distributed oncall triage queue open source Reverted 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.

Enable torch.overrides typechecks during CI
9 participants
@guilhermeleobas @ezyang @hameerabbasi @facebook-github-bot @samestep @mrshenli @rgommers @pytorchbot @mruberry