Skip to content

Add type annotations to torch.overrides#48493

Closed
guilhermeleobas wants to merge 20 commits intopytorch:masterfrom
guilhermeleobas:override
Closed

Add type annotations to torch.overrides#48493
guilhermeleobas wants to merge 20 commits intopytorch:masterfrom
guilhermeleobas:override

Conversation

@guilhermeleobas
Copy link
Copy Markdown
Collaborator

@guilhermeleobas guilhermeleobas commented Nov 26, 2020

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
Copy Markdown

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
Copy Markdown

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
Copy Markdown
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.

Comment thread torch/overrides.py Outdated
@rgommers rgommers changed the title Override Add type annotations to torch.overrides Nov 27, 2020
rgommers
rgommers reviewed Dec 1, 2020
View reviewed changes
Comment thread torch/overrides.py Outdated
hameerabbasi
hameerabbasi approved these changes Jan 18, 2021
View reviewed changes
Copy link
Copy Markdown
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!

Comment thread torch/overrides.py Outdated
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
Copy Markdown
Collaborator

@hameerabbasi hameerabbasi Jan 18, 2021

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
Copy Markdown
Collaborator Author

@guilhermeleobas guilhermeleobas Jan 18, 2021

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
Copy Markdown
Collaborator

@hameerabbasi hameerabbasi Jan 18, 2021

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
Copy Markdown
Collaborator Author

@guilhermeleobas guilhermeleobas Jan 18, 2021

Choose a reason for hiding this comment

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

Done!

@hameerabbasi
Copy link
Copy Markdown
Collaborator

hameerabbasi commented Jan 18, 2021

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
Copy Markdown
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
Copy Markdown
Contributor

ezyang commented Jan 19, 2021

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

@guilhermeleobas
Copy link
Copy Markdown
Collaborator Author

guilhermeleobas commented Jan 20, 2021

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

@hameerabbasi
Copy link
Copy Markdown
Collaborator

hameerabbasi commented Jan 20, 2021

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
Copy Markdown
Contributor

facebook-github-bot commented Jan 20, 2021

@ezyang merged this pull request in 2ace4fc.

@samestep
Copy link
Copy Markdown
Contributor

samestep commented Jan 20, 2021

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
Copy Markdown
Collaborator Author

guilhermeleobas commented Jan 20, 2021

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

@mrshenli
Copy link
Copy Markdown
Contributor

mrshenli commented Jan 20, 2021

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
Copy Markdown
Contributor

facebook-github-bot commented Jan 20, 2021

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
laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
@guilhermeleobas @facebook-github-bot
Add type annotations to torch.overrides (pytorch#48493)
5f2ae6a 
Summary:
Fixes pytorch#48492

Pull Request resolved: pytorch#48493

Reviewed By: mruberry

Differential Revision: D25958987

Pulled By: ezyang

fbshipit-source-id: aadc065c489bf1a8c6258de14c930e396df763bc
laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
@guilhermeleobas @facebook-github-bot
Add type annotations to torch.overrides (pytorch#50824)
e7d4bc1 
Summary:
This is a follow up PR of pytorch#48493.

Fixes pytorch#48492

Pull Request resolved: pytorch#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

@ezyang ezyang Awaiting requested review from ezyang

@rgommers rgommers Awaiting requested review from rgommers

+1 more reviewer

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

Reviewers whose approvals may not affect merge requirements

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