Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(utils): add hashing a checkpoint utility #2272

Merged
merged 4 commits into from
Oct 18, 2021

Conversation

ydcjeff
Copy link
Contributor

@ydcjeff ydcjeff commented Oct 18, 2021

fix #2269

Description: Add hashing utility for checkpoint

Check list:

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

@github-actions github-actions bot added the module: utils Utils module label Oct 18, 2021
@ydcjeff ydcjeff changed the title feat(utils): add hashing utility for checkpoints feat(utils): add hashing checkpoints utility Oct 18, 2021
Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ydcjeff thanks for the PR ! First of all, mypy is not passing on it. I'm hesitating on adding the cmd line tool. I see the benefit but not sure if ignite should provide cmd line tools for the moment. @sdesrozis @trsvchn thoughts ?

ignite/hash.py Outdated Show resolved Hide resolved
@ydcjeff ydcjeff changed the title feat(utils): add hashing checkpoints utility feat(utils): add hashing a checkpoint utility Oct 18, 2021
@sdesrozis
Copy link
Contributor

@ydcjeff Thanks for this feature !

The hash_checkpoint looks good and can be used as we prefer, that's great. Although I'm not sure about providing a cmdline tool... I would rather have this as a checkpointing option even if it seems not so simple according to loggers, etc.

@ydcjeff
Copy link
Contributor Author

ydcjeff commented Oct 18, 2021

I'm hesitating on adding the cmd line tool.

@vfdev-5 If we are not sure about CLI, we could add it in the future.

I would rather have this as a checkpointing option even if it seems not so simple according to loggers, etc.

@sdesrozis As I stated in the issue, I believe this function is mostly used for the finalized checkpoint, so I am not sure adding another argument in the Checkpoint class which could add read/write overhead if used.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Oct 18, 2021

If we are not sure about for CLI, we could add it in the future.

@ydcjeff sounds good, let's scope this PR for python utils method.

Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @ydcjeff

@vfdev-5 vfdev-5 merged commit 1c25adf into pytorch:master Oct 18, 2021
@ydcjeff ydcjeff deleted the feat/hash-ckpt branch October 18, 2021 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: utils Utils module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A utility for hashing a checkpoint file
3 participants