Repository: pytorch/ignite

Issue:
Getting iterations in Checkpoint is wrong for `global_step_transform`
There seems to be two bugs in `Checkpoint` related to `global_step_transform` when it's attached to the valid rather than train engine.

First, `global_step_transform` does a lookup based on the event fired. This causes issues when the handler is not attached to an `{EPOCH/ITERATION}_COMPLETED`, eg. when it's attached to `COMPLETED` on the valid engine as the docs suggest. 

Second, `global_step_transform` is intended to give the "true" count (iteration, epoch, whatever it may be). As such, it should not only be used in the filename, but also as the `priority`. Right now, priority is the iteration count of the engine it's attached to, which again does not work for valid engine.

A third point, which isn't really a bug but more usability: `Checkpoint` silently drops checkpoints if it has checkpointed the same filename before. I think such occurrences are likely user error (or in my case, framework error, since my iteration count of valid engine is always the same at `COMPLETED`). Perhaps a warning log is warranted. Alternatively, if the checkpoint is truly the same, writing it again is idempotent, so perhaps this check should be removed entirely.

Discussion:
amatsukawa: @vfdev-5 there are some CI failures, but they don't look related to this change. Can you confirm?
vfdev-5: @amatsukawa yes our ci is currently a bit flaky. Yes, those failures are not related to the PR.

Patch:
diff --git a/ignite/handlers/checkpoint.py b/ignite/handlers/checkpoint.py
index a2faf3b9..dcb2a50f 100644
--- a/ignite/handlers/checkpoint.py
+++ b/ignite/handlers/checkpoint.py
@@ -111,6 +111,9 @@ class Checkpoint:
         setup by attached engine's current iteration. The filename will be
         `{filename_prefix}_{name}_{engine.state.iteration}.{ext}`.
 
+        If only ``global_step_transform`` is defined, then suffix is setup using its return value.
+        The filename will be `{filename_prefix}_{name}_{global_step}.{ext}`.
+
         If defined a ``score_function``, but without ``score_name``, then suffix is defined by provided score.
         The filename will be `{filename_prefix}_{name}_{global_step}_{score}.pt`.
 
@@ -260,6 +263,7 @@ class Checkpoint:
     def __call__(self, engine: Engine) -> None:
 
         suffix = ""
+        global_step = None
         if self.global_step_transform is not None:
             global_step = self.global_step_transform(engine, engine.last_event_name)
             suffix = "{}".format(global_step)
@@ -269,7 +273,10 @@ class Checkpoint:
             if not isinstance(priority, numbers.Number):
                 raise ValueError("Output of score_function should be a number")
         else:
-            priority = engine.state.get_event_attrib_value(Events.ITERATION_COMPLETED)
+            if global_step is not None:
+                priority = global_step
+            else:
+                priority = engine.state.get_event_attrib_value(Events.ITERATION_COMPLETED)
 
         if self._check_lt_n_saved() or self._saved[0].priority < priority:
 


Test patch:
diff --git a/tests/ignite/handlers/test_checkpoint.py b/tests/ignite/handlers/test_checkpoint.py
index 70626f74..f69c82f5 100644
--- a/tests/ignite/handlers/test_checkpoint.py
+++ b/tests/ignite/handlers/test_checkpoint.py
@@ -141,7 +141,7 @@ def test_checkpoint_with_global_step_transform():
         )
 
         trainer = Engine(lambda e, b: None)
-        trainer.state = State(epoch=1, iteration=1)
+        trainer.state = State(epoch=2, iteration=1)
 
         checkpointer(trainer)
         assert save_handler.call_count == 1
@@ -149,17 +149,17 @@ def test_checkpoint_with_global_step_transform():
         if len(filename_prefix) > 0:
             filename_prefix += "_"
 
-        metadata = {"basename": "{}{}".format(filename_prefix, name), "score_name": None, "priority": 1}
-        save_handler.assert_called_with(obj, "{}{}_1.pt".format(filename_prefix, name), metadata)
+        metadata = {"basename": "{}{}".format(filename_prefix, name), "score_name": None, "priority": 2}
+        save_handler.assert_called_with(obj, "{}{}_2.pt".format(filename_prefix, name), metadata)
 
         trainer.state.epoch = 12
         trainer.state.iteration = 1234
         checkpointer(trainer)
         assert save_handler.call_count == 2
-        metadata["priority"] = 1234
+        metadata["priority"] = 12
         save_handler.assert_called_with(obj, "{}{}_12.pt".format(filename_prefix, name), metadata)
         assert save_handler.remove.call_count == 1
-        save_handler.remove.assert_called_with("{}{}_1.pt".format(filename_prefix, name))
+        save_handler.remove.assert_called_with("{}{}_2.pt".format(filename_prefix, name))
         assert checkpointer.last_checkpoint == "{}{}_12.pt".format(filename_prefix, name)
 
     for prefix in ["", "dummytask"]: