Add tracking of basic states in Trainer [wip - to-be-merged after v0.9] #2541

zerogerc · 2020-07-07T16:53:40Z

What does this PR do?

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? Otherwise, we ask you to create a separate PR for every change.
Did you make sure to update the documentation with your changes?
Did you write any new necessary tests?
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.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

tests/trainer/test_states.py

codecov · 2020-07-07T17:04:33Z

Codecov Report

Merging #2541 into master will increase coverage by 32%.
The diff coverage is 97%.

@@           Coverage Diff            @@
##           master   #2541     +/-   ##
========================================
+ Coverage      58%     90%    +32%     
========================================
  Files          79      80      +1     
  Lines        7281    7282      +1     
========================================
+ Hits         4245    6574   +2329     
+ Misses       3036     708   -2328

pytorch_lightning/trainer/states.py

mergify · 2020-07-08T08:30:55Z

This pull request is now in conflict... :(

pytorch_lightning/trainer/states.py

zerogerc · 2020-07-09T08:46:12Z

pytorch_lightning/trainer/training_loop.py

@@ -397,6 +399,7 @@ def train(self):
            # user could press ctrl+c many times... only shutdown once
            if not self.interrupted:
                self.interrupted = True


I guess that self.interrupted can be removed in the future?

ye, more of them could be cleaned...

awaelchli · 2020-07-10T14:59:57Z

I was thinking maybe it would be better and cleaner if we made these state updates in decorators and add the decorators to methdods like the training loop, validation loop etc.
Haven't thought it through in detail. What do you think?
#1633 (comment)

zerogerc · 2020-07-13T08:17:12Z

Hi @awaelchli! Yes I think it could look nicer with decorators, I try to implement it and mention you as soon as it's ready.

williamFalcon · 2020-07-13T13:06:57Z

ummm. can you give an example?
i’d prefer to avoid decorators as much as possible

zerogerc · 2020-07-15T08:34:30Z

@williamFalcon my idea is something like this to wrap run functions (fit, test). However it requires a little bit of restructuring =)

def decorate_run(run_fn):
    def wrapper():
        try:
            state = RUNNING
            run_fn()
            state = FINISHED
        except KeyboardInterrupt as e:
            state = INTERRUPTED
    return wrapper

pytorch_lightning/trainer/states.py

tests/trainer/test_states.py

mergify · 2020-07-20T23:02:11Z

This pull request is now in conflict... :(

zerogerc · 2020-08-07T13:50:34Z

Sure. I could do it as well. Next PR

SkafteNicki

LGTM

…ack to a trainer.

…e, add tests for decorator only.

Borda · 2020-08-07T14:59:23Z

@zerogerc one test was failing, but it seems unrelated, so I have just rebased it again, let's see if it helps...

nateraw

LGTM - just a quick question...

nateraw · 2020-08-07T20:04:39Z

pytorch_lightning/trainer/trainer.py

@@ -396,6 +397,7 @@ def __init__(
        self.interrupted = False
        self.should_stop = False
        self.running_sanity_check = False
+        self.state = TrainerState.INITIALIZING


Here you set trainer's self.state to TrainerState.INITIALIZING...(see next comment)

nateraw · 2020-08-07T20:05:20Z

tests/trainer/test_states.py

+        return self.state
+
+    trainer = Trainer(default_root_dir=tmpdir)
+    trainer.state = TrainerState.INITIALIZING


Why are you setting trainer.state = TrainerState.INITIALIZING if its set in the init of Trainer? Should this be ==?

Suggested change

trainer.state = TrainerState.INITIALIZING

Could be wrong, just let me know what's going on here 😄

Very good point by nate, we should make the state read only property! and only the trainer should to write to the internal attribute trainer._state

awaelchli · 2020-08-07T20:14:05Z

@williamFalcon said he wants to wait with this, so we should probably wait for his approval before automerging

Borda · 2020-08-07T20:37:47Z

@williamFalcon said he wants to wait with this, so we should probably wait for his approval before auto merging

yes, this goes after v0.9, so I ll put there wip just in case...

mergify · 2020-08-08T09:09:02Z

This pull request is now in conflict... :(

williamFalcon · 2020-08-09T10:13:39Z

this looks great! let's merge it

Borda · 2020-08-09T10:30:27Z

this looks great! let's merge it

I know that's why we made it =) just you we a bit conservative and leave it for 0.9.1
@zerogerc let's update all tests checks accordingly #2541 (comment)

mergify bot requested a review from a team July 7, 2020 16:54

zerogerc commented Jul 7, 2020

View reviewed changes

tests/trainer/test_states.py Outdated Show resolved Hide resolved

Borda changed the title ~~feature/1633 Add tracking of basic states in Trainer.~~ Add tracking of basic states in Trainer. Jul 7, 2020

Borda added the feature Is an improvement or enhancement label Jul 7, 2020

Borda added this to the 0.9.0 milestone Jul 7, 2020

Borda reviewed Jul 7, 2020

View reviewed changes

pytorch_lightning/trainer/states.py Outdated Show resolved Hide resolved

mergify bot requested a review from a team July 7, 2020 17:42

Borda reviewed Jul 7, 2020

View reviewed changes

pytorch_lightning/trainer/states.py Outdated Show resolved Hide resolved

mergify bot requested a review from a team July 7, 2020 17:43

Borda reviewed Jul 7, 2020

View reviewed changes

pytorch_lightning/trainer/states.py Outdated Show resolved Hide resolved

Borda reviewed Jul 7, 2020

View reviewed changes

pytorch_lightning/trainer/states.py Outdated Show resolved Hide resolved

mergify bot requested a review from a team July 7, 2020 17:43

zerogerc force-pushed the feature/1633_trainer_phases branch from 68bddde to 17947ab Compare July 8, 2020 08:35

zerogerc commented Jul 8, 2020

View reviewed changes

pytorch_lightning/trainer/states.py Show resolved Hide resolved

zerogerc commented Jul 9, 2020

View reviewed changes

zerogerc force-pushed the feature/1633_trainer_phases branch from 17947ab to 4d533ed Compare July 16, 2020 11:36

awaelchli previously requested changes Jul 18, 2020

View reviewed changes

mergify bot requested a review from a team July 18, 2020 02:02

zerogerc requested review from awaelchli and removed request for a team July 20, 2020 09:20

mergify bot requested a review from a team July 20, 2020 09:21

zerogerc force-pushed the feature/1633_trainer_phases branch from c94404f to 47b3c6a Compare July 21, 2020 08:18

zerogerc removed the request for review from a team July 21, 2020 08:19

SkafteNicki approved these changes Aug 7, 2020

View reviewed changes

mergify bot requested a review from a team August 7, 2020 14:29

zerogerc added 5 commits August 7, 2020 17:55

Add initial tracking of states in Trainer.

36c579a

Add INTERRUPTED state, improve tests, move state switching from callb…

7bfd564

…ack to a trainer.

Move part of a trainer state switching to a decorator.

89dff65

Add documentation.

c61702f

Fix docs, rename state enum, restore state to previous on exit if Non…

c93d8c8

…e, add tests for decorator only.

Borda force-pushed the feature/1633_trainer_phases branch from 4af5fdd to b512b05 Compare August 7, 2020 14:56

Fix callback typing.

f9f762e

zerogerc force-pushed the feature/1633_trainer_phases branch from b512b05 to f9f762e Compare August 7, 2020 14:57

nateraw approved these changes Aug 7, 2020

View reviewed changes

Borda changed the title ~~Add tracking of basic states in Trainer.~~ Add tracking of basic states in Trainer [wip - to-be-merged after v0.9] Aug 7, 2020

Borda modified the milestones: 0.9.0, 0.9.x Aug 7, 2020

justusschock approved these changes Aug 8, 2020

View reviewed changes

awaelchli modified the milestones: 0.9.x, 1.0.0 Aug 8, 2020

Merge branch 'master' into feature/1633_trainer_phases

6b8858b

williamFalcon merged commit e9846dd into Lightning-AI:master Aug 9, 2020

Borda modified the milestones: 1.0.0, 0.9.0 Aug 20, 2020

zerogerc mentioned this pull request Aug 23, 2020

Make trainer.state a read-only property #3109

Merged

7 tasks

ZhaofengWu mentioned this pull request Dec 23, 2020

give "validation sanity check" flag for "validation_epoch_end" & "validation_step" #1391

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tracking of basic states in Trainer [wip - to-be-merged after v0.9] #2541

Add tracking of basic states in Trainer [wip - to-be-merged after v0.9] #2541

zerogerc commented Jul 7, 2020 •

edited

Loading

codecov bot commented Jul 7, 2020 •

edited

Loading

mergify bot commented Jul 8, 2020

zerogerc Jul 9, 2020

Borda Jul 9, 2020

awaelchli commented Jul 10, 2020

zerogerc commented Jul 13, 2020

williamFalcon commented Jul 13, 2020

zerogerc commented Jul 15, 2020 •

edited

Loading

mergify bot commented Jul 20, 2020

zerogerc commented Aug 7, 2020

SkafteNicki left a comment

Borda commented Aug 7, 2020

nateraw left a comment

nateraw Aug 7, 2020

nateraw Aug 7, 2020

nateraw Aug 7, 2020

nateraw Aug 7, 2020

awaelchli Aug 7, 2020

awaelchli commented Aug 7, 2020

Borda commented Aug 7, 2020

mergify bot commented Aug 8, 2020

williamFalcon commented Aug 9, 2020

Borda commented Aug 9, 2020

Add tracking of basic states in Trainer [wip - to-be-merged after v0.9] #2541

Add tracking of basic states in Trainer [wip - to-be-merged after v0.9] #2541

Conversation

zerogerc commented Jul 7, 2020 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

codecov bot commented Jul 7, 2020 • edited Loading

Codecov Report

mergify bot commented Jul 8, 2020

zerogerc Jul 9, 2020

Choose a reason for hiding this comment

Borda Jul 9, 2020

Choose a reason for hiding this comment

awaelchli commented Jul 10, 2020

zerogerc commented Jul 13, 2020

williamFalcon commented Jul 13, 2020

zerogerc commented Jul 15, 2020 • edited Loading

mergify bot commented Jul 20, 2020

zerogerc commented Aug 7, 2020

SkafteNicki left a comment

Choose a reason for hiding this comment

Borda commented Aug 7, 2020

nateraw left a comment

Choose a reason for hiding this comment

nateraw Aug 7, 2020

Choose a reason for hiding this comment

nateraw Aug 7, 2020

Choose a reason for hiding this comment

nateraw Aug 7, 2020

Choose a reason for hiding this comment

nateraw Aug 7, 2020

Choose a reason for hiding this comment

awaelchli Aug 7, 2020

Choose a reason for hiding this comment

awaelchli commented Aug 7, 2020

Borda commented Aug 7, 2020

mergify bot commented Aug 8, 2020

williamFalcon commented Aug 9, 2020

Borda commented Aug 9, 2020

zerogerc commented Jul 7, 2020 •

edited

Loading

codecov bot commented Jul 7, 2020 •

edited

Loading

zerogerc commented Jul 15, 2020 •

edited

Loading