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.

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

[reland] Remove balance and devices parameter from Pipe. #48432

Closed

Conversation

pritamdamania87
Copy link
Contributor

@pritamdamania87 pritamdamania87 commented Nov 24, 2020

Stack from ghstack:

As per our design in #44827,
changign the API such that the user places modules on appropriate devices
instead of having a balance and devices parameter that decides this.

This design allows us to use RemoteModule in the future.

Differential Revision: D25172970

As per our design in #44827,
changign the API such that the user places modules on appropriate devices
instead of having a `balance` and `devices` parameter that decides this.

This design allows us to use RemoteModule in the future.
ghstack-source-id: 116479842

Differential Revision: [D25172970](https://our.internmc.facebook.com/intern/diff/D25172970/)

[ghstack-poisoned]
@facebook-github-bot facebook-github-bot added cla signed oncall: distributed Add this issue/PR to distributed oncall triage queue labels Nov 24, 2020
pritamdamania87 pushed a commit that referenced this pull request Nov 24, 2020
As per our design in #44827,
changign the API such that the user places modules on appropriate devices
instead of having a `balance` and `devices` parameter that decides this.

This design allows us to use RemoteModule in the future.
ghstack-source-id: 116479842

Differential Revision: [D25172970](https://our.internmc.facebook.com/intern/diff/D25172970/)

ghstack-source-id: 117361676
Pull Request resolved: #48432
@dr-ci
Copy link

dr-ci bot commented Nov 24, 2020

💊 CI failures summary and remediations

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



3 failures not recognized by patterns:

Job Step Action
CircleCI pytorch_linux_xenial_py3_clang5_asan_build Build 🔁 rerun
CircleCI pytorch_libtorch_linux_xenial_cuda11_1_cudnn8_py3_gcc7_build Set Up CI Environment After attach_workspace 🔁 rerun
CircleCI pytorch-linux-xenial-py3-clang5-android-ndk-r19c-gradle-custom-build-single Set Up CI Environment After attach_workspace 🔁 rerun

🚧 1 fixed upstream failure:

These were probably caused by upstream breakages that were already fixed.

Please rebase on the viable/strict branch (expand for instructions)

If your commit is newer than viable/strict, you can try basing on an older, stable commit:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase --onto FETCH_HEAD $(git merge-base origin/master HEAD)

If your commit is older than viable/strict:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD

Check out the recency history of this "viable master" tracking branch.


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

As per our design in #44827,
changign the API such that the user places modules on appropriate devices
instead of having a `balance` and `devices` parameter that decides this.

This design allows us to use RemoteModule in the future.

Differential Revision: [D25172970](https://our.internmc.facebook.com/intern/diff/D25172970/)

[ghstack-poisoned]
pritamdamania87 pushed a commit that referenced this pull request Nov 26, 2020
Pull Request resolved: #48432

As per our design in #44827,
changign the API such that the user places modules on appropriate devices
instead of having a `balance` and `devices` parameter that decides this.

This design allows us to use RemoteModule in the future.
ghstack-source-id: 117378115
ghstack-source-id: 117378115

Differential Revision: [D25172970](https://our.internmc.facebook.com/intern/diff/D25172970/)
@codecov
Copy link

codecov bot commented Nov 26, 2020

Codecov Report

Merging #48432 (c28a7dd) into gh/pritamdamania87/182/base (5bb2a87) will decrease coverage by 0.14%.
The diff coverage is 61.76%.

@@                       Coverage Diff                       @@
##           gh/pritamdamania87/182/base   #48432      +/-   ##
===============================================================
- Coverage                        80.91%   80.76%   -0.15%     
===============================================================
  Files                             1855     1856       +1     
  Lines                           200241   200130     -111     
===============================================================
- Hits                            162023   161639     -384     
- Misses                           38218    38491     +273     

@pritamdamania87 pritamdamania87 changed the title Remove balance and devices parameter from Pipe. [reland] Remove balance and devices parameter from Pipe. Nov 30, 2020
As per our design in #44827,
changign the API such that the user places modules on appropriate devices
instead of having a `balance` and `devices` parameter that decides this.

This design allows us to use RemoteModule in the future.

Differential Revision: [D25172970](https://our.internmc.facebook.com/intern/diff/D25172970/)

[ghstack-poisoned]
pritamdamania87 pushed a commit that referenced this pull request Nov 30, 2020
Pull Request resolved: #48432

As per our design in #44827,
changign the API such that the user places modules on appropriate devices
instead of having a `balance` and `devices` parameter that decides this.

This design allows us to use RemoteModule in the future.
ghstack-source-id: 117491992
ghstack-source-id: 117491992

Differential Revision: [D25172970](https://our.internmc.facebook.com/intern/diff/D25172970/)

with pytest.raises(
ValueError,
match=r'should have all parameters on a single device, please use .to\(\)'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This message matching has changed and was failing lint earlier to the escape sequence \(

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 7c73fda.

1 similar comment
@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 7c73fda.

@facebook-github-bot facebook-github-bot deleted the gh/pritamdamania87/182/head branch December 5, 2020 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla signed Merged oncall: distributed Add this issue/PR to distributed oncall triage queue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants