Repository: pytorch/ignite

Issue:
Support options to compare in CheckpointHander
## 🚀 Feature

<!-- A clear and concise description of the feature proposal -->
<!-- Please outline the motivation for the proposal. Is your feature request related to a problem? e.g., I'm always frustrated when [...]. If this is related to another GitHub issue, please link here too -->
<!-- A clear and concise description of what you want to happen. -->
<!-- A clear and concise description of any alternative solutions or features you've considered, if any. -->
<!-- Add any other context or screenshots about the feature request here. -->
Hi @vfdev-5 ,

In current ignite CheckpointHander, it compares `priority`(may be metrics) > `previous priority` and save models:
https://github.com/pytorch/ignite/blob/master/ignite/handlers/checkpoint.py#L357
But we received several requests to customize this compare logic, is it possible to make options for it?

Thanks.


Discussion:
vfdev-5: Hi @Nic-Ma , thanks a lot for the PR ! 
I see your point about score_function and this option : https://github.com/pytorch/ignite/issues/1586#issuecomment-770737387

Seems like `greater_or_equal` will be only used with `score_function`, right ? As if we use Checkpoint that defines its priority by the iteration, than `greater_or_equal=False` wont be helpful...

Nic-Ma: Hi @vfdev-5 ,

Yes, I didn't change the default behavior, just added an option for the case that `score` equals to the previous `_save[0]` and you want to save the latest checkpoint even with the same score.
If using `iteration` as the priority, it will not have the case that priority equals to the previous one, right?

Thanks.

vfdev-5: @Nic-Ma I think I misunderstood the PR. 

> Yes, I didn't change the default behavior, just added an option for the case that score equals to the previous _save[0] and you want to save the latest checkpoint even with the same score.
If using iteration as the priority, it will not have the case that priority equals to the previous one, right?

I think it was asked previously and we added that such that latest equally scored model is stored... Let me check that

vfdev-5: OK, catched up, what we did was to be able to save the latest model with the same filename.

Nic-Ma: Thanks for your confirmation, and this is a really useful feature, especially in FL, for example:
During FL training, we keep the training run for a long time and add more and more data to train the model day by day, the later model should be better even has the same metrics.

Thanks.

vfdev-5: Thanks for the explanation! Yes, true that this makes sense. I wonder if we should not set that as a default behaviour now ? And for users who'd like to have BC, they could use `greater_or_equal=False`...
Nic-Ma: > Thanks for the explanation! Yes, true that this makes a lot of sense. I wonder if we should not set that as a default behaviour now ? And for users who'd like to have BC, they could use `greater_or_equal=False`...

But I think for regular training on a fixed dataset, maybe the earlier model with the same metrics is better, because the later model may be overfitting? That's why we usually use `early-stopping`?

Thanks.
vfdev-5: yes, it could also prevent saving overfitted model. Let's keep it False by default, I agree.
Nic-Ma: I can't see the CI errors, could you please help me figure it out?

Thanks.

vfdev-5: > I can't see the CI errors, could you please help me figure it out?
> 
> Thanks.

@Nic-Ma it is not an error, I just updated the PR to the latest master and thus cancelled Circle CI tests, but Github interpret this as a failure with a red cross.

Patch:
diff --git a/ignite/handlers/checkpoint.py b/ignite/handlers/checkpoint.py
index 4bcebcab..d7da8527 100644
--- a/ignite/handlers/checkpoint.py
+++ b/ignite/handlers/checkpoint.py
@@ -98,6 +98,8 @@ class Checkpoint(Serializable):
             details.
         include_self (bool): Whether to include the `state_dict` of this object in the checkpoint. If `True`, then
             there must not be another object in ``to_save`` with key ``checkpointer``.
+        greater_or_equal (bool): if `True`, the latest equally scored model is stored. Otherwise, the first model.
+            Default, `False`.
 
     .. _DistributedDataParallel: https://pytorch.org/docs/stable/generated/
         torch.nn.parallel.DistributedDataParallel.html
@@ -261,6 +263,7 @@ class Checkpoint(Serializable):
         global_step_transform: Optional[Callable] = None,
         filename_pattern: Optional[str] = None,
         include_self: bool = False,
+        greater_or_equal: bool = False,
     ) -> None:
 
         if to_save is not None:  # for compatibility with ModelCheckpoint
@@ -301,6 +304,7 @@ class Checkpoint(Serializable):
         self.filename_pattern = filename_pattern
         self._saved = []  # type: List["Checkpoint.Item"]
         self.include_self = include_self
+        self.greater_or_equal = greater_or_equal
 
     def reset(self) -> None:
         """Method to reset saved checkpoint names.
@@ -339,6 +343,12 @@ class Checkpoint(Serializable):
             return True
         return len(self._saved) < self.n_saved + int(or_equal)
 
+    def _compare_fn(self, new: Union[int, float]) -> bool:
+        if self.greater_or_equal:
+            return new >= self._saved[0].priority
+        else:
+            return new > self._saved[0].priority
+
     def __call__(self, engine: Engine) -> None:
 
         global_step = None
@@ -354,7 +364,7 @@ class Checkpoint(Serializable):
                 global_step = engine.state.get_event_attrib_value(Events.ITERATION_COMPLETED)
             priority = global_step
 
-        if self._check_lt_n_saved() or self._saved[0].priority < priority:
+        if self._check_lt_n_saved() or self._compare_fn(priority):
 
             priority_str = f"{priority}" if isinstance(priority, numbers.Integral) else f"{priority:.4f}"
 


Test patch:
diff --git a/tests/ignite/handlers/test_checkpoint.py b/tests/ignite/handlers/test_checkpoint.py
index 8acc7340..e5cd023e 100644
--- a/tests/ignite/handlers/test_checkpoint.py
+++ b/tests/ignite/handlers/test_checkpoint.py
@@ -1526,3 +1526,36 @@ def test_checkpoint_reset_with_engine(dirname):
     expected += [f"{_PREFIX}_{name}_{i}.pt" for i in [1 * 2, 2 * 2]]
     assert sorted(os.listdir(dirname)) == sorted(expected)
     assert "PREFIX_model_4.pt" in handler.last_checkpoint
+
+
+def test_greater_or_equal():
+    scores = iter([1, 2, 2, 2])
+
+    def score_function(_):
+        return next(scores)
+
+    class Saver:
+        def __init__(self):
+            self.counter = 0
+
+        def __call__(self, c, f, m):
+            if self.counter == 0:
+                assert f == "model_1.pt"
+            else:
+                assert f == "model_2.pt"
+            self.counter += 1
+
+    handler = Saver()
+
+    checkpointer = Checkpoint(
+        to_save={"model": DummyModel()},
+        save_handler=handler,
+        score_function=score_function,
+        n_saved=2,
+        greater_or_equal=True,
+    )
+    trainer = Engine(lambda e, b: None)
+
+    for _ in range(4):
+        checkpointer(trainer)
+    assert handler.counter == 4