Repository: Lightning-AI/pytorch-lightning
Pull request: Fix Configuring Learning Rate Schedulers
# Before submitting

- [x] Was this **discussed**/~~approved~~ via a Github issue? (no need for typos and docs improvements)
- [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 to update the docs?   
- [x] Did you write any new necessary tests?  
- [x] If you made a notable change (that affects users), did you update the [CHANGELOG](https://github.com/PyTorchLightning/pytorch-lightning/blob/master/.github/CHANGELOG.md)?

## What does this PR do?
Fixes #1157 Clarifies within the docs how and where to set scheduler parameters.

## 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?
This PR would have saved me an hour, so collectively, I hope it it savs many dev-hours.

At least two others [A](https://github.com/PyTorchLightning/pytorch-lightning/issues/1157), [B](https://github.com/PyTorchLightning/pytorch-lightning/issues/1156) similarly shared confusion about how best and where to set scheduler parameters.

Two places in the docs speak to configuring schedulers:

- [Common Use Cases](https://pytorch-lightning.readthedocs.io/en/0.7.1/optimizers.html#learning-rate-scheduling)
- [Core .configure_optimizers Docs](https://pytorch-lightning.readthedocs.io/en/latest/pytorch_lightning.core.html#pytorch_lightning.core.LightningModule.configure_optimizers)

Neither sufficiently instructs users the appropriate or intended manner of monitoring non-default values or what parameters pytorch searches for to manage schedulers. The functionality exist; it just needs better marketing / documentation =). This PR updates the appropriate .rst files to provide clarity how best to use this functionality.

Docs updated and PEP8 is still in effect.

Problem statement:
Learning Rate Schedulers' default dictionary parameters should be set via the Trainer
## 🚀 Feature
The default Learning Rate Schedulers (LRS) dictionary parameters should be settable from the Trainer constructor. 

### Motivation
The documentation doesn't seem to be clear that the LRS have the following additional parameters available to be set when you configure the optimizers: 
```
    'interval': 'epoch',  # default every epoch
    'frequency': 1,  # default every epoch/batch
    'reduce_on_plateau': False,  # most often not ReduceLROnPlateau scheduler
    'monitor': 'val_loss'
```
### Pitch
Set those defaults in the constructor of the Trainer and the user can set them themselves. 

### Alternatives
Force the return type of the function LightningModule.configure_optimizers() to be List[Optimizer], List[Dict]

Hints from issue discussion:
I disagree mainly due to:
* The trainer constructor already have many arguments, and this would potentially add 3 extra (the `'reduce_on_plateau'` is automatically determined from the LRS) arguments
* The user can already set these in `configure_optimizers` method by passing a LRS as a dict with these keywords.
* If I have two LRS and I want one to step after every epoch and one after every batch i.e. setting `interval="epoch"` for the first and `interval="step"` for the second, I would still need to change at least one of them in `configure_optimizers()` away from the default

I do agree that it is not very clear in the documentation that these are an option to set (this feature was introduced in 0.7.0), this probably needs to be changed

Commit message: Update docs so users know the desired manner of configuring learning rate schedulers.

Patch to review:
diff --git a/docs/source/optimizers.rst b/docs/source/optimizers.rst
index 1d9e77a7de036..9020d5009198f 100644
--- a/docs/source/optimizers.rst
+++ b/docs/source/optimizers.rst
@@ -19,6 +19,20 @@ Every optimizer you use can be paired with any `LearningRateScheduler <https://p
    def configure_optimizers(self):
       return [Adam(...), SGD(...)], [ReduceLROnPlateau(), LambdaLR()]
 
+   # Same as above with additional params passed to the first scheduler
+   def configure_optimizers(self):
+      optimizers = [Adam(...), SGD(...)]
+      schedulers = [
+         {
+            'scheduler': ReduceLROnPlateau(mode='max', patience=7),
+            'monitor': 'val_recall', # Default: val_loss
+            'interval': 'epoch',
+            'frequency': 1
+         },
+         LambdaLR()
+      ]
+      return optimizers, schedulers
+
 
 Use multiple optimizers (like GANs)
 -------------------------------------
diff --git a/pytorch_lightning/core/lightning.py b/pytorch_lightning/core/lightning.py
index d5b408423dce1..ed0bf053905d9 100644
--- a/pytorch_lightning/core/lightning.py
+++ b/pytorch_lightning/core/lightning.py
@@ -942,28 +942,36 @@ def configure_optimizers(self):
                     dis_sched = CosineAnnealing(discriminator_opt, T_max=10) # called every epoch
                     return [gen_opt, dis_opt], [gen_sched, dis_sched]
 
-        Some things to know
+        Some things to note:
 
-            - Lightning calls ``.backward()`` and ``.step()`` on each optimizer
+            Lightning calls ``.backward()`` and ``.step()`` on each optimizer
             and learning rate scheduler as needed.
 
-            - If you use 16-bit precision (``precision=16``), Lightning will automatically
+            If you use 16-bit precision (``precision=16``), Lightning will automatically
             handle the optimizers for you.
 
-            - If you use multiple optimizers, training_step will have an additional
+            If you use multiple optimizers, training_step will have an additional
             ``optimizer_idx`` parameter.
 
-            - If you use LBFGS lightning handles the closure function automatically for you
+            If you use LBFGS lightning handles the closure function automatically for you.
 
-            - If you use multiple optimizers, gradients will be calculated only
+            If you use multiple optimizers, gradients will be calculated only
             for the parameters of current optimizer at each training step.
 
-            - If you need to control how often those optimizers step or override the
+            If you need to control how often those optimizers step or override the
             default .step() schedule, override the `optimizer_step` hook.
 
-            - If you only want to call a learning rate scheduler every `x` step or epoch,
-            you can input this as 'frequency' key: dict(scheduler=lr_scheduler,
-                                                        interval='step' or 'epoch', frequency=x)
+            If you only want to call a learning rate scheduler every `x` step or epoch,
+            or want to monitor a custom metric, you can specify these in a dictionary:
+
+            .. code-block:: python
+
+                {
+                    'scheduler': lr_scheduler,
+                    'interval': 'step'  # or 'epoch'
+                    'monitor': 'val_f1',
+                    'frequency': x
+                }
 
         """
         return Adam(self.parameters(), lr=1e-3)


Code review comments:
diff_hunk: @@ -942,28 +942,36 @@ def configure_optimizers(self):
                     dis_sched = CosineAnnealing(discriminator_opt, T_max=10) # called every epoch
                     return [gen_opt, dis_opt], [gen_sched, dis_sched]
 
-        Some things to know
+        Some things to note:
 
-            - Lightning calls ``.backward()`` and ``.step()`` on each optimizer
path: pytorch_lightning/core/lightning.py
text: @user1:
why is this listing removed?

@author:
This section used a functionally different formatting in 0.6.0. It rendered properly.

<img width="760" alt="060" src="https://user-images.githubusercontent.com/1803297/76966369-da7a2100-68f3-11ea-80ea-ad62ac4709bd.png">

I noticed it was [changed here](https://github.com/PyTorchLightning/pytorch-lightning/commit/f996c2892f60b428b1c5ba4b397fa361103e5c65#diff-2d943a0d0806233840a33725d8a7ce37), which is its current state today =(.

<img width="616" alt="07x" src="https://user-images.githubusercontent.com/1803297/76966392-e4038900-68f3-11ea-9762-d8b622c5abec.png">

Attempting to understand the intent of the above commit, I believe it was recognized that `.. note::` was being abused and that no other section in the docs looked similar; hence it was removed completely.

I considered the option of bring back `.. note:`, back but under a single consolidated note, which would have rendered properly and is actually my desired outcome, very crisp and clean:

<img width="761" alt="060b" src="https://user-images.githubusercontent.com/1803297/76966407-e960d380-68f3-11ea-82f4-e108d9eba3b8.png">

But my commit adds a code block into this note section, and even without it, the note block is already sufficiently large. It really isn't a note but just part of the body/content of the section. I believe the formatting proposed in my current commit works the best.

---

Separately, in the current master / nightly, the entire trainer docs section is missing / isn't built (!) https://pytorch-lightning.readthedocs.io/en/latest/trainer.html due to `WARNING: autodoc: failed to import module ....` while doing a `make html`. However the cause of that is external to this PR.

@user1:
Well to be casted to the same item the new lines has to have one extra space at its beginning...
```
- first line starts 
 and continue here 
```