Repository: Lightning-AI/pytorch-lightning
Pull request: validate manual optimization and supported features before running training
## What does this PR do?

Provides a better error message for unsupported features with manual optimization. 

- gradient clipping (cryptic error message)
- gradient accumulation (would silently not work)

Fixes #7698 

## 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?
- [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)

<!-- 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)
 - [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:
AttributeError: 'NoneType' object has no attribute 'param_groups'
```python
class myModel(pl.LightningModule):
    def __init__(self):
        super(myModel, self).__init__()
        self.automatic_optimization = False
        self.model=nn.Linear(1,1)
    def training_step(self,batch,batch_idx):
        opt = self.optimizers()
        opt.zero_grad()
        x,y=batch
        x=x.float()
        y=y.float()
        x=self.model(x)
        loss_fn=nn.MSELoss()
        loss = loss_fn(x,y)
        self.manual_backward(loss)
        return loss
    def configure_optimizers(self):
        return torch.optim.Adam(self.model.parameters())
    def train_dataloader(self):
        num_sample=3
        x=torch.randn(num_sample,1)
        y=torch.randn(num_sample,1)
        return DataLoader(TensorDataset(x,y),batch_size=1)
mymodel = myModel()
trainer = pl.Trainer(gradient_clip_val=10)
trainer.fit(mymodel)
```
the error is following:
<details>
<summary>error</summary>

```python
AttributeError                            Traceback (most recent call last)

<ipython-input-11-7e84802c7444> in <module>
     24 mymodel = myModel()
     25 trainer = pl.Trainer(gradient_clip_val=10)
---> 26 trainer.fit(mymodel)

D:\Python37\lib\site-packages\pytorch_lightning\trainer\trainer.py in fit(self, model, train_dataloader, val_dataloaders, datamodule)
    456         )
    457 
--> 458         self._run(model)
    459 
    460         assert self.state.stopped

D:\Python37\lib\site-packages\pytorch_lightning\trainer\trainer.py in _run(self, model)
    754 
    755         # dispatch `start_training` or `start_evaluating` or `start_predicting`
--> 756         self.dispatch()
    757 
    758         # plugin will finalized fitting (e.g. ddp_spawn will load trained model)

D:\Python37\lib\site-packages\pytorch_lightning\trainer\trainer.py in dispatch(self)
    795             self.accelerator.start_predicting(self)
    796         else:
--> 797             self.accelerator.start_training(self)
    798 
    799     def run_stage(self):

D:\Python37\lib\site-packages\pytorch_lightning\accelerators\accelerator.py in start_training(self, trainer)
     94 
     95     def start_training(self, trainer: 'pl.Trainer') -> None:
---> 96         self.training_type_plugin.start_training(trainer)
     97 
     98     def start_evaluating(self, trainer: 'pl.Trainer') -> None:

D:\Python37\lib\site-packages\pytorch_lightning\plugins\training_type\training_type_plugin.py in start_training(self, trainer)
    142     def start_training(self, trainer: 'pl.Trainer') -> None:
    143         # double dispatch to initiate the training loop
--> 144         self._results = trainer.run_stage()
    145 
    146     def start_evaluating(self, trainer: 'pl.Trainer') -> None:

D:\Python37\lib\site-packages\pytorch_lightning\trainer\trainer.py in run_stage(self)
    805         if self.predicting:
    806             return self.run_predict()
--> 807         return self.run_train()
    808 
    809     def _pre_training_routine(self):

D:\Python37\lib\site-packages\pytorch_lightning\trainer\trainer.py in run_train(self)
    867                 with self.profiler.profile("run_training_epoch"):
    868                     # run train epoch
--> 869                     self.train_loop.run_training_epoch()
    870 
    871                 if self.max_steps and self.max_steps <= self.global_step:

D:\Python37\lib\site-packages\pytorch_lightning\trainer\training_loop.py in run_training_epoch(self)
    487             # ------------------------------------
    488             with self.trainer.profiler.profile("run_training_batch"):
--> 489                 batch_output = self.run_training_batch(batch, batch_idx, dataloader_idx)
    490 
    491             # when returning -1 from train_step, we end epoch early

D:\Python37\lib\site-packages\pytorch_lightning\trainer\training_loop.py in run_training_batch(self, batch, batch_idx, dataloader_idx)
    731                     else:
    732                         self._curr_step_result = self.training_step(
--> 733                             split_batch, batch_idx, opt_idx, self.trainer.hiddens
    734                         )
    735 

D:\Python37\lib\site-packages\pytorch_lightning\trainer\training_loop.py in training_step(self, split_batch, batch_idx, opt_idx, hiddens)
    278             model_ref._results = Result()
    279             with self.trainer.profiler.profile("training_step"):
--> 280                 training_step_output = self.trainer.accelerator.training_step(args)
    281                 self.trainer.accelerator.post_training_step()
    282 

D:\Python37\lib\site-packages\pytorch_lightning\accelerators\accelerator.py in training_step(self, args)
    202 
    203         with self.precision_plugin.train_step_context(), self.training_type_plugin.train_step_context():
--> 204             return self.training_type_plugin.training_step(*args)
    205 
    206     def post_training_step(self) -> None:

D:\Python37\lib\site-packages\pytorch_lightning\plugins\training_type\training_type_plugin.py in training_step(self, *args, **kwargs)
    153 
    154     def training_step(self, *args, **kwargs):
--> 155         return self.lightning_module.training_step(*args, **kwargs)
    156 
    157     def post_training_step(self):

<ipython-input-11-7e84802c7444> in training_step(self, batch, batch_idx)
     13         loss_fn=nn.MSELoss()
     14         loss = loss_fn(x,y)
---> 15         self.manual_backward(loss)
     16         return loss
     17     def configure_optimizers(self):

D:\Python37\lib\site-packages\pytorch_lightning\core\lightning.py in manual_backward(self, loss, optimizer, *args, **kwargs)
   1250         # backward
   1251         self._running_manual_backward = True
-> 1252         self.trainer.train_loop.backward(loss, optimizer=None, opt_idx=None, *args, **kwargs)
   1253         self._running_manual_backward = False
   1254 

D:\Python37\lib\site-packages\pytorch_lightning\trainer\training_loop.py in backward(self, result, optimizer, opt_idx, *args, **kwargs)
    866         if not self.should_accumulate():
    867             # track gradients
--> 868             self.track_and_norm_grad(optimizer=optimizer)
    869 
    870     def update_train_loop_lr_schedulers(self, monitor_metrics=None):

D:\Python37\lib\site-packages\pytorch_lightning\trainer\training_loop.py in track_and_norm_grad(self, optimizer)
    445         # clip gradients
    446         self.trainer.accelerator.clip_gradients(
--> 447             optimizer, self.trainer.gradient_clip_val, gradient_clip_algorithm=self.trainer.gradient_clip_algorithm
    448         )
    449         self._cur_grad_norm_dict = grad_norm_dic

D:\Python37\lib\site-packages\pytorch_lightning\accelerators\accelerator.py in clip_gradients(self, optimizer, clip_val, gradient_clip_algorithm)
    352             clip_val,
    353             gradient_clip_algorithm=gradient_clip_algorithm,
--> 354             model=self.model,
    355         )
    356 

D:\Python37\lib\site-packages\pytorch_lightning\plugins\precision\precision_plugin.py in clip_gradients(self, optimizer, clip_val, gradient_clip_algorithm, model)
    119         elif gradient_clip_algorithm == GradClipAlgorithmType.NORM:
    120             # TODO: there should be a mechanism to set `norm_type`
--> 121             self.clip_grad_by_norm(optimizer, clip_val, eps=self.EPSILON)
    122 
    123     def clip_grad_by_value(self, optimizer: Optimizer, clip_val: Union[int, float]) -> None:

D:\Python37\lib\site-packages\pytorch_lightning\plugins\precision\precision_plugin.py in clip_grad_by_norm(self, optimizer, clip_val, norm_type, eps)
    133 
    134         # TODO: replace this with torch.nn.clip_grad_norm_
--> 135         parameters = list(filter(lambda p: p.grad is not None, parameters))
    136         device = parameters[0].device
    137 

D:\Python37\lib\site-packages\pytorch_lightning\plugins\precision\precision_plugin.py in master_params(self, optimizer)
     40         Maybe different in other precision plugins.
     41         """
---> 42         for group in optimizer.param_groups:
     43             for p in group["params"]:
     44                 yield p

AttributeError: 'NoneType' object has no attribute 'param_groups'
```
</details>


if set `gradient_clip_val` and  `self.automatic_optimization = False` then the error will occur.
Is this expected?

Hints from issue discussion:
you are missing an optimizer.step() if I see correctly. Is it intentional?
looks like in manual optimization, grad clipping is not been taken care of. Should it be done my lightning in such a case or user should do it?? @awaelchli 
yeah, gradient clipping has to be done manually too, but it may differ based on the precision. 
I can imagine having an API like this:

```python
self.manual_backward(loss, gradient_clip_val=...)
```

but haven't studied it in detail or if there are any pitfalls. Might be worth exploring.
I recently also encounter this problem. I think there is indeed some logic error in `manual_loss`.


```python
    ....
    self.manual_backward(loss)
```

- `lightning.py: manual_backward`
```python
    def manual_backward(self, loss: Tensor, optimizer: Optional[Optimizer] = None, *args, **kwargs) -> None:
        ...
        # backward
        self._running_manual_backward = True
        self.trainer.train_loop.backward(loss, optimizer=None, opt_idx=None, *args, **kwargs)
        self._running_manual_backward = False
```

- `trainer.train_loop.backward(loss, optimizer=None,...)`

the argument `optimizer` is `None`, but the last line requires a `optimizer`

```python
    def backward(self, result, optimizer, opt_idx, *args, **kwargs):
        self.trainer.dev_debugger.track_event("backward_call")

        should_accumulate = self.should_accumulate()

        # backward can be called manually in the training loop
        if isinstance(result, torch.Tensor):
            self.trainer.accelerator.backward(result, optimizer, opt_idx, should_accumulate, *args, **kwargs)
        else:
            result.closure_loss = self.trainer.accelerator.backward(
                result.closure_loss, optimizer, opt_idx, should_accumulate, *args, **kwargs
            )

        if not self.should_accumulate():
            # track gradients
            self.track_and_norm_grad(optimizer=optimizer)   # raise error here
```


Yes so the conclusion is: we run into this problem in manual optimization when we use gradient clipping, which we cannot support directly through the Trainer flag. 

In #7788 I'm adding a friendly message. But we probably want to convert this issue to a feature request adding something like this https://github.com/PyTorchLightning/pytorch-lightning/issues/7698#issuecomment-848230244

Commit message: update changelog

Patch to review:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5302b2199d46a..e5ff8f8689bb8 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -110,6 +110,9 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 - Changed `teardown()` in `Accelerator` to allow `training_type_plugin` to customize `teardown` logic ([#7579](https://github.com/PyTorchLightning/pytorch-lightning/pull/7579))
 
 
+- `Trainer.fit` now raises an error when using manual optimization with unsupported features such as `gradient_clip_val` or `accumulate_grad_batches` ([#7788](https://github.com/PyTorchLightning/pytorch-lightning/pull/7788))
+
+
 ### Deprecated
 
 
diff --git a/pytorch_lightning/trainer/configuration_validator.py b/pytorch_lightning/trainer/configuration_validator.py
index e73bee761a241..365951d07924b 100644
--- a/pytorch_lightning/trainer/configuration_validator.py
+++ b/pytorch_lightning/trainer/configuration_validator.py
@@ -34,6 +34,7 @@ def verify_loop_configurations(self, model: 'pl.LightningModule') -> None:
         if self.trainer.state.fn in (TrainerFn.FITTING, TrainerFn.TUNING):
             self.__verify_train_loop_configuration(model)
             self.__verify_eval_loop_configuration(model, 'val')
+            self.__verify_manual_optimization_support(model)
         elif self.trainer.state.fn == TrainerFn.VALIDATING:
             self.__verify_eval_loop_configuration(model, 'val')
         elif self.trainer.state.fn == TrainerFn.TESTING:
@@ -112,3 +113,19 @@ def __verify_dp_batch_transfer_support(self, model: 'pl.LightningModule') -> Non
         for hook in batch_transfer_hooks:
             if self.trainer.accelerator_connector.use_dp and is_overridden(hook, model):
                 raise MisconfigurationException(f'Overriding `{hook}` is not supported in DP mode.')
+
+    def __verify_manual_optimization_support(self, model: 'pl.LightningModule') -> None:
+        if model.automatic_optimization:
+            return
+        if self.trainer.gradient_clip_val > 0:
+            raise MisconfigurationException(
+                f"Automatic gradient clipping is not supported for manual optimization."
+                f" Remove the Trainer setting Trainer(gradient_clip_val={self.trainer.gradient_clip_val})"
+                f" or switch to automatic optimization."
+            )
+        if self.trainer.accumulate_grad_batches != 1:
+            raise MisconfigurationException(
+                f"Automatic gradient accumulation is not supported for manual optimization."
+                f" Remove the Trainer setting Trainer(accumulate_grad_batches={self.trainer.accumulate_grad_batches})"
+                f" or switch to automatic optimization."
+            )
diff --git a/tests/core/test_lightning_optimizer.py b/tests/core/test_lightning_optimizer.py
index d79cae75956a2..1f12ef4abe074 100644
--- a/tests/core/test_lightning_optimizer.py
+++ b/tests/core/test_lightning_optimizer.py
@@ -123,7 +123,6 @@ def configure_optimizers(self):
         limit_val_batches=1,
         max_epochs=1,
         weights_summary=None,
-        accumulate_grad_batches=999,  # does not do anything if manual optimization
     )
 
     with patch.multiple(torch.optim.SGD, zero_grad=DEFAULT, step=DEFAULT) as sgd, \
diff --git a/tests/trainer/optimization/test_manual_optimization.py b/tests/trainer/optimization/test_manual_optimization.py
index 7767890706bf2..8589473564f00 100644
--- a/tests/trainer/optimization/test_manual_optimization.py
+++ b/tests/trainer/optimization/test_manual_optimization.py
@@ -424,7 +424,6 @@ def on_train_epoch_end(self, *_, **__):
         limit_val_batches=0,
         precision=16,
         amp_backend='native',
-        accumulate_grad_batches=4,
         gpus=1,
     )
     trainer.fit(model)
@@ -631,7 +630,6 @@ def configure_optimizers(self):
         limit_val_batches=2,
         max_epochs=1,
         log_every_n_steps=1,
-        accumulate_grad_batches=2,
     )
 
     trainer.fit(model)
@@ -682,7 +680,6 @@ def configure_optimizers(self):
         limit_val_batches=2,
         max_epochs=1,
         log_every_n_steps=1,
-        accumulate_grad_batches=2,
     )
 
     trainer.fit(model)
@@ -757,7 +754,6 @@ def configure_optimizers(self):
         limit_val_batches=2,
         max_epochs=1,
         log_every_n_steps=1,
-        accumulate_grad_batches=2,
     )
 
     trainer.fit(model)
@@ -867,7 +863,6 @@ def train_manual_optimization(tmpdir, accelerator, model_cls=TesManualOptimizati
         limit_val_batches=2,
         max_epochs=1,
         log_every_n_steps=1,
-        accumulate_grad_batches=2,
         gpus=2,
         accelerator=accelerator,
         callbacks=[TestManualOptimizationDDPCallack()]
diff --git a/tests/trainer/test_config_validator.py b/tests/trainer/test_config_validator.py
index 9fccd9b36440a..4294320c622ee 100644
--- a/tests/trainer/test_config_validator.py
+++ b/tests/trainer/test_config_validator.py
@@ -147,3 +147,17 @@ def predict_dataloader(self):
 
     with pytest.raises(MisconfigurationException, match="Dataloader not found for `Trainer.predict`"):
         trainer.predict(model)
+
+
+def test_trainer_manual_optimization_config(tmpdir):
+    """ Test error message when requesting Trainer features unsupported with manual optimization """
+    model = BoringModel()
+    model.automatic_optimization = False
+
+    with pytest.raises(MisconfigurationException, match="Automatic gradient clipping is not supported"):
+        trainer = Trainer(gradient_clip_val=1.0)
+        trainer.fit(model)
+
+    with pytest.raises(MisconfigurationException, match="Automatic gradient accumulation is not supported"):
+        trainer = Trainer(accumulate_grad_batches=2)
+        trainer.fit(model)


Code review comments:
diff_hunk: @@ -147,3 +147,17 @@ def predict_dataloader(self):
 
     with pytest.raises(MisconfigurationException, match="Dataloader not found for `Trainer.predict`"):
         trainer.predict(model)
+
+
+def test_trainer_manual_optimization_config(tmpdir):
+    """ Test error message when requesting Trainer features unsupported with manual optimization """
+    model = BoringModel()
+    model.automatic_optimization = False
+
+    with pytest.raises(MisconfigurationException, match="Automatic gradient clipping is not supported"):
+        trainer = Trainer(gradient_clip_val=1.0)
path: tests/trainer/test_config_validator.py
text: @user1:
nit: The trainer creation should be out of the with block

@author:
addressed the flaw

diff_hunk: @@ -112,3 +113,19 @@ def __verify_dp_batch_transfer_support(self, model: 'pl.LightningModule') -> Non
         for hook in batch_transfer_hooks:
             if self.trainer.accelerator_connector.use_dp and is_overridden(hook, model):
                 raise MisconfigurationException(f'Overriding `{hook}` is not supported in DP mode.')
+
+    def __verify_manual_optimization_support(self, model: 'pl.LightningModule') -> None:
+        if model.automatic_optimization:
+            return
+        if self.trainer.gradient_clip_val > 0:
+            raise MisconfigurationException(
+                f"Automatic gradient clipping is not supported for manual optimization."
+                f" Remove the Trainer setting Trainer(gradient_clip_val={self.trainer.gradient_clip_val})"
path: pytorch_lightning/trainer/configuration_validator.py
text: @user1:
nit: redundant wording

```suggestion
                f" Remove `Trainer(gradient_clip_val={self.trainer.gradient_clip_val})`"
```

@author:
do you by any chance know why we add backticks to error and warning messages?

@user1:
Just to differentiate between code and text. Same pattern as for docstrings

diff_hunk: @@ -112,3 +113,19 @@ def __verify_dp_batch_transfer_support(self, model: 'pl.LightningModule') -> Non
         for hook in batch_transfer_hooks:
             if self.trainer.accelerator_connector.use_dp and is_overridden(hook, model):
                 raise MisconfigurationException(f'Overriding `{hook}` is not supported in DP mode.')
+
+    def __verify_manual_optimization_support(self, model: 'pl.LightningModule') -> None:
+        if model.automatic_optimization:
+            return
+        if self.trainer.gradient_clip_val > 0:
+            raise MisconfigurationException(
+                f"Automatic gradient clipping is not supported for manual optimization."
+                f" Remove the Trainer setting Trainer(gradient_clip_val={self.trainer.gradient_clip_val})"
+                f" or switch to automatic optimization."
+            )
+        if self.trainer.accumulate_grad_batches != 1:
+            raise MisconfigurationException(
+                f"Automatic gradient accumulation is not supported for manual optimization."
+                f" Remove the Trainer setting Trainer(accumulate_grad_batches={self.trainer.accumulate_grad_batches})"
path: pytorch_lightning/trainer/configuration_validator.py
text: @user1:
```suggestion
                f" Remove `Trainer(accumulate_grad_batches={self.trainer.accumulate_grad_batches})`"
```