Custom filename pattern for saving checkpoints

[Joel-hanson]

Fixes #1074

This PR adds a feature to set custom filename by updating the class attribute filename_pattern.

Description:

There was an issue #1074 reported some days back where the "=" in the checkpoint name can cause certain issues in some operating systems.
I have added a class attribute filename_pattern with the value {filename_prefix}_{name}_{global_step}_{score_name}={score}.{ext}, This file pattern is used when we save a checkpoint. If we update this attribute to a new pattern the checkpoint name will be updated to the new pattern which the user specifies.

Example:

Checkpoint:

Output:

Checklist:

• New tests are added (if a new feature is added)
• New docstrings: description and/or example code are in RST format
• Documentation is updated (if required)

[vfdev-5]

@Joel-hanson thanks for the PR ! Let me check it and comment out things I do not understand...

[Joel-hanson]

Looking into the test which failed.

[Joel-hanson]

Looking into the test which failed.

Fixed some of the tests which were related to the current implementation

@vfdev-5 @sdesrozis can you please help me fix the other test cases

[vfdev-5]

@Joel-hanson our CI can fail on distributed tests, so no worries for failed tests...
I restarted failed test on Travis

[vfdev-5]

@Joel-hanson thanks for working on it ! I left several comments that may improve current implementation and open a small discussion about improvements of Checkpoint class itself.

[vfdev-5]

@Joel-hanson the problem of using predefined pattern like {filename_prefix}_{name}_{global_step}_{score_name}={score}.{ext} forces additions and suppressions because actual default pattern is more like {filename_prefix}{name}_{global_step}{score_name}{score}.{ext} with special processing of fields.

Just tried to play around all of that, I think class attribute is not a good approach... For example, I would like to setup 2 savers: training checkpoint (to save trainer, model, optimizer etc) and best model. For the first one, I do not want to alter filename pattern and for the second I provide score name and score function but for the filename instead of best_model_9_val_acc=0.77.pt I would like to have best_model_9.pt...

Maybe, we can introduce it as an additional argument to __init__ with default as None.

[Joel-hanson]

Good catch @vfdev-5

@vfdev-5 @sdesrozis
Just to be clear for me I will note down what I have in mind for the new implementation introducing additional argument to __init__.

• I will remove the class attributes and will introduce a new additional argument called filename_pattern
• Will also make user-defined attributes as public

self._fname_prefix -> self.filename_prefix
self._score_function -> self.score_function
self._score_name -> self.score_name
self._n_saved -> self.n_saved
self._ext -> self.ext

• The string formatting is not used when there is no filename_pattern provided
• When a filename_pattern is provided, we will use the string formatting to replace its values.

[vfdev-5]

@Joel-hanson yes I think your notes are correct. Two last points

• The string formatting is not used when there is no filename_pattern provided
• When a filename_pattern is provided, we will use the string formatting to replace its values.

may be not clear how to implement. It could nice to have a single code for user pattern or our pattern. We can setup our default pattern {filename_prefix}{name}_{global_step}{score_name}{score}.{ext} at __init__ if nothing provided. In __call__ we can use something like

    def _add_field(f, sep="_"):
        return "{}{}".format(f, sep) if len(f) > 0 else ""

for "filename_prefix": _add_field(kwargs.get("filename_prefix", "")), but certainly a specific processing of {global_step}{score_name}{score} is needed in order to have a good number of _ etc.

[Joel-hanson]

@vfdev-5 @sdesrozis I have updated the PR with the required changes.

There are 2 things to note

1. I have not set any default filename pattern but works the same way as before, What I have done is to take the pattern only when we have a custom filename pattern assigned as an argument. @vfdev-5 I can change this to what you have suggested in the above comment to have default pattern {filename_prefix}{name}_{global_step}{score_name}{score}.{ext} if the current implementation doesn't meet the expectations.
2. What should we do when a user gives a filename pattern which doesn't have the right keys.
   Example {filename}{scores}.{ext} this pattern has the keys filename and scores which we are not string formatting, this can cause a key error.
   Will we mention this in the docs?

[sdesrozis]

@Joel-hanson keys should be described in docs and the list of handled keys must be fixed.

Thank you again for your work!

[sdesrozis]

I played with the existing rules of checkpoint naming.

First, documentation states the rules (slightly modified)

"""
- `filename_prefix` is the argument passed to the constructor,
- `name` is the key in `to_save` if a single object is to store, otherwise `name` is "checkpoint".
- `suffix` is composed as following `{global_step}_{score_name}={score}`.
   
Above `global_step` defined by the output of `global_step_transform` and `score` defined 
by the output of `score_function`.

By default, none of `score_function`, `score_name`, `global_step_transform` is defined, 
then suffix is setup by attached engine's current iteration. The filename will be
`{filename_prefix}_{name}_{engine.state.iteration}.{ext}`.

If defined a `score_function`, but without `score_name`, then suffix is defined by provided score.
The filename will be `{filename_prefix}_{name}_{score}.{ext}`. If `global_step_transform` is 
provided, then the filename will be `{filename_prefix}_{name}_{global_step}_{score}.{ext}`.

If defined `score_function` and `score_name`, then the filename will
be `{filename_prefix}_{name}_{score_name}={score}.{ext}`. 
If `global_step_transform` is provided, then the filename will be 
`{filename_prefix}_{name}_{global_step}_{score_name}={score}.{ext}`

For example, `score_name="neg_val_loss"` and `score_function` that returns `-loss` 
(as objects with highest scores will be retained), then saved filename will be
`{filename_prefix}_{name}_neg_val_loss=-0.1234.pt`.
"""

I wrote a tiny test which could help

from typing import Callable, Optional

def test_pattern(
    filename_prefix: str = "",
    score_function: Optional[Callable] = None,
    score_name: Optional[str] = None,
    global_step_transform: Callable = None,
    filename_pattern: Optional[str] = None,
):
    # ignite rules are applied
    if filename_pattern is None:

        # filename_prefix will be included in pattern at the end
        
        if score_function is None and score_name is None and global_step_transform is None:
            # policy 1 `{filename_prefix}_{name}_{engine.state.iteration}.{ext}`
            filename_pattern = "{name}_%s.{ext}" % 203  # fake engine.state.iteration
        elif score_function and score_name is None:
            if global_step_transform:
                # policy 2a `{filename_prefix}_{name}_{global_step}_{score}.{ext}`
                filename_pattern = "{name}_{global_step}_{score}.{ext}"
            else:
                # policy 2b `{filename_prefix}_{name}_{global_step}_{score}.{ext}`
                filename_pattern = "{name}_{score}.{ext}"
        elif score_function and score_name:
            if global_step_transform:
                # policy 3a `{filename_prefix}_{name}_{global_step}_{score_name}={score}.{ext}`
                filename_pattern = "{name}_{global_step}_{score_name}={score}.{ext}"
            else:
                # policy 3b `{filename_prefix}_{name}_{global_step}_{score}.{ext}`
                filename_pattern = "{name}_{score_name}={score}.{ext}"
        else:
            raise Exception("weird case score_name is not None but score_function is None")
        
        # prefix is added to pattern if needed
        if filename_prefix:
            filename_pattern = "{filename_prefix}_" + filename_pattern

    # filename_pattern contains user specific pattern or ignite pattern

    # keys should be computed
    # compute score : fake call
    score = score_function() if score_function else None
    # compute global_step : fake call
    global_step = global_step_transform() if global_step_transform else None

    # dict of keys
    kwargs = {
        "filename_prefix": filename_prefix,
        "ext": "pt",
        "name": "checkpoint",
        "score_name": score_name,
        "score": score,
        "global_step": global_step
    }

    # pattern is applied wrt keys
    return filename_pattern.format(**kwargs)


if __name__ == '__main__':
    print(test_pattern())
    print(test_pattern("best"))
    print(test_pattern(score_function=lambda: 0.9999))
    print(test_pattern(score_function=lambda: 0.9999, global_step_transform=lambda: 12))
    print(test_pattern(score_function=lambda: 0.9999, score_name="acc"))
    print(test_pattern(score_function=lambda: 0.9999, score_name="acc", global_step_transform=lambda: 12))
    print(test_pattern("best", score_function=lambda: 0.9999))
    print(test_pattern("best", score_function=lambda: 0.9999, global_step_transform=lambda: 12))
    print(test_pattern("best", score_function=lambda: 0.9999, score_name="acc"))
    print(test_pattern("best", score_function=lambda: 0.9999, score_name="acc", global_step_transform=lambda: 12))

    pattern = "SAVE:{name}-{score_name}--{score}.pth"
    print(test_pattern("best", score_function=lambda: 0.9999, score_name="acc", global_step_transform=lambda: 12, filename_pattern=pattern))

Results are

checkpoint_203.pt
best_checkpoint_203.pt
checkpoint_0.9999.pt
checkpoint_12_0.9999.pt
checkpoint_acc=0.9999.pt
checkpoint_12_acc=0.9999.pt
best_checkpoint_0.9999.pt
best_checkpoint_12_0.9999.pt
best_checkpoint_acc=0.9999.pt
best_checkpoint_12_acc=0.9999.pt
SAVE:checkpoint-acc--0.9999.pth

Hope that helps and no bug inside :)

[Joel-hanson]

@sdesrozis Thank you so much for the docs and the test. I shall add it to this PR.
I have some doubt

1. The docstring which you have shared didn't have the first part which is

This class stores a single file as a dictionary of provided objects to save.
The filename has the following structure: `{filename_prefix}_{name}_{suffix}.{ext}` where

is this intentional if so I will remove this from the docstring.

---

2. I have read the docstring and found that this is the only part which is updated

from

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`.

to

If defined a `score_function`, but without `score_name`, then the suffix is defined by the provided score.
The filename will be `{filename_prefix}_{name}_{score}.{ext}`. If `global_step_transform` is provided,
then the filename will be `{filename_prefix}_{name}_{global_step}_{score}.{ext}`.

correct, this is just to confirm.

---

3. The current code has the checkpoint extension as self.ext = ".pt", so when someone gives a custom filename pattern like custom_{name}_{global_step}_{score}.{ext} he will get it with 2 dots in front of extension.
   Examplecurrent_checkpoint_acc_0.9999..pt
   So I shall change the default extension value to just pt, ie self.ext = "pt" this will solve this issue.

[sdesrozis]

@Joel-hanson

1. I extracted partially and I missed this part. The idea was to have rules in mind. Please, don't remove the missing part from docstring.
2. That is correct.
3. You are right. The dot should be removed. My idea was to have patterns written in the most understandable way. Maybe the current implementation fits perfectly the different cases but I was not sure... Feel free to use (and change) this snippet if it helps otherwise we could use it to check implementation.

[Joel-hanson]

@vfdev-5 @sdesrozis I have pushed the latest changes 🤞 .

• Have updated the function _get_filename_pattern to seetup_filename_pattern which is not public and static
• The arguments which were defined with typing were removed.

    @staticmethod
    def setup_filename_pattern(
        filename_prefix, with_score, with_score_name, with_global_step_transform, filename_pattern=None,
    ):

• Have updated the argument as with_score, with_score_name and with_global_step_transform to have the following values

 with_score=self.score_function is None,
 with_score_name=self.score_name is None,
 with_global_step_transform=global_step is None,

[vfdev-5]

Thanks for the update @Joel-hanson ! I left some comments to improve the code and docs.

[vfdev-5]

We need to provide a docstring for this public method, remove filename_pattern=None from args and update the code accordingly.
We also need annotate input args (typing) and add output type

[Joel-hanson]

    @staticmethod
    def setup_filename_pattern(
        filename_prefix: str = "",
        with_score: bool = False,
        with_score_name: bool = False,
        with_global_step_transform: bool = False,
        filename_pattern: union[str, None] = None,
    ) -> str:

Is this ok @vfdev-5

[sdesrozis]

The typehint for filename_pattern should be

filename_pattern: Optional[str] = None

Optional is a shortcut for Union[..., None]

[vfdev-5]

@sdesrozis we do not need this arg at all. See my message above, please

[Joel-hanson]

Sorry, @vfdev-5 I was a bit confused,
Are you suggesting that we don't require the filename_pattern to be passed but rather checked before calling setup_filename_pattern.
def call

if self.filename_pattern is None:
    filename_pattern = self.setup_filename_pattern(
        filename_prefix=self.filename_prefix,
        with_score=self.score_function is None,
        with_score_name=self.score_name is None,
        with_global_step_transform=global_step is None,
    )
else:
    filename_pattern = self.filename_pattern

def setup_filename_pattern

def setup_filename_pattern(
    filename_prefix: str = "",
    with_score: bool = False,
    with_score_name: bool = False,
    with_global_step_transform: bool = False,
) -> Union[str, None]:

is this what you are suggesting?

[vfdev-5]

Yes, something like that. Note that setup_filename_pattern returns str and not Union.

def setup_filename_pattern(...) -> str

[Joel-hanson]

you are right @vfdev-5 will update this as well.

[vfdev-5]

Suggested change

	raise Exception("weird case score_name is not None but score_function is None")
	raise ValueError("If score_name is provided, score_function can not be None")

[sdesrozis]

@Joel-hanson thank you! I don’t have any comment more than those from @vfdev-5 😊

Thank you again for your work !

[Joel-hanson]

@vfdev-5 I have updated the code accordingly which fixes the following.

1. The docstring for this new filename_pattern argument - Custom filename pattern for saving checkpoints #1127 (comment)
2. The typing issue with the arguments and also the removal of filename_pattern from static function setup_filename_pattern Custom filename pattern for saving checkpoints #1127 (comment)
3. The setup filename exception update Custom filename pattern for saving checkpoints #1127 (comment)
4. The tests for the public function setup_filename_pattern Custom filename pattern for saving checkpoints #1127 (comment)

[vfdev-5]

@Joel-hanson thanks for working on the PR, but there is something weird with docstring and implementation of setup_filename_pattern. Maybe, I'm missing something, please explain it, but the code does not look simple...

[vfdev-5]

@Joel-hanson seems like I didn't read attentively the last time the docstring. Now, the description IMO is a bit cumbersome. Argument with_score:

If True, this indicates that the score function is not provided

it looks very misleading, IMO. More intuitive approach is if with_score, then template will have {score}.
Am I missing something ?

[Joel-hanson]

yes, @vfdev-5 its a bit confusing. I think I had mentioned this earlier #1127 (comment). Sorry for the confusion I can update this asap I should have communicated this properly.

[vfdev-5]

Please, try to make it simple and intuitive as much as possible.

[vfdev-5]

is this contre-intuitive too or I do not understand something ?

"if with score and with score name and with global step transform" -> I would assume that filepattern contains those field.

[Joel-hanson]

@vfdev-5 we will have the pattern {name}_{score}.{ext} if the score function, score name, and global step transform function are not provided.

[vfdev-5]

The code is wrong, IMO:

if with_score and with_score_name and with_global_step_transform:
    filename_pattern = "{name}_{score}.{ext}"

• with_score means filepattern will be with a field for the value from score function
• with_score_name means filepattern will be with score name
• with_global_step_transform means filepattern will be with global step
  so I expect to have filename_pattern = "{name}_{global_step}_{score_name}={score}.{ext}"

[Joel-hanson]

I have updated accordingly @vfdev-5.

[vfdev-5]

@Joel-hanson I modified the code and docs from my side.

[sdesrozis]

Few comments about double quotation in docstring.

Otherwise, it is clear and concise!! Nice work, thank you.

[sdesrozis]

I fixed double quotations. The PR is ready for merge when CI will be ok.

[vfdev-5]

@sdesrozis actually, docs rendering with double quotes is better than with single quotes. I prefer to keep double quotes.

[sdesrozis]

😱 I completely missed that! Very sorry, I revert!

[sdesrozis]

@Joel-hanson thank you for this work !! A lot of iteration but the result is nice 👍🏻

[Joel-hanson]

Thanks a lot, @vfdev-5 and @sdesrozis,

It was my biggest opportunity to contribute to a well-known library like ignite. There have been several iterations, and each iteration has brought me something different to experience. Thank you again for motivating me to get this PR as successful as possible. It would be fantastic if you could find any of the pieces where I should have improved that might have made fewer iterations and made it easy for you guys.