Repository: Lightning-AI/pytorch-lightning
Pull request: Add logger flag to save_hyperparameters
## 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 #6338 

## 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:
Support save_hyperparameters() to checkpoints without sending to logger
## 🚀 Feature
`self.save_hyperparameters()` is an awesome simple way to save input arguments with checkpoints such that when loading a checkpoint the module will be constructed with the same parameters. The method also seems to log the hyperparameters to the experiment logger, which is not always desired. It would be great to add a flag to turn this on or off.

### Pitch

Add a `log=True` flag, which is true by default, to `save_hyperparameters()`. This way hyperparameters could be saved but not logged by calling `self.save_hyperparameters(log=False)`.

Hints from issue discussion:
This issue has been automatically marked as stale because it hasn't had any recent activity. This issue will be closed in 7 days if no further activity occurs. Thank you for your contributions, Pytorch Lightning Team!

I still believe this should be fixed. Logging hyperparameters and saving them to a checkpoint serve different purposes, and saving them should not necessitate logging them.

I do not wish to log hyperparams, but I want to save them as part of the checkpoint. If this will not be fixed as I suggested, is there another way to achieve this while still leveraging ligtning's checkpointing logic?
Could you describe why it's not desirable for the logger to save hyperparameters? Which logger do you use? Could you extend the class to override the log_hyperarameters method and disable it? 
Sure, thanks for asking.

I am using the W&B logger. The lightning module hyperparameters constitute a portion of my configuration, but I also have configuration parameters for setup, data, training, as well as other custom parts of my experiment. I use W&B to log my entire configuration at the start of my experiment, which has a nice hierarchical structure that makes it easy to track configurations with the W&B interface. I call log_hyperparameters myself to do this.

I also call save_hyperparameters so that they are saved and loaded with checkpoints. Unfortunately this also logs all input arguments to the module. I do not wish to log these because 1) they are redundant with my logging of the entire configuration, 2) they do not have a hierarchical structure like my logging of the entire structure which makes them harder to interpret, 3) I have many input arguments and not all are user configurable or intended to be logged.

I believe a flag could be set in save_hyperparameters that is then checked here:
https://github.com/PyTorchLightning/pytorch-lightning/blob/1302766f83a5fc861ae77ef6f8798cd0b1d606c7/pytorch_lightning/trainer/trainer.py#L507
For anyone else having this issue, here's a workaround I've been using.

```python
class WandbManualHparamsLogger(pl.loggers.WandbLogger):
    @pl.utilities.rank_zero_only
    def log_hyperparams(self, params, skip=True):
        if not skip:
            super().log_hyperparams(params)
```

All hyperparameter logging internal to Lightning is skipped, and you can invoke `log_hyperparams(..., skip=False)` to log them manually.

It would be nice if this were available by default though.
@timothybrooks does this solution work for you ? Would be nice if any of you guys could send a PR with this.

> For anyone else having this issue, here's a workaround I've been using.
> 
> ```python
> class WandbManualHparamsLogger(pl.loggers.WandbLogger):
>     @pl.utilities.rank_zero_only
>     def log_hyperparams(self, params, skip=True):
>         if not skip:
>             super().log_hyperparams(params)
> ```
> 
> All hyperparameter logging internal to Lightning is skipped, and you can invoke `log_hyperparams(..., skip=False)` to log them manually.
> 
> It would be nice if this were available by default though.


Picking this up, once the `LoggerConnector` Refactor PR is merged.

Commit message: Update CHANGELOG.md

Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

Patch to review:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index a91f7fc3189ff..6568614497eeb 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -116,7 +116,10 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 - Added support `LightningModule.save_hyperparameters` when `LightningModule` is a dataclass ([#7992](https://github.com/PyTorchLightning/pytorch-lightning/pull/7992))
 
 
-- Add support for overriding `optimizer_zero_grad` and `optimizer_step` when using accumulate_grad_batches ([#7980](https://github.com/PyTorchLightning/pytorch-lightning/pull/7980))
+- Added support for overriding `optimizer_zero_grad` and `optimizer_step` when using accumulate_grad_batches ([#7980](https://github.com/PyTorchLightning/pytorch-lightning/pull/7980))
+
+
+- Added `logger` boolean flag to `save_hyperparameters` ([#7960](https://github.com/PyTorchLightning/pytorch-lightning/pull/7960))
 
 
 - Add support for calling scripts using the module syntax (`python -m package.script`) ([#8073](https://github.com/PyTorchLightning/pytorch-lightning/pull/8073))
diff --git a/pytorch_lightning/core/datamodule.py b/pytorch_lightning/core/datamodule.py
index b1f42c6ea4390..a55900dd15b9c 100644
--- a/pytorch_lightning/core/datamodule.py
+++ b/pytorch_lightning/core/datamodule.py
@@ -97,6 +97,8 @@ def __init__(
         self._has_teardown_test = False
         self._has_teardown_predict = False
 
+        self._log_hyperparams = True
+
     @property
     def train_transforms(self):
         """
diff --git a/pytorch_lightning/core/lightning.py b/pytorch_lightning/core/lightning.py
index 735f8ab160c1f..d4fb0a6832a3b 100644
--- a/pytorch_lightning/core/lightning.py
+++ b/pytorch_lightning/core/lightning.py
@@ -108,6 +108,7 @@ def __init__(self, *args: Any, **kwargs: Any) -> None:
         self._automatic_optimization: bool = True
         self._truncated_bptt_steps: int = 0
         self._param_requires_grad_state = dict()
+        self._log_hyperparams = True
         self._metric_attributes: Optional[Dict[int, str]] = None
 
         # deprecated, will be removed in 1.6
diff --git a/pytorch_lightning/trainer/trainer.py b/pytorch_lightning/trainer/trainer.py
index 4c034ac843361..cf6722c62e18d 100644
--- a/pytorch_lightning/trainer/trainer.py
+++ b/pytorch_lightning/trainer/trainer.py
@@ -907,20 +907,27 @@ def _pre_dispatch(self):
 
     def _log_hyperparams(self):
         # log hyper-parameters
+        hparams_initial = None
+
         if self.logger is not None:
             # save exp to get started (this is where the first experiment logs are written)
-            datamodule_hparams = self.datamodule.hparams_initial if self.datamodule is not None else {}
-            lightning_hparams = self.lightning_module.hparams_initial
-            colliding_keys = lightning_hparams.keys() & datamodule_hparams.keys()
-            if colliding_keys:
-                raise MisconfigurationException(
-                    f"Error while merging hparams: the keys {colliding_keys} are present "
-                    "in both the LightningModule's and LightningDataModule's hparams."
-                )
-
-            hparams_initial = {**lightning_hparams, **datamodule_hparams}
-
-            self.logger.log_hyperparams(hparams_initial)
+            if self.lightning_module._log_hyperparams and self.datamodule._log_hyperparams:
+                datamodule_hparams = self.datamodule.hparams_initial if self.datamodule is not None else {}
+                lightning_hparams = self.lightning_module.hparams_initial
+                colliding_keys = lightning_hparams.keys() & datamodule_hparams.keys()
+                if colliding_keys:
+                    raise MisconfigurationException(
+                        f"Error while merging hparams: the keys {colliding_keys} are present "
+                        "in both the LightningModule's and LightningDataModule's hparams."
+                    )
+                hparams_initial = {**lightning_hparams, **datamodule_hparams}
+            elif self.lightning_module._log_hyperparams:
+                hparams_initial = self.lightning_module.hparams_initial
+            elif self.datamodule._log_hyperparams:
+                hparams_initial = self.datamodule.hparams_initial
+
+            if hparams_initial is not None:
+                self.logger.log_hyperparams(hparams_initial)
             self.logger.log_graph(self.lightning_module)
             self.logger.save()
 
diff --git a/pytorch_lightning/utilities/hparams_mixin.py b/pytorch_lightning/utilities/hparams_mixin.py
index 8dd4b23c89398..e5b553ad189b1 100644
--- a/pytorch_lightning/utilities/hparams_mixin.py
+++ b/pytorch_lightning/utilities/hparams_mixin.py
@@ -30,7 +30,8 @@ def save_hyperparameters(
         self,
         *args,
         ignore: Optional[Union[Sequence[str], str]] = None,
-        frame: Optional[types.FrameType] = None
+        frame: Optional[types.FrameType] = None,
+        logger: bool = True
     ) -> None:
         """Save arguments to ``hparams`` attribute.
 
@@ -40,6 +41,7 @@ def save_hyperparameters(
             ignore: an argument name or a list of argument names from
                 class ``__init__`` to be ignored
             frame: a frame object. Default is None
+            logger: Whether to save hyperparameters by logger. Default: True
 
         Example::
             >>> class ManuallyArgsModel(HyperparametersMixin):
@@ -92,6 +94,7 @@ class ``__init__`` to be ignored
             "arg1": 1
             "arg3": 3.14
         """
+        self._log_hyperparams = logger
         # the frame needs to be created in this file.
         if not frame:
             frame = inspect.currentframe().f_back
diff --git a/tests/loggers/test_base.py b/tests/loggers/test_base.py
index 9209083148265..981913f432063 100644
--- a/tests/loggers/test_base.py
+++ b/tests/loggers/test_base.py
@@ -17,6 +17,7 @@
 from unittest.mock import MagicMock
 
 import numpy as np
+import pytest
 
 from pytorch_lightning import Trainer
 from pytorch_lightning.loggers import LightningLoggerBase, LoggerCollection, TensorBoardLogger
@@ -290,3 +291,37 @@ def log_hyperparams(self, params):
     }
     logger.log_hyperparams(Namespace(**np_params))
     assert logger.logged_params == sanitized_params
+
+
+@pytest.mark.parametrize("logger", [True, False])
+def test_log_hyperparams_being_called(tmpdir, logger):
+
+    class TestLogger(DummyLogger):
+
+        def __init__(self):
+            super().__init__()
+            self.log_hyperparams_called = False
+
+        def log_hyperparams(self, *args, **kwargs):
+            self.log_hyperparams_called = True
+
+    class TestModel(BoringModel):
+
+        def __init__(self, param_one, param_two):
+            super().__init__()
+            self.save_hyperparameters(logger=logger)
+
+    test_logger = TestLogger()
+    model = TestModel("pytorch", "lightning")
+
+    trainer = Trainer(
+        default_root_dir=tmpdir,
+        logger=test_logger,
+        max_epochs=1,
+        limit_train_batches=0.1,
+        limit_val_batches=0.1,
+        num_sanity_val_steps=0,
+    )
+    trainer.fit(model)
+
+    assert logger == test_logger.log_hyperparams_called


Code review comments:
diff_hunk: @@ -40,6 +41,7 @@ def save_hyperparameters(
             ignore: an argument name or a list of argument names from
                 class ``__init__`` to be ignored
             frame: a frame object. Default is None
+            logger: Whether to save hyperparameters by logger. Default: True
path: pytorch_lightning/utilities/hparams_mixin.py
text: @user1:
```suggestion
            logger: Whether to send the hyperparameters to the logger. Default: True
```
this was slightly ambiguous here, because some loggers actually save a file.

diff_hunk: @@ -97,6 +97,8 @@ def __init__(
         self._has_teardown_test = False
         self._has_teardown_predict = False
 
+        self._log_hyperparams = True
path: pytorch_lightning/core/datamodule.py
text: @user1:
- should this be part of the hyperparams mixin? 
- why is it protected? are users expected to toggle this on or off? 

@author:
> should this be part of the hyperparams mixin?

Could be added. Would need an additional check for `_log_hyperparams` in Trainer.
```python
if not hasattr(self, "_log_hyperparams"):
```

> why is it protected? are users expected to toggle this on or off?

Nope, should be expected to toggle with the `save_hyperparameters` api.

@user2:
yes I believe too this should go to the mixin class, otherwise the LightningModule has a dependency on the Mixin interface, but this should not be, or we can't call it a mixin.  

we can simply add the initialization of this attribute to the mixin, right?

@author:
Updated!

@user2:
eggsellent!