Skip to content

Embed torch.nn typing annotations #43044

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
malfet wants to merge 5 commits into from
Closed

Embed torch.nn typing annotations #43044

malfet wants to merge 5 commits into from

Conversation

@malfet
Copy link
Contributor

@malfet malfet commented Aug 14, 2020

Delete several .pyi files and embed annotations from those files in respective .py

@malfet malfet added module: typing Related to mypy type annotations triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Aug 14, 2020
@malfet malfet requested a review from apaszke as a code owner August 14, 2020 01:52
facebook-github-bot
facebook-github-bot reviewed Aug 14, 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.

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

@dr-ci
Copy link

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

💊 CI failures summary and remediations

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

  • 2/2 failures possibly* introduced in this PR
    • 1/2 non-CircleCI failure(s)

XLA failure

Job pytorch_xla_linux_bionic_py3_6_clang9_test is failing. Please create an issue with title prefixed by [PT_BREAK] in pytorch/xla and link to to this PR. If you have questions, please reach out to @ailzhang / @dlibenzi / @JackCaoG.

ci.pytorch.org: 1 failed

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 9 times.

@malfet malfet force-pushed the malfet/embed-torch.nn-typing-anotations branch from 9565caf to 8be5f91 Compare August 14, 2020 06:56
@malfet
Embed typing into conver_parameters.py
a851f01 
Also add typing anotations to _check_param_device
@malfet malfet force-pushed the malfet/embed-torch.nn-typing-anotations branch from 8be5f91 to a851f01 Compare August 14, 2020 07:04
facebook-github-bot
facebook-github-bot reviewed Aug 14, 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.

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

rgommers
rgommers approved these changes Aug 14, 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.

LGTM, just one question about class attributes.

torch/nn/utils/spectral_norm.py
name: str
dim: int
n_power_iterations: int
eps: float
Copy link
Collaborator

@rgommers rgommers Aug 14, 2020

Choose a reason for hiding this comment

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

These seem unnecessary, mypy is happy with only the annotations of __init__() as far as I can tell. Adding these is harmless, but we want to make a habit of this?

Copy link
Contributor Author

@malfet malfet Aug 14, 2020

Choose a reason for hiding this comment

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

This PR just copies existing annotations.
I thought this is done to annotate class members, which are likely to be accessed outside of the class scope.

Copy link
Collaborator

@rgommers rgommers Aug 14, 2020

Choose a reason for hiding this comment

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

It's covered in PEP 526; several methods are valid it looks like. One could also leave these out, or use

self.name: str = name

inline instead. I don't have a strong preference either way.

malfet added a commit to malfet/xla that referenced this pull request Aug 14, 2020
@malfet
Update clip_grad_norm_ patched function signature
c3c37d1 
Make it match type-annotated signature from pytorch/pytorch#43044
@malfet malfet mentioned this pull request Aug 14, 2020
Merged
@malfet malfet deleted the malfet/embed-torch.nn-typing-anotations branch August 14, 2020 20:46
malfet added a commit to malfet/xla that referenced this pull request Aug 14, 2020
@malfet
Update clip_grad_norm_ patched function signature
4c87879 
Make it match type-annotated signature from pytorch/pytorch#43044
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Aug 15, 2020

@malfet merged this pull request in 1c6ace8.

@ezyang ezyang mentioned this pull request Jan 5, 2023
Closed
@ringohoffman ringohoffman mentioned this pull request Jul 26, 2024
Closed
pytorchmergebot pushed a commit that referenced this pull request Jul 31, 2024
@pytorchmergebot
Merge torch.nn.utils.rnn type stubs (#131872)
deb788f 
I want to re-attempt:

* #61467

See:

* #10536 (comment)

and this is one of the files I would touch.

quoting @ezyang:

* #91648 (comment)

> The back story here is that in #19089 we added pyi stubs for nn modules, but when we got off Python 2 we started merging the pyi stubs directly into the py files, e.g., as in #43044. But not all the modules got the treatment.

Pull Request resolved: #131872
Approved by: https://github.com/Skylion007, https://github.com/ezyang
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rgommers rgommers rgommers approved these changes

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

@ezyang ezyang ezyang approved these changes

@seemethere seemethere Awaiting requested review from seemethere

@scintiller scintiller Awaiting requested review from scintiller

@apaszke apaszke Awaiting requested review from apaszke

Assignees

No one assigned

Labels

Merged module: typing Related to mypy type annotations 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.

5 participants

@malfet @facebook-github-bot @ezyang @rgommers @mruberry