Repository: Lightning-AI/pytorch-lightning
Pull request: Fix: passing wrong strings for scheduler interval doesn't throw an error
## What does this PR do?
- Raises `MisconfigurationException` if scheduler's interval parameter is not one of `epoch`, `step`
- Adds a test case to verify this behavior
<!--
IMPORTANT:
 We separated bug-fix PRs and feature PRs and they shall land in master and release/1.X-dev accordingly.
 By default all PR are targeted to master which is correct for bug-fixes, but need to be change for features.
 If you miss it we can still fix it for you, just ping us... :]

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.
-->

Fixes #5548 
## 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?
- [x] 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?
- [ ] 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)

<!-- For CHANGELOG separate each item in the unreleased section by a blank line to reduce collisions -->

## 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](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)
 - [ ] 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
 - [x] **Check that target branch and milestone match!**


## Did you have fun?
Make sure you had fun coding 🙃


Problem statement:
Passing wrong strings for scheduler interval doesn't throw an error
I noticed that when you pass an interval as scheduler parameter that is not 'step' or 'epoch' (for example 'batch') there's no error, it just trains without ever calling the scheduler. Had to look into the source code because I didn't find much info about it in the documentation

Hints from issue discussion:
@finnBsch that is a good observation, mind sending a PR with adding assert or check/raise with exception?
I am not sure where the argument gets assigned.. I just know that there is no check for it in `update_learning_rates`, however I am sure that it gets assigned somewhere earlier but I cannot find it.
@finnBsch here is the code you need to update: https://github.com/PyTorchLightning/pytorch-lightning/blob/8cb2451303fac5d57c79115697e6426f307e8c2c/pytorch_lightning/trainer/optimizers.py#L97

Commit message: Add test for unknown 'interval' value in scheduler

Patch to review:
diff --git a/pytorch_lightning/trainer/optimizers.py b/pytorch_lightning/trainer/optimizers.py
index 6772dcc645e3b..c94b51c2bfd95 100644
--- a/pytorch_lightning/trainer/optimizers.py
+++ b/pytorch_lightning/trainer/optimizers.py
@@ -117,6 +117,13 @@ def configure_schedulers(self, schedulers: list, monitor: Optional[str] = None):
                     raise MisconfigurationException(
                         'The lr scheduler dict must have the key "scheduler" with its item being an lr scheduler'
                     )
+                allowed_interval_keys = ['step', 'epoch']
+                if 'interval' in scheduler and scheduler['interval'] not in allowed_interval_keys:
+                    raise MisconfigurationException(
+                        f'The "interval" key in lr scheduler dict must be one of {allowed_interval_keys},'
+                        f' but is "{scheduler["interval"]}"'
+                    )
+
                 scheduler['reduce_on_plateau'] = isinstance(
                     scheduler['scheduler'], optim.lr_scheduler.ReduceLROnPlateau
                 )
diff --git a/tests/trainer/optimization/test_optimizers.py b/tests/trainer/optimization/test_optimizers.py
index c9a9250995dd0..4844e79383847 100644
--- a/tests/trainer/optimization/test_optimizers.py
+++ b/tests/trainer/optimization/test_optimizers.py
@@ -459,6 +459,24 @@ def test_unknown_configure_optimizers_raises(tmpdir):
         trainer.fit(model)
 
 
+def test_lr_scheduler_with_unknown_interval_raises(tmpdir):
+    """
+    Test exception when lr_scheduler dict has unknown interval param value
+    """
+    model = EvalModelTemplate()
+    optimizer = torch.optim.Adam(model.parameters())
+    model.configure_optimizers = lambda: {
+        'optimizer': optimizer,
+        'lr_scheduler': {
+            'scheduler': torch.optim.lr_scheduler.StepLR(optimizer, 1),
+            'interval': "incorrect_unknown_value"
+        },
+    }
+    trainer = Trainer(default_root_dir=tmpdir, fast_dev_run=True)
+    with pytest.raises(MisconfigurationException, match=r'The "interval" key in lr scheduler dict must be one of.*'):
+        trainer.fit(model)
+
+
 def test_lr_scheduler_with_extra_keys_warns(tmpdir):
     """
     Test warning when lr_scheduler dict has extra keys


Code review comments:
diff_hunk: @@ -459,6 +459,24 @@ def test_unknown_configure_optimizers_raises(tmpdir):
         trainer.fit(model)
 
 
+def test_lr_scheduler_with_unknown_interval_raises(tmpdir):
+    """
+    Test exception when lr_scheduler dict has unknown interval param value
+    """
+    model = EvalModelTemplate()
path: tests/trainer/optimization/test_optimizers.py
text: @user1:
```suggestion
    model = BoringModel()
```