[Refactor] Improve loops API 1/n #8334

tchaton · 2021-07-08T10:42:40Z

What does this PR do?

This PR adds functionalities to the loop similar to torch nn.Module for checkpointing.

Next PR: #8362

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)
Did you list all the breaking changes introduced by this pull request?

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-07-08T10:43:51Z

Codecov Report

Merging #8334 (350abea) into master (e9d0fe8) will decrease coverage by 5%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #8334    +/-   ##
=======================================
- Coverage      93%     88%    -5%     
=======================================
  Files         214     216     +2     
  Lines       13914   14097   +183     
=======================================
- Hits        12897   12404   -493     
- Misses       1017    1693   +676

pytorch_lightning/loops/base.py

justusschock

TBH: I am not sure why we really need most of the stuff here. I left some comments, but IMO all the information about parents and children is not really required.

pytorch_lightning/loops/base.py

pep8speaks · 2021-07-12T08:04:12Z

Hello @tchaton! Thanks for updating this PR.

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

Comment last updated at 2021-07-12 09:47:46 UTC

for more information, see https://pre-commit.ci

CHANGELOG.md

justusschock

Looks good to me overall. Just some comments

CHANGELOG.md

pytorch_lightning/loops/base.py

tests/loops/test_loop_state_dict.py

carmocca · 2021-07-12T11:45:56Z

pytorch_lightning/loops/base.py

@@ -88,11 +102,7 @@ def run(self, *args: Any, **kwargs: Any) -> Optional[Any]:
        if self.skip:
            return self.on_skip()

-        if self.restarting:


So why do we not want the restarting property anymore?

And why did you leave it in the __init__?

This is still used in reset self.restarting. Check #8364.

Adrian convinced me it was cleaner to make it manual.

self.restarting is automatically set to True by the Trainer when calling loop load_state_dict function.

Where exactly is that PR setting restarting on the loops?

Hey @carmocca, right there: https://github.com/PyTorchLightning/pytorch-lightning/pull/8334/files#diff-4076755145b42e55247c8c12a7a7961a91f4d28a3e962c80a98161163bfa3ec9R205.

Each loop is going to load its own checkpoint and be put in restarting mode.

tests/loops/test_loops.py

resolve issues

fe55d6e

tchaton added the refactor label Jul 8, 2021

tchaton added this to the v1.4 milestone Jul 8, 2021

tchaton requested a review from awaelchli as a code owner July 8, 2021 10:42

tchaton self-assigned this Jul 8, 2021

tchaton requested review from Borda, carmocca, justusschock, kaushikb11, SeanNaren and williamFalcon as code owners July 8, 2021 10:42

tchaton added 5 commits July 8, 2021 15:02

update

4ee4a73

update

1291418

update

0e69bea

add more exceptions

368e179

resolve bug

eb4475c

awaelchli reviewed Jul 8, 2021

View reviewed changes

pytorch_lightning/loops/base.py Outdated Show resolved Hide resolved

pytorch_lightning/loops/base.py Outdated Show resolved Hide resolved

tchaton added 4 commits July 8, 2021 16:36

update

449ca62

update

cdf38f0

update changelog

88bafaf

resolve bug

0981e94

justusschock reviewed Jul 8, 2021

View reviewed changes

edenlightning modified the milestones: v1.4, v1.5 Jul 9, 2021

tchaton added 5 commits July 10, 2021 11:59

resolve comments

7f8000f

update

4153b81

update

d6280e0

update changelog

c499c24

update

3cb6df2

tchaton added 2 commits July 10, 2021 13:19

remove space

df4b1ba

update

ee8d9b8

tchaton mentioned this pull request Jul 10, 2021

Add progress tracking on Loops - 2/n #8362

Merged

12 tasks

tchaton modified the milestones: v1.5, v1.4 Jul 10, 2021

tchaton changed the title ~~[Refactor] Improve loops API~~ [Refactor] Improve loops API 1/n Jul 11, 2021

tchaton mentioned this pull request Jul 11, 2021

[Feat] Add DataLoader state dict to Loop state dict - 5/n #8370

Closed

12 tasks

awaelchli added 3 commits July 12, 2021 09:30

re-order protected trainer attr

794522d

move public method up

1136428

add docs to state dict methods

60e3f28

awaelchli reviewed Jul 12, 2021

View reviewed changes

pytorch_lightning/loops/base.py Outdated Show resolved Hide resolved

pytorch_lightning/loops/base.py Outdated Show resolved Hide resolved

awaelchli added 2 commits July 12, 2021 10:02

combine __load with load_state_dict

fbc7168

rename shadowed variable

407a1ac

pre-commit-ci bot and others added 2 commits July 12, 2021 08:05

[pre-commit.ci] auto fixes from pre-commit.com hooks

b229828

for more information, see https://pre-commit.ci

move changelog entry to refactor section

683e8ea

awaelchli reviewed Jul 12, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

awaelchli added 2 commits July 12, 2021 10:59

refactor loop_progress property for test helper function

fecd58f

update trainer setter docstring

c0698be

justusschock approved these changes Jul 12, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

pytorch_lightning/loops/base.py Outdated Show resolved Hide resolved

pytorch_lightning/loops/base.py Show resolved Hide resolved

pytorch_lightning/loops/base.py Outdated Show resolved Hide resolved

justusschock added 2 commits July 12, 2021 11:04

Update CHANGELOG.md

2470a7b

Update pytorch_lightning/loops/base.py

19183a8

awaelchli approved these changes Jul 12, 2021

View reviewed changes

remove trainer check

350abea

SeanNaren approved these changes Jul 12, 2021

View reviewed changes

tchaton enabled auto-merge (squash) July 12, 2021 10:01

tchaton merged commit 370fa67 into master Jul 12, 2021

tchaton deleted the loop_improvement branch July 12, 2021 11:13

carmocca reviewed Jul 12, 2021

View reviewed changes

carmocca mentioned this pull request Sep 22, 2021

Use completed over processed in reset_on_restart #9656

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Refactor] Improve loops API 1/n #8334

[Refactor] Improve loops API 1/n #8334

tchaton commented Jul 8, 2021 •

edited by awaelchli

Loading

codecov bot commented Jul 8, 2021 •

edited

Loading

justusschock left a comment

pep8speaks commented Jul 12, 2021 •

edited

Loading

justusschock left a comment

carmocca Jul 12, 2021

tchaton Jul 12, 2021

carmocca Jul 12, 2021

tchaton Jul 13, 2021

[Refactor] Improve loops API 1/n #8334

[Refactor] Improve loops API 1/n #8334

Conversation

tchaton commented Jul 8, 2021 • edited by awaelchli Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

codecov bot commented Jul 8, 2021 • edited Loading

Codecov Report

justusschock left a comment

Choose a reason for hiding this comment

pep8speaks commented Jul 12, 2021 • edited Loading

Comment last updated at 2021-07-12 09:47:46 UTC

justusschock left a comment

Choose a reason for hiding this comment

carmocca Jul 12, 2021

Choose a reason for hiding this comment

tchaton Jul 12, 2021

Choose a reason for hiding this comment

carmocca Jul 12, 2021

Choose a reason for hiding this comment

tchaton Jul 13, 2021

Choose a reason for hiding this comment

tchaton commented Jul 8, 2021 •

edited by awaelchli

Loading

codecov bot commented Jul 8, 2021 •

edited

Loading

pep8speaks commented Jul 12, 2021 •

edited

Loading