Skip to content

Remove legacy constructor calls from pytorch codebase. #54142

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

Remove legacy constructor calls from pytorch codebase. #54142

wants to merge 14 commits into from

Conversation

@ysiraichi
Copy link
Collaborator

@ysiraichi ysiraichi commented Mar 17, 2021

Follow up from #53889
Related to #47112

Removing every occurrence of the legacy constructor call present in PyTorch at:

  • docs
  • benchmarks
  • test
  • caffe2
  • CONTRIBUTING.md

@ysiraichi ysiraichi requested a review from glaringlee as a code owner March 17, 2021 12:31
@facebook-github-bot facebook-github-bot added oncall: jit Add this issue/PR to JIT oncall triage queue cla signed labels Mar 17, 2021
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Mar 17, 2021 •
edited
Loading

💊 CI failures summary and remediations

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

  • 1/1 failures possibly* introduced in this PR
    • 1/1 non-scanned 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 to the (internal) Dr. CI Users group.

@ysiraichi ysiraichi force-pushed the rm-legacy-ctor-rest branch from 3134806 to fc64e16 Compare March 19, 2021 09:57
rgommers
rgommers reviewed Mar 19, 2021
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 @ysiraichi. Overall looks good; my Tensor -> tensor default dtype question applies to all similar changes - have you verified whether the different behavior matters?

@codecov
Copy link

codecov bot commented Mar 19, 2021 •
edited
Loading

Codecov Report

Merging #54142 (0aefc85) into master (bc05867) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #54142   +/-   ##
=======================================
  Coverage   77.42%   77.42%           
=======================================
  Files        1895     1895           
  Lines      187524   187524           
=======================================
+ Hits       145194   145196    +2     
+ Misses      42330    42328    -2

ysiraichi
ysiraichi commented Mar 20, 2021
View reviewed changes
ysiraichi
ysiraichi commented Mar 20, 2021
View reviewed changes
ysiraichi
ysiraichi commented Mar 20, 2021
View reviewed changes
@ysiraichi ysiraichi force-pushed the rm-legacy-ctor-rest branch from 896b12c to 65538a0 Compare March 25, 2021 11:47
@ysiraichi ysiraichi force-pushed the rm-legacy-ctor-rest branch from 65538a0 to 0486ed1 Compare April 3, 2021 10:21
@ysiraichi ysiraichi force-pushed the rm-legacy-ctor-rest branch from c606e54 to 0aefc85 Compare April 7, 2021 08:40
@rgommers rgommers requested a review from mruberry April 9, 2021 13:33
@rgommers
Copy link
Collaborator

rgommers commented Apr 9, 2021

The one CI failure on ROCm is unrelated, this is ready to land. @mruberry could you please have a look at this PR?

mruberry
mruberry approved these changes Apr 11, 2021
View reviewed changes
Copy link
Collaborator

@mruberry mruberry left a comment

Choose a reason for hiding this comment

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

Impressively thorough, @ysiraichi, this is great!

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Apr 11, 2021

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

@mruberry
Copy link
Collaborator

mruberry commented Apr 11, 2021

What's your thinking for a next step, @ysiraichi?

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Apr 11, 2021

@mruberry merged this pull request in 93bf0ae.

@ysiraichi ysiraichi mentioned this pull request Apr 12, 2021
Closed
@ysiraichi
Copy link
Collaborator Author

ysiraichi commented Apr 12, 2021 •
edited
Loading

@mruberry So, I guess deprecating the legacy constructor caused creating instances of subclasses of Tensor to raise the warning introduced in #54414 (which I believe are not desirable). I opened an issue #55780 for addressing it.

@heitorschueroff
Copy link
Contributor

heitorschueroff commented Apr 12, 2021 •
edited
Loading

@mruberry So, I guess deprecating the legacy constructor caused creating instances of subclasses of Tensor to raise the warning introduced in #54414 (which I believe are not desirable). I opened an issue #55780 for addressing it.

@ysiraichi Thank you for opening this issue. Indeed we do not want to raise warning for supported features. This was my fault for not thoroughly considering all the use cases of the Tensor constructor. I appreciate your effort in helping make PyTorch better but for now I am going to revert the PR that added this warning (#54414). We should follow up on another way to deprecate this without affecting valid use cases.

@rafi-cohen rafi-cohen mentioned this pull request Apr 13, 2021
Closed
krshrimali pushed a commit to krshrimali/pytorch that referenced this pull request May 19, 2021
@ysiraichi
Remove legacy constructor calls from pytorch codebase. (pytorch#54142)
231bd45 
Summary:
Follow up from pytorch#53889
Related to pytorch#47112

Removing every occurrence of the legacy constructor call present in PyTorch at:
- _docs_
- _benchmarks_
- _test_
- _caffe2_
- _CONTRIBUTING.md_

Pull Request resolved: pytorch#54142

Reviewed By: ngimel

Differential Revision: D27699450

Pulled By: mruberry

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

Reviewers

@rgommers rgommers rgommers left review comments

@mruberry mruberry mruberry approved these changes

@glaringlee glaringlee Awaiting requested review from glaringlee

@BowenBao BowenBao Awaiting requested review from BowenBao

@neginraoof neginraoof Awaiting requested review from neginraoof

@spandantiwari spandantiwari Awaiting requested review from spandantiwari

Assignees

No one assigned

Labels

cla signed Merged oncall: jit Add this issue/PR to JIT oncall triage queue open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ysiraichi @facebook-github-bot @rgommers @mruberry @heitorschueroff @pytorchbot