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

Refactor: clean trainer device & distrib getters #5300

Merged
merged 17 commits into from
Jan 12, 2021

Conversation

Borda
Copy link
Member

@Borda Borda commented Dec 30, 2020

What does this PR do?

Fixes # (issue) <- this links related issue to this PR

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • 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 needed]?
  • Did you write any new necessary tests [no need for typos, docs]?
  • Did you verify new and existing tests pass locally with your changes?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

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
  • Check that target branch and milestone are aligned!

Did you have fun?

Make sure you had fun coding 🙃

@Borda Borda added the refactor label Dec 30, 2020
@Borda Borda added this to the 1.2 milestone Dec 30, 2020
@pep8speaks
Copy link

pep8speaks commented Dec 30, 2020

Hello @Borda! Thanks for updating this PR.

Line 190:13: W503 line break before binary operator
Line 201:13: W503 line break before binary operator

Line 295:21: W503 line break before binary operator

Line 156:17: W503 line break before binary operator
Line 157:17: W503 line break before binary operator

Comment last updated at 2021-01-12 08:41:03 UTC

@Borda Borda self-assigned this Dec 30, 2020
@codecov
Copy link

codecov bot commented Dec 30, 2020

Codecov Report

Merging #5300 (79ce3be) into release/1.2-dev (2373858) will increase coverage by 46%.
The diff coverage is 98%.

@@                Coverage Diff                @@
##           release/1.2-dev   #5300     +/-   ##
=================================================
+ Coverage               46%     93%    +46%     
=================================================
  Files                  151     151             
  Lines                10482   10616    +134     
=================================================
+ Hits                  4844    9831   +4987     
+ Misses                5638     785   -4853     

@Borda Borda changed the base branch from v1.1.2 to release/1.2-dev December 30, 2020 10:54
@Borda Borda changed the title Refactor: clean trainer device & distrib getters [blocked by #5303] Refactor: clean trainer device & distrib getters Dec 30, 2020
@Borda Borda mentioned this pull request Dec 30, 2020
12 tasks
@Borda Borda changed the title [blocked by #5303] Refactor: clean trainer device & distrib getters Refactor: clean trainer device & distrib getters Dec 31, 2020
@Borda Borda force-pushed the refactor/trainer-getters branch from 10d5136 to 0e562bb Compare December 31, 2020 10:32
@Borda Borda force-pushed the refactor/trainer-getters branch from 0e562bb to 7bfa0c5 Compare January 5, 2021 12:42
@Borda Borda requested review from awaelchli and tchaton and removed request for tchaton January 5, 2021 12:43
@Borda Borda added the priority: 1 Medium priority task label Jan 7, 2021
@Borda Borda force-pushed the refactor/trainer-getters branch from 988c352 to 9b9bb10 Compare January 10, 2021 21:12
@Borda Borda marked this pull request as ready for review January 10, 2021 22:01
Copy link
Contributor

@awaelchli awaelchli left a comment

Choose a reason for hiding this comment

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

@Borda may need some help from you to port these changes properly when we eventually rebase #5385

@@ -23,7 +23,7 @@ class DeprecatedDistDeviceAttributes:

@property
def on_cpu(self) -> bool:
# rank_zero_warn("Internal: `on_cpu` is deprecated in v1.1 and will be removed in v1.2.", DeprecationWarning)
rank_zero_warn("Internal: `on_cpu` is deprecated in v1.2 and will be removed in v1.4.", DeprecationWarning)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should make sure we don't use these properties internally otherwise users gets spammed with deprecation warnings and they can't do anything about it.

This already happens in some places with other deprecation warnings and this is very confusing to the user (warning utils)

@awaelchli awaelchli mentioned this pull request Jan 11, 2021
9 tasks
@Borda Borda force-pushed the refactor/trainer-getters branch from 79ce3be to a42e8c0 Compare January 12, 2021 08:39
@Borda Borda requested a review from carmocca January 12, 2021 08:39
@Borda Borda mentioned this pull request Jan 12, 2021
3 tasks
@Borda Borda enabled auto-merge (squash) January 12, 2021 10:20
@Borda Borda merged commit 54d20dc into release/1.2-dev Jan 12, 2021
@Borda Borda deleted the refactor/trainer-getters branch January 12, 2021 10:22
tarepan added a commit to tarepan/pytorch-lightning that referenced this pull request Jan 19, 2021
Normal and HPC load now use common GPU type check (Lightning-AI#5300).
Now that there is no needs of accepting both bool and int.
@justusschock justusschock mentioned this pull request Feb 2, 2021
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: 1 Medium priority task ready PRs ready to be merged refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants