make gpus=str in Trainer consistent with command line parsing of string #6388

awaelchli · 2021-03-07T04:52:08Z

What does this PR do?

Current behavior:

# Selects GPU device 3
Trainer(gpus="3")

Desired behavior:

# Selects first 3 GPUS
Trainer(gpus="3")

This change makes the string type parsing for the gpus flag consistent with how the command line arguments are parsed.
**This PR announces the change today (from 1.3), the changes will take effect in 1.5 (breaking change). **

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 update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
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?

Make sure you had fun coding 🙃

codecov · 2021-03-07T04:54:10Z

Codecov Report

Merging #6388 (fe4f636) into master (597b309) will decrease coverage by 0%.
The diff coverage is 85%.

@@          Coverage Diff           @@
##           master   #6388   +/-   ##
======================================
- Coverage      91%     91%   -0%     
======================================
  Files         200     200           
  Lines       12896   12985   +89     
======================================
+ Hits        11746   11799   +53     
- Misses       1150    1186   +36

This reverts commit c5f3893.

carmocca

No strong feelings for either a) or b). But if you choose b), you could leave all this logic for the future behaviour implemented already by checking the current lightning version. Just in case we forget about this.

Then when 1.5 comes we just have to remove the old code instead of resolving TODOs

tests/models/test_gpu.py

edenlightning · 2021-03-09T15:19:56Z

I really think we should avoid breaking changes, esp when this is unexpected.

awaelchli · 2021-03-09T15:27:55Z

@edenlightning so you want me to close this PR? this would mean we keep the current behavior, nothing changes, and also close the linked issue.

Borda · 2021-03-14T23:48:19Z

@awaelchli still draft?

awaelchli · 2021-03-18T10:19:02Z

@Borda it's 95% done, just waiting for a go/no-go decision here.
If backward incompatible changes are not acceptable, this PR can be closed. @edenlightning

pytorch_lightning/utilities/device_parser.py

kaushikb11

LGTM!

pytorch_lightning/utilities/device_parser.py

Co-authored-by: Kaushik B <[email protected]>

pytorch_lightning/utilities/device_parser.py

carmocca · 2021-05-03T13:07:40Z

@awaelchli I see you modified the milestone here. Note that if this goes into 1.4 you'll have to update the deprecation warnings

awaelchli · 2021-05-03T13:17:09Z

Yes this got pushed back so I'll pick it up after 1.3. Thanks.

pep8speaks · 2021-05-04T07:12:19Z

Hello @awaelchli! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-05-04 09:03:46 UTC

Borda · 2021-05-04T07:13:58Z

docs/source/advanced/multi_gpu.rst

+.. warning::
+    The behavior for :code:`gpus="3"` (str) will change. Currently it selects the GPU with index 3, but will
+    select the first 3 GPUs from v1.5.


why are we announcing a change in 2 versions ahead?

are you saying one version is enough? I followed deprecation guideline of two versions.

tchaton

I am for it. BC should be prevented when possible, unless it is to correct a behaviour.

tchaton · 2021-05-04T08:08:15Z

tests/models/test_gpu.py

 from tests.helpers import BoringModel
 from tests.helpers.datamodules import ClassifDataModule
 from tests.helpers.imports import Batch, Dataset, Example, Field, LabelField
 from tests.helpers.runif import RunIf
 from tests.helpers.simple_models import ClassificationModel

+PL_VERSION_LT_1_5 = _compare_version("pytorch_lightning", operator.lt, "1.5")


awaelchli added 2 commits March 7, 2021 05:46

string gpu input

819a087

update docs

c5f3893

awaelchli added feature Is an improvement or enhancement design Includes a design discussion labels Mar 7, 2021

awaelchli added this to the 1.3 milestone Mar 7, 2021

awaelchli added 6 commits March 7, 2021 06:11

deprecation warning

875e472

Revert "update docs"

02850ed

This reverts commit c5f3893.

deprecation

a994daf

add changelog

7d9bb44

update parser

84cae6b

update warning

b41e5a6

awaelchli mentioned this pull request Mar 7, 2021

cli: Confused on (str, int, List[int]) variants for argparse for --gpus flag? #6228

Closed

carmocca reviewed Mar 7, 2021

View reviewed changes

tests/models/test_gpu.py Show resolved Hide resolved

Merge branch 'master' into bugfix/str-gpu

623feb3

Merge branch 'master' into bugfix/str-gpu

318b97c

awaelchli added 3 commits March 15, 2021 01:10

implement v1.5 behavior ahead of time

975aab7

formatting

08a3ba8

set accelerator in test to avoid different warning

76b927a

awaelchli closed this Apr 20, 2021

Borda deleted the bugfix/str-gpu branch April 23, 2021 07:29

awaelchli mentioned this pull request Apr 28, 2021

Add warning on argparse gpus flag #7236

Closed

awaelchli restored the bugfix/str-gpu branch May 2, 2021 20:18

Merge branch 'master' into bugfix/str-gpu

ca603ba

awaelchli reopened this May 2, 2021

add warning

eac3926

awaelchli requested review from tchaton and williamFalcon as code owners May 2, 2021 21:20

kaushikb11 reviewed May 2, 2021

View reviewed changes

pytorch_lightning/utilities/device_parser.py Outdated Show resolved Hide resolved

kaushikb11 reviewed May 2, 2021

View reviewed changes

pytorch_lightning/utilities/device_parser.py Outdated Show resolved Hide resolved

kaushikb11 approved these changes May 2, 2021

View reviewed changes

kaushikb11 reviewed May 2, 2021

View reviewed changes

pytorch_lightning/utilities/device_parser.py Show resolved Hide resolved

awaelchli and others added 2 commits May 3, 2021 00:05

remove todo warn

50b13ba

Update pytorch_lightning/utilities/device_parser.py

b420d9e

Co-authored-by: Kaushik B <[email protected]>

ananthsub approved these changes May 2, 2021

View reviewed changes

kaushikb11 added the ready PRs ready to be merged label May 2, 2021

awaelchli modified the milestones: v1.3, v1.4 May 3, 2021

EricCousineau-TRI reviewed May 3, 2021

View reviewed changes

pytorch_lightning/utilities/device_parser.py Show resolved Hide resolved

awaelchli changed the title ~~make gpus=str in Trainer consistent with command line parsing of string~~ [wip] make gpus=str in Trainer consistent with command line parsing of string May 3, 2021

awaelchli changed the title ~~[wip] make gpus=str in Trainer consistent with command line parsing of string~~ make gpus=str in Trainer consistent with command line parsing of string May 3, 2021

awaelchli modified the milestones: v1.4, v1.3 May 3, 2021

mergify bot added the has conflicts label May 4, 2021

Merge branch 'master' into bugfix/str-gpu

16196fd

mergify bot removed the has conflicts label May 4, 2021

Borda approved these changes May 4, 2021

View reviewed changes

tchaton approved these changes May 4, 2021

View reviewed changes

resolve flake8

99b81a3

tchaton enabled auto-merge (squash) May 4, 2021 08:37

clarify int in message

fe4f636

tchaton merged commit a6aa1a0 into master May 4, 2021

tchaton deleted the bugfix/str-gpu branch May 4, 2021 09:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make gpus=str in Trainer consistent with command line parsing of string #6388

make gpus=str in Trainer consistent with command line parsing of string #6388

awaelchli commented Mar 7, 2021 •

edited by carmocca

Loading

codecov bot commented Mar 7, 2021 •

edited

Loading

carmocca left a comment

edenlightning commented Mar 9, 2021

awaelchli commented Mar 9, 2021

Borda commented Mar 14, 2021

awaelchli commented Mar 18, 2021

kaushikb11 left a comment

carmocca commented May 3, 2021

awaelchli commented May 3, 2021

pep8speaks commented May 4, 2021 •

edited

Loading

Borda May 4, 2021

awaelchli May 4, 2021

tchaton left a comment

tchaton May 4, 2021

make gpus=str in Trainer consistent with command line parsing of string #6388

make gpus=str in Trainer consistent with command line parsing of string #6388

Conversation

awaelchli commented Mar 7, 2021 • edited by carmocca Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

codecov bot commented Mar 7, 2021 • edited Loading

Codecov Report

carmocca left a comment

Choose a reason for hiding this comment

edenlightning commented Mar 9, 2021

awaelchli commented Mar 9, 2021

Borda commented Mar 14, 2021

awaelchli commented Mar 18, 2021

kaushikb11 left a comment

Choose a reason for hiding this comment

carmocca commented May 3, 2021

awaelchli commented May 3, 2021

pep8speaks commented May 4, 2021 • edited Loading

Comment last updated at 2021-05-04 09:03:46 UTC

Borda May 4, 2021

Choose a reason for hiding this comment

awaelchli May 4, 2021

Choose a reason for hiding this comment

tchaton left a comment

Choose a reason for hiding this comment

tchaton May 4, 2021

Choose a reason for hiding this comment

awaelchli commented Mar 7, 2021 •

edited by carmocca

Loading

codecov bot commented Mar 7, 2021 •

edited

Loading

pep8speaks commented May 4, 2021 •

edited

Loading