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.

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

Deprecate Trainer.root_gpu #12064

Closed
wants to merge 8 commits into from
Closed

Deprecate Trainer.root_gpu #12064

wants to merge 8 commits into from

Conversation

vpavankalyan
Copy link

@vpavankalyan vpavankalyan commented Feb 23, 2022 •
edited
Loading

What does this PR do?

Deprecate Trainer.root_gpu and recommends using trainer.strategy.root_device and replace the test that uses it with Trainer.strategy.root_device

Fixes #11994

Does your PR introduce any breaking changes? If yes, please list them.

None

Before submitting

  • Was this discussed/approved via a GitHub issue? (not for typos and docs)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you list all the breaking changes introduced by this pull request?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Yes 😄

@vpavankalyan vpavankalyan marked this pull request as draft February 23, 2022 09:54
@vpavankalyan vpavankalyan force-pushed the feature/11994_Deprecate-root_gpu branch from 0587208 to 46af892 Compare February 23, 2022 11:47
carmocca
carmocca reviewed Feb 23, 2022
View reviewed changes
Copy link
Contributor

@carmocca carmocca left a comment

Choose a reason for hiding this comment

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

Can you add a test for the deprecation message in tests/deprecated_api/test_remove_1-8.py?

pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved
@carmocca carmocca added deprecation Includes a deprecation trainer labels Feb 23, 2022
ananthsub and others added 2 commits February 28, 2022 23:41
@ananthsub @carmocca
Update pytorch_lightning/trainer/trainer.py
a53130e 
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
f311944 
for more information, see https://pre-commit.ci
@ananthsub
Copy link
Contributor

Hi @vpavankalyan , any updates?

@vpavankalyan
Copy link
Author

Hi @ananthsub,
Was genuinely held back with other work, but I am free now and will continue to work on this issue.
Thanks for checking in on me.

carmocca
carmocca reviewed Mar 3, 2022
View reviewed changes
Copy link
Contributor

@carmocca carmocca left a comment

Choose a reason for hiding this comment

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

The deprecation test needs to be added to this file, for example, you can copy this one but for root_gpu:

https://github.com/PyTorchLightning/pytorch-lightning/blob/15364c18c81909e213f1910a57982f100f8a356d/tests/deprecated_api/test_remove_1-8.py#L164-L169

pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved
tests/models/data/horovod/train_default_model.py Outdated Show resolved Hide resolved
tests/models/test_gpu.py Outdated Show resolved Hide resolved
tests/models/test_gpu.py Outdated Show resolved Hide resolved
tests/models/test_gpu.py Outdated Show resolved Hide resolved
tests/models/test_gpu.py Outdated Show resolved Hide resolved
vpavankalyan and others added 3 commits March 4, 2022 19:57
@vpavankalyan @carmocca
Updating root_gpu for returning index
6e7d044 
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
@vpavankalyan @carmocca
Updated depreciation message for root_gpu
a923224 
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
@vpavankalyan @carmocca
Updated tests for root_gpu with strategy.root_device
5624649 
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
ananthsub
ananthsub reviewed Mar 4, 2022
View reviewed changes
CHANGELOG.md
@@ -354,6 +354,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).

- Deprecated `Trainer.lr_schedulers` in favor of `Trainer.lr_scheduler_configs` which returns a list of dataclasses instead of dictionaries ([#11443](https://github.com/PyTorchLightning/pytorch-lightning/pull/11443))

- Deprecated `Trainer.root_gpu` in favor of `Trainer.strategy.root_device` ([#11994](https://github.com/PyTorchLightning/pytorch-lightning/pull/11994))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Deprecated `Trainer.root_gpu` in favor of `Trainer.strategy.root_device` ([#11994](https://github.com/PyTorchLightning/pytorch-lightning/pull/11994))
- Deprecated `Trainer.root_gpu` in favor of `Trainer.strategy.root_device.index` ([#11994](https://github.com/PyTorchLightning/pytorch-lightning/pull/11994))

@vpavankalyan
Copy link
Author

As PR #12262 solves the issue, I am closing this Draft PR.
Being a beginner learned a lot of things and I would like to thank the community for the opportunity and especially @carmocca and @ananthsub for their support.
On to the next one 🚀 🚀

@DuYicong515
Copy link
Contributor

DuYicong515 commented Mar 11, 2022 •
edited
Loading

Sorry I didn't notice that you've worked on this PR already, my bad!
Thanks for working on it @vpavankalyan. Look forward to your next contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carmocca carmocca carmocca left review comments

@ananthsub ananthsub ananthsub left review comments

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

@tchaton tchaton Awaiting requested review from tchaton tchaton is a code owner

@awaelchli awaelchli Awaiting requested review from awaelchli

@Borda Borda Awaiting requested review from Borda Borda is a code owner

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

@justusschock justusschock Awaiting requested review from justusschock justusschock is a code owner

@kaushikb11 kaushikb11 Awaiting requested review from kaushikb11

@rohitgr7 rohitgr7 Awaiting requested review from rohitgr7

@lantiga lantiga Awaiting requested review from lantiga lantiga will be requested when the pull request is marked ready for review lantiga is a code owner

Assignees
No one assigned
Labels
deprecation Includes a deprecation trainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deprecate Trainer.root_gpu
4 participants
@vpavankalyan @ananthsub @DuYicong515 @carmocca