Repository: Lightning-AI/pytorch-lightning
Pull request: Check `max_time` when setting defaults for min/max epochs
## What does this PR do?

<!--
Please include a summary of the change and which issue is fixed.
Please also include relevant motivation and context.
List any dependencies that are required for this change.

If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

The following links the related issue to the PR (https://docs.github.com/en/free-pro-team@latest/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword)
-->

Fixes #8210
Added test which fails in trunk right now

### Does your PR introduce any breaking changes? If yes, please list them.

<!-- FILL IN or None -->

## Before submitting

- [x] Was this **discussed/approved** via a GitHub issue? (not for typos and docs)
- [x] Did you read the [contributor guideline](https://github.com/PyTorchLightning/pytorch-lightning/blob/master/.github/CONTRIBUTING.md), **Pull Request** section?
- [x] 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)
- [x] Did you write any **new necessary tests**? (not for typos and docs)
- [x] Did you verify new and **existing tests pass** locally with your changes?
- [x] Did you list all the **breaking changes** introduced by this pull request?
- [x] Did you **update the [CHANGELOG](https://github.com/PyTorchLightning/pytorch-lightning/blob/master/CHANGELOG.md)**? (not for typos, docs, test updates, or internal minor changes/refactorings)

<!-- In the CHANGELOG, separate each item in the unreleased section by a blank line to reduce collisions -->

## PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read [Review guidelines](https://github.com/PyTorchLightning/pytorch-lightning/wiki/Review-guidelines). In short, see the following bullet-list:

- [x] Is this pull request ready for review? (if not, please submit in draft mode)
- [x] Check that all items from **Before submitting** are resolved
- [x] Make sure the title is self-explanatory and the description concisely explains the PR
- [x] 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 🙃


Problem statement:
Trainer should overwrite max_epoch when max_time is specified
## 🚀 Feature
Currently, the Trainer terminates at max_epoch (1000) even if the max_time is specified. Even if min_epoch is set to be greater than 1000, the Trainer should either overwrite the max_epoch value, or the documentation should state that min_epoch is only applicable if early_stopping is enabled. It could also throw warning when min_epoch is set while early_stopping is disabled.

### Motivation

To continue training for X amount of time (e.g 7 days) when you don't know the maximum number of epochs and no early_stopping/criteria is specified.

### Pitch

I think overwriting max_epoch with values of min_epoch or max_time should be reasonable when they are larger than max_epoch. I wanted to train a big model for 2 days, but after 1 day if was stopped despite setting the max_time and min_epoch parameters. 
If you are concerned with the duration of training, or you are interested in the asymptotic behavior of your model in infinite time horizon,   it makes sense to stop training when it reaches a max_time regardless of max_epoch.
### Alternatives

The alternative is to set a very large max_epoch to ensure the training won't stop until max_time is reached.

Hints from issue discussion:
@awaelchli - should we pass `max_time` to the fit loop constructor and skip setting the default of `max_epochs` if `max_time is not None`?

https://github.com/PyTorchLightning/pytorch-lightning/blob/4af8eff0a13309b2e030c423db91156b1e869fac/pytorch_lightning/loops/fit_loop.py#L51

This also raises what the default should be if no stopping condition is passed to the trainer (e.g. no max steps / max epochs / max time) - should this be an infinite while loop and require an external signal to kill the job? Is it conceivable someone wants to train forever, or for an absurdly long amount of time where they neither know the number of epochs or time it takes for convergence? Should we allow this only if there's an early stopping callback attached to the trainer?

> Even if min_epoch is set to be greater than 1000, the Trainer should either overwrite the max_epoch value, or the documentation should state that min_epoch is only applicable if early_stopping is enabled. It could also throw warning when min_epoch is set while early_stopping is disabled.

Would this be needed if we addressed the max time issue? Overwriting the arguments to me doesn't feel right. If a user is specifying min_epochs > max_epochs, we should raise a misconfiguration exception.

Commit message: Update CHANGELOG.md

Patch to review:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 35404e85a5816..57e717a91b017 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -203,9 +203,13 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 
 - Fixed bug where data-loading functions where not getting the correct running stage passed ([#8858](https://github.com/PyTorchLightning/pytorch-lightning/pull/8858))
 
+
 - Fixed a bug in the binary search mode of auto batch size scaling where exception was thrown if the first trainer run resulted in OOM ([#8954](https://github.com/PyTorchLightning/pytorch-lightning/pull/8954))
 
 
+- Fixed not setting a default value for `max_epochs` if `max_time` was specified on the `Trainer` constructor ([#9072](https://github.com/PyTorchLightning/pytorch-lightning/pull/9072))
+
+
 ## [1.4.3] - 2021-08-17
 
 - Fixed plateau scheduler stepping on incomplete epoch ([#8861](https://github.com/PyTorchLightning/pytorch-lightning/pull/8861))
diff --git a/pytorch_lightning/trainer/trainer.py b/pytorch_lightning/trainer/trainer.py
index 739e34aa249b5..c80c368aaad4e 100644
--- a/pytorch_lightning/trainer/trainer.py
+++ b/pytorch_lightning/trainer/trainer.py
@@ -378,8 +378,8 @@ def __init__(
         self.tuner = Tuner(self)
 
         fit_loop = FitLoop(
-            min_epochs=(1 if (min_epochs is None and min_steps is None) else min_epochs),
-            max_epochs=(1000 if (max_epochs is None and max_steps is None) else max_epochs),
+            min_epochs=(1 if (min_epochs is None and min_steps is None and max_time is None) else min_epochs),
+            max_epochs=(1000 if (max_epochs is None and max_steps is None and max_time is None) else max_epochs),
         )
         training_epoch_loop = TrainingEpochLoop(min_steps, max_steps)
         training_batch_loop = TrainingBatchLoop()
diff --git a/tests/callbacks/test_timer.py b/tests/callbacks/test_timer.py
index 44d3c305bb1ac..e94df0cbeb3e0 100644
--- a/tests/callbacks/test_timer.py
+++ b/tests/callbacks/test_timer.py
@@ -42,6 +42,10 @@ def on_fit_start(self):
         trainer.fit(TestModel())
     assert "callbacks list already contains a Timer" in caplog.text
 
+    trainer = Trainer(max_time=dict(seconds=1337))
+    assert trainer.max_epochs is None
+    assert trainer.max_steps is None
+
 
 @pytest.mark.parametrize(
     "duration,expected",


Code review comments:
diff_hunk: @@ -42,6 +42,10 @@ def on_fit_start(self):
         trainer.fit(TestModel())
     assert "callbacks list already contains a Timer" in caplog.text
 
+    trainer = Trainer(max_time=dict(seconds=1337))
path: tests/callbacks/test_timer.py
text: @user1:
noob question. Can the model actually train within max_epochs, max_steps not defined ? Won't it be better to set them to inf ?

@author:
for infinite training, https://github.com/PyTorchLightning/pytorch-lightning/issues/8818 will cover this