AttributeError in `Checkpoint::reload_objects` when `save_handler` is a callable object

[sadra-barikbin]

🐛 Bug description

When using Checkpoint with a callable object as save_handler, calling reload_objects causes AttributeError. To reproduce:

def test_checkpoint_simple_recovery():
    from pickle import dump
    class SaveHandler:
        def __call__(self, checkpoint, filename):
            with open(filename, 'wb') as f:
                dump(checkpoint, f)
    to_save = {'model': DummyModel()}
    h = Checkpoint(to_save, SaveHandler())
    engine = Engine(lambda e, b: None)
    engine.state = State(epoch=0, iteration=1)
    h(engine)
    to_load = {"model": DummyModel()}
    h.reload_objects(to_load, global_step=1)

result:

>       path = self.save_handler.dirname / checkpoint_fp
E       AttributeError: 'SaveHandler' object has no attribute 'dirname'

It attempts to access dirname of the handler while dirname is only the attribute of the DiskSaver not even BaseSaveHandler.

Also in the Checkpoint docstring, it states String, method or callable class (by callable class it means callable object?) for save_handler type but in type checks we allow a callable function:

if not (
    isinstance(save_handler, str)
    or isinstance(save_handler, Path)
    or callable(save_handler)
    or isinstance(save_handler, BaseSaveHandler)
):
    raise TypeError(
        "Argument `save_handler` should be a string or Path object or callable or inherit from BaseSaveHandler"
    )

Checklist

• ignite/handlers/checkpoint.py

• In the reload_objects method of the Checkpoint class, before the line path = self.save_handler.dirname / checkpoint_fp, add a condition to check if self.save_handler is an instance of DiskSaver.
• If self.save_handler is an instance of DiskSaver, keep the existing logic to get the path.
• If self.save_handler is not an instance of DiskSaver, handle the checkpoint reloading for the callable object. This might involve calling the save_handler with the appropriate arguments to reload the checkpoint.

[github-actions]

Hey 👋, I've just created a thread for this issue on PyTorch-Ignite Discord where you can quickly talk to the community on the topic.

^{🤖 This comment was automatically posted by Discuss on Discord}

[vfdev-5]

Thanks for reporting Sadra!

Also in the Checkpoint docstring, it states String, method or callable class (by callable class it means callable object?) for save_handler type but in type checks we allow a callable function

yes, method or callable class should be replaced by a function or callable object. In the type check we allow strings, path object, any callable objects (functions or instances of classes with __call__ method defined) and an instance of BaseSaveHandler. Sorry, I'm not sure to understand if something wrong with the type check or what is the point you would like to raise here?

[sadra-barikbin]

You answered it. I meant in the if check we have callable(save_handler) which allows save_handler to be a function, but in the docstring, it wants save_handler to be String, method or callable class.

[vfdev-5]

Would you like to send a quick PR with docstring fix ?

[vfdev-5]

@sadra-barikbin can we close this issue or there is something still should be addressed ? =>
Still not fixed the bug with AttributeError: 'SaveHandler' object has no attribute 'dirname'

[sadra-barikbin]

No, we should fix it. When user calls reload_objects when save_handler is a callable object, he/she gets an unexpected error.

[vfdev-5]

@sadra-barikbin any ideas / thoughts how to fix it ?

[sadra-barikbin]

How is to add dirname:Optional[Union[str, Path]]=None arg to `reload_objects and change

ignite/ignite/handlers/checkpoint.py

Line 700 in 02aab6f

path = self.save_handler.dirname / checkpoint_fp

to

if dirname is not None:
    path = Path(dirname) / checkpoint_fp
elif hasattr(self.save_handler, "dirname"):
    path = self.save_handler.dirname / checkpoint_fp
else:
    path = checkpoint_fp

?

[vfdev-5]

Or maybe we can just raise an error if save_handler is not a DiskSaver ?

[sadra-barikbin]

That works as well.