Refactor setup_training and remove test_mode

[rohitgr7]

What does this PR do?

Clean up and split setup_training into setup_trainer & setup_training to avoid call to on_pretrain_routine_* while testing. Removed redundant test_mode, use trainer.testing instead. Also a few minor updates, to make sure tests passes locally.

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

Did you have fun?

Make sure you had fun coding 🙃

[codecov]

Codecov Report

Merging #5388 (2e65f6a) into master (83b1ff4) will decrease coverage by 0%.
The diff coverage is 98%.

@@           Coverage Diff           @@
##           master   #5388    +/-   ##
=======================================
- Coverage      93%     93%    -0%     
=======================================
  Files         135     135            
  Lines       10015    9863   -152     
=======================================
- Hits         9338    9173   -165     
- Misses        677     690    +13     

[pep8speaks]

Hello @rohitgr7! Thanks for updating this PR.

• In the file pytorch_lightning/trainer/evaluation_loop.py:

Line 148:13: W503 line break before binary operator
Line 152:13: W503 line break before binary operator

Comment last updated at 2021-01-08 20:02:13 UTC

[awaelchli]

@rohitgr7 regarding accelerators, please have a look at #4510 #5385. we are working on some large scale refactoring.

[rohitgr7]

@awaelchli I see. Will remove other changes in the accelerators except setup_training -> setup_trainer. Will that be ok?

[tchaton]

Great work ! Could you check self.logger_connector.set_stage. It is duplicating the information.
Would be better to have a higher level State for this.
self.logger_connector.set_stage was introduced as validation can be performed during training and need to change store.

And it would be good to have trainer.training = True argument and not rely on the model for it.

[rohitgr7]

add internal phase for the trainer as enum for Train/Valid/Test as we seem to have LoggerStages

@Borda need help with this

[Borda]

add internal phase for the trainer as enum for Train/Valid/Test as we seem to have LoggerStages

@Borda need help with this

I made a draft for it here #5419 🐰

[rohitgr7]

@SeanNaren @tchaton mind review??

[ananthsub]

I think this refactor broke this functionality:

trainer = Trainer(resume_from_checkpoint=ckpt)
trainer.test()

I don't see where self.trainer.checkpoint_connector.restore_weights() is called in the evaluation loop or in the trainer. before this happened as part of the TrainLoop's setup_training

In Trainer's run_evaluation I don't see any calls to the checkpoint connector either. Is that intentional?

[rohitgr7]

this is done in setup_training here: https://github.com/PyTorchLightning/pytorch-lightning/pull/5388/files#diff-6b21474ed45079f01dfa45ee3b9d40d23efe693c9c005ce897e25384ef425349

as of now restore is done while calling .fit. Previously it was happening during .test() too and was reloading the resume_from_checkpoint state and model weights that were wrong. But for your use-case, I opened an issue for reloading the callback states optionally.