Refactor RunningStage usage in advance of implementing Trainer.validate()

[EliaCereda]

What does this PR do?

author: @carmocca. original author: @EliaCereda

Refactor RunningStage usage in advance of implementing Trainer.validate(...).

• Define trainer.evaluating to check if we are on RunningStage.VALIDATING or RunningStage.TESTING
• Define RunningStage.SANITY_CHECKING
• Define TrainerState.{FITTING,VALIDATING,TESTING,PREDICTING,TUNING}
• Deprecate trainer.running_sanity_check in favor of trainer.sanity_checking
• Update the other components to use trainer.evaluating instead of trainer.testing
• Disable the EarlyStopping and ModelCheckpoint callbacks when not TrainerState.FITTING. This has no effect when evaluating on the test set, since they were already disabled, but it will be necessary for the validation set.
• Rename a few other attributes of Trainer to clarify that they will be used by both test(…) and validate(…)

Fixes #5593

Feature request issue #4634
Split from PR #4707, see for full discussion
Second PR #4948, for context

Backwards compatibility

Unfortunately, there is already trainer.evaluating to indicate if validation is being run. That property has been changed to trainer.validating. I don't see any way we can avoid this compatibility break but it should be mostly fine as we are the ones who rely on these attributes

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

• 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

[codecov]

Codecov Report

Merging #4945 (c46ae62) into master (46540ee) will increase coverage by 0%.
The diff coverage is 91%.

@@          Coverage Diff           @@
##           master   #4945   +/-   ##
======================================
  Coverage      91%     92%           
======================================
  Files         161     161           
  Lines       11457   11479   +22     
======================================
+ Hits        10481   10512   +31     
+ Misses        976     967    -9     

[rohitgr7]

In trainer/evaluation_loop.py and accelerators/tpu_accelerator.py it still uses self.testing at some places. Can you check??

[EliaCereda]

You're absolutely correct about accelerators/tpu_accelerator.py, it is still using Trainer.testing. In addition, I found other usages in plugins/sharded_plugin.py, accelerators/ddp_spawn_accelerator.py, tests/model_train_steps.py and possibly in connectors/logger_connector.py. I must have missed these occurrences, tomorrow I'll review them.

On the other hand, it seems to me that evaluation_loop.py only accesses EvaluationLoop.testing which is a different attribute: its purpose is to determine whether EvaluationLoop is currently in the validation or test loop and Trainer already sets it correctly from this line. It will be False during the validation phase of fit() and in validate(), but True during test().

Let me know if I missed any usage of Trainer.testing directly in EvaluationLoop.

[rohitgr7]

EvaluationLoop.testing doesn't seems to be doing anything special other than checking whether to load and call test methods or eval methods. I think we can easily do this with self.trainer.evaluating.

if self.testing:
    do something
else:
    do something else

can be:

if self.trainer.evaluating == 'test':
    do something
else:
    do something else

but need to make sure it's correct and not breaking anything else.

[EliaCereda]

Yes, I also think that would work. As you say, the harder part is ensuring everything works correctly, which is not trivial when the logic is intertwined over a number of files.

This is the main reason I stopped the refactoring at the attributes on Trainer.

In addition, EvaluationLoop specifically doesn’t need to distinguish fit, validate and test but only whether the current evaluation is over validation or test. This is what replacing testing with evaluating essentially makes possibile, but it would not be of much benefit here.

And Trainer is a user-facing class, where it was important to both provide a coherent API and not break existing users which were potentially already using testing or tested_checkpoint_path. With EvaluationLoop being mainly an internal object, hidden from users, maybe it makes sense to leave this as a future refactor?

[Borda]

hi @EliaCereda how is going here? just waiting for review?
cc: @tchaton @awaelchli

[rohitgr7]

@EliaCereda I have cleaned #4945 (comment) on master. Will be available once 1.1.5 get's synced to 1.2-dev: #5583

[tchaton]

Awesome PR. Small comments !

[awaelchli]

nice