Repository: Lightning-AI/pytorch-lightning
Pull request: Add "monitor" to saved ModelCheckpoints
## What does this PR do?

Adds "monitor" to saved ModelCheckpoints
Removes `CHECKPOINT_STATE_BEST_{SCORE,PATH}`. I think these were forgotten after a checkpoint connector refactor. They are unused now.

I don't think we have to load the monitor in `on_load_checkpoint`.

Fixes #4368 

## 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 your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
- [x] Did you make sure to update the documentation with your changes?
- [x] Did you write any new necessary tests? 
- [x] Did you verify new and existing tests pass locally with your changes?
- [x] If you made a notable change (that affects users), did you update the [CHANGELOG](https://github.com/PyTorchLightning/pytorch-lightning/blob/master/CHANGELOG.md)?

## PR review
 - [x] Is this pull request ready for review? (if not, please submit in draft mode)

Problem statement:
Add best_model_monitor to ModelCheckpoint
### Motivation

If I have a directory with many checkpoints, possibly belonging to runs with different monitors, I don't know which metric was tracked by each checkpoint.

### Pitch

```python
ckpt = torch.load("saved_model.ckpt")
assert hasattr(ckpt, "checkpoint_callback_best_model_monitor"
```

### Alternatives

Guess If I included the monitor in the checkpoint name, I could parse it to know. But seems much more convenient to add the property.

Hints from issue discussion:
another alternative: you can also add that to checkpoint itself in `on_save_checkpoint`.
Do you mean by overriding it in my `LightningModule`?

It is an ugly solution because it does not know about any ModelCheckpoints used. Only the trainer knows about them.

So I would need to instantiate my LightningModule with an added `checkpoint_callback_monitor` parameter in order to do:

```python
def on_save_checkpoint(self, checkpoint):
    checkpoint["checkpoint_callback_best_model_monitor"] = self.checkpoint_callback_monitor
```
@carmocca [here](https://github.com/PyTorchLightning/pytorch-lightning/blob/6abc254ae6830bcf2688f780cd12ef68e57531ce/pytorch_lightning/callbacks/model_checkpoint.py#L188-L192) 
If you guys agree with this issue, I can work on the PR
I think it makes sense to add `monitor` especially since `best_model_score` is already included. Lightning should track what that score actually represents 😃 
> Do you mean by overriding it in my LightningModule?

na.. not the LightningModule but ModelCheckpoint, I know it's ugly haha..
IMO I think it's good to have the monitor as well. But need to check whether it is required to load it back or not in `on_load_checkpoint`.

Commit message: Remove unused variables

Patch to review:
diff --git a/pytorch_lightning/callbacks/model_checkpoint.py b/pytorch_lightning/callbacks/model_checkpoint.py
index 6c6a1741c31c5..f44ce4f57d2aa 100644
--- a/pytorch_lightning/callbacks/model_checkpoint.py
+++ b/pytorch_lightning/callbacks/model_checkpoint.py
@@ -131,8 +131,6 @@ class ModelCheckpoint(Callback):
 
     CHECKPOINT_JOIN_CHAR = "-"
     CHECKPOINT_NAME_LAST = "last"
-    CHECKPOINT_STATE_BEST_SCORE = "checkpoint_callback_best_model_score"
-    CHECKPOINT_STATE_BEST_PATH = "checkpoint_callback_best_model_path"
 
     def __init__(
         self,
@@ -187,6 +185,7 @@ def on_validation_end(self, trainer, pl_module):
 
     def on_save_checkpoint(self, trainer, pl_module) -> Dict[str, Any]:
         return {
+            "best_model_monitor": self.monitor,
             "best_model_score": self.best_model_score,
             "best_model_path": self.best_model_path,
         }
diff --git a/tests/checkpointing/test_model_checkpoint.py b/tests/checkpointing/test_model_checkpoint.py
index 976a91f551e0a..1634b73424dd1 100644
--- a/tests/checkpointing/test_model_checkpoint.py
+++ b/tests/checkpointing/test_model_checkpoint.py
@@ -507,10 +507,7 @@ def test_model_checkpoint_save_last_checkpoint_contents(tmpdir):
     assert all(ckpt_last_epoch[k] == ckpt_last[k] for k in ("epoch", "global_step"))
 
     ch_type = type(model_checkpoint)
-    assert all(list(
-        ckpt_last["callbacks"][ch_type][k] == ckpt_last_epoch["callbacks"][ch_type][k]
-        for k in ("best_model_score", "best_model_path")
-    ))
+    assert ckpt_last["callbacks"][ch_type] == ckpt_last_epoch["callbacks"][ch_type]
 
     # it is easier to load the model objects than to iterate over the raw dict of tensors
     model_last_epoch = EvalModelTemplate.load_from_checkpoint(path_last_epoch)


Code review comments:
diff_hunk: @@ -187,6 +185,7 @@ def on_validation_end(self, trainer, pl_module):
 
     def on_save_checkpoint(self, trainer, pl_module) -> Dict[str, Any]:
         return {
+            "best_model_monitor": self.monitor,
path: pytorch_lightning/callbacks/model_checkpoint.py
text: @user1:
this can just be `monitor` right? not sure what makes it the "best" since it'll be the same value in each checkpoint
```suggestion
            "monitor": self.monitor,
```

@author:
It can, kept the prefix for consistency. The best refers to "the monitor of the best model" not "the best monitor of the model".

I am okay with either way. Does anybody else have an opinion on it?

@user2:
just `monitor` is fine I think or maybe `checkpoint_monitor`.

@user3:
@author are you saying the monitor could change from iteration to iteration? my first choice would also have been just monitor.