Skip to content

add typing annotations for a few torch.utils.* modules #43806

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

add typing annotations for a few torch.utils.* modules #43806

guilhermeleobas wants to merge 5 commits into from

Conversation

@guilhermeleobas
Copy link
Collaborator

@guilhermeleobas guilhermeleobas commented Aug 28, 2020 •
edited by rgommers
Loading

Fixes #43431. Depends on gh-43862 (EDIT: now merged)

Modules:

  • torch.utils.mkldnn
  • torch.utils.mobile_optimizer
  • torch.utils.bundled_inputs

@guilhermeleobas guilhermeleobas added the module: typing Related to mypy type annotations label Aug 28, 2020
@guilhermeleobas guilhermeleobas self-assigned this Aug 28, 2020
@guilhermeleobas guilhermeleobas requested review from rgommers and removed request for rgommers August 28, 2020 21:39
rgommers
rgommers reviewed Aug 28, 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. The TorchType thing deserves some explanation. Overall looks good though.

@dr-ci
Copy link

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

💊 CI failures summary and remediations

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

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

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

@ailzhang ailzhang requested a review from rgommers August 31, 2020 22:29
@ailzhang ailzhang added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Aug 31, 2020
@codecov
Copy link

codecov bot commented Sep 1, 2020 •
edited
Loading

Codecov Report

Merging #43806 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #43806   +/-   ##
=======================================
  Coverage   69.24%   69.24%           
=======================================
  Files         381      381           
  Lines       47573    47574    +1     
=======================================
+ Hits        32943    32944    +1     
  Misses      14630    14630
Impacted Files Coverage Δ
torch/jit/_serialization.py 85.71% <100.00%> (ø)
torch/utils/bundled_inputs.py 93.44% <100.00%> (+0.10%) ⬆️
torch/utils/tensorboard/writer.py 76.20% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cce5982...ac92e5d. Read the comment docs.

@guilhermeleobas guilhermeleobas marked this pull request as draft September 3, 2020 18:24
@guilhermeleobas guilhermeleobas marked this pull request as ready for review September 8, 2020 21:49
@guilhermeleobas
Copy link
Collaborator Author

guilhermeleobas commented Sep 8, 2020

PR is ready for review. Failure doesn't seem to be caused by any changes introduced in this PR. cc @rgommers

@guilhermeleobas guilhermeleobas removed the request for review from apaszke September 8, 2020 21:50
SplitInfinity
SplitInfinity reviewed Sep 8, 2020
View reviewed changes
torch/_C/__init__.pyi.in
Comment on lines +634 to +636
class TensorType(JitType):
@classmethod
def get(cls) -> TensorType: ...
Copy link

@SplitInfinity SplitInfinity Sep 8, 2020

Choose a reason for hiding this comment

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

Just curious, why is this not a staticmethod with no args?

Copy link
Collaborator Author

@guilhermeleobas guilhermeleobas Sep 8, 2020

Choose a reason for hiding this comment

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

No particular reason. This code was introduced in a different PR. I just moved it to be close to the other methods that you defined before. I can the @classmethod to a @staticmethod if you want.

rgommers
rgommers approved these changes Sep 10, 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, ready to merge.

@rgommers
Copy link
Collaborator

rgommers commented Sep 10, 2020

@SplitInfinity since you've already reviewed this PR, would you be able to land it?

@SplitInfinity
Copy link

SplitInfinity commented Sep 10, 2020

I didn't quite review it but I can take charge of landing it, sure.

facebook-github-bot
facebook-github-bot reviewed Sep 10, 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.

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

@guilhermeleobas guilhermeleobas deleted the torch-utils branch September 11, 2020 17:42
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Sep 11, 2020

@SplitInfinity merged this pull request in cdf5e2a.

xuzhao9 pushed a commit that referenced this pull request Sep 18, 2020
@guilhermeleobas @xuzhao9
add typing annotations for a few torch.utils.* modules (#43806)
4951062 
Summary:
Fixes #43431. Depends on [gh-43862](#43862) (EDIT: now merged)

Modules:
- torch.utils.mkldnn
- torch.utils.mobile_optimizer
- torch.utils.bundled_inputs

Pull Request resolved: #43806

Reviewed By: gmagogsfm

Differential Revision: D23635151

Pulled By: SplitInfinity

fbshipit-source-id: a85b75a7927dde6cc55bcb361f8ff601ffb0b2a1
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

@rgommers rgommers rgommers approved these changes

+1 more reviewer

@SplitInfinity SplitInfinity SplitInfinity left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@guilhermeleobas guilhermeleobas

Labels

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

Enable torch.utils.(mkldnn/mobile_optimizer/bundled_inputs) typechecks during CI

7 participants

@guilhermeleobas @rgommers @SplitInfinity @facebook-github-bot @ailzhang @pytorchbot @mruberry