Skip to content

Refactor: clean trainer device & distrib getters#5300

Merged
Borda merged 17 commits intorelease/1.2-devfrom
refactor/trainer-getters
Jan 12, 2021
Merged

Refactor: clean trainer device & distrib getters#5300
Borda merged 17 commits intorelease/1.2-devfrom
refactor/trainer-getters

Conversation

@Borda
Copy link
Copy Markdown
Collaborator

@Borda Borda commented Dec 30, 2020 •
edited
Loading

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
Copy Markdown

pep8speaks commented Dec 30, 2020 •
edited
Loading

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
Copy Markdown

codecov bot commented Dec 30, 2020 •
edited
Loading

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
Merged
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 mentioned this pull request Jan 1, 2021
Merged
12 tasks
@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
@Borda Borda added the ready to be merged PRs ready to be merged label Jan 11, 2021
awaelchli
awaelchli approved these changes Jan 11, 2021
View reviewed changes
Copy link
Copy Markdown
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

pytorch_lightning/trainer/deprecated_api.py
@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
Copy Markdown
Contributor

@awaelchli awaelchli Jan 11, 2021

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
Closed
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
Closed
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
@tarepan
Refactor with_gpu type with simple typing
ae13e0c 
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 Jan 22, 2021
Closed
16 tasks
@justusschock justusschock mentioned this pull request Feb 2, 2021
Merged
16 tasks
@elimohl elimohl mentioned this pull request Apr 2, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@justusschock justusschock justusschock approved these changes

@tchaton tchaton tchaton approved these changes

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

@lezwon lezwon Awaiting requested review from lezwon

@rohitgr7 rohitgr7 Awaiting requested review from rohitgr7

@SkafteNicki SkafteNicki Awaiting requested review from SkafteNicki

@yukw777 yukw777 Awaiting requested review from yukw777

@carmocca carmocca Awaiting requested review from carmocca

+2 more reviewers

@awaelchli awaelchli awaelchli approved these changes

@s-rog s-rog s-rog approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Borda Borda

Labels

priority: 1 Medium priority task ready to be merged PRs ready to be merged refactor

Projects

None yet

Milestone

pl:1.2

Development

Successfully merging this pull request may close these issues.

7 participants

@Borda @pep8speaks @tchaton @awaelchli @carmocca @justusschock @s-rog