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

Removed DiskHandler wrapper in Checkpoint calls #72

Merged
merged 2 commits into from
Dec 3, 2021

Conversation

anirudhb11
Copy link
Contributor

The following PR addresses #53 by replacing all instances of save_handler = DiskHandler()

@anirudhb11
Copy link
Contributor Author

@Priyansi the diff is huge even though changes are minimum -- making changes to one of the files (02-convert-pytorch-to-ignite) automatically introduced some new changes -- perhaps indentation introduced via the IDE?

" return DiskSaver(config[\"output_path_\"], require_empty=False)"
" return config[\"output_path_\"]"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @anirudhb11 thanks for the changes! Since now it is returning only a path (and not a handler) should we change the name of the function to something like get_save_path ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought over this, you're right that it returns the path, but if we do this we would have code like: save_handler=get_save_path(config) in some places which might also not be good. Something that would look better would be to modify it to save_path=get_save_path(config) and then we also change the Checkpoint class to take in save_path instead of save_handler. Let me know your thoughts.

@vfdev-5 vfdev-5 requested a review from Priyansi November 22, 2021 13:59
@Priyansi
Copy link
Collaborator

Hey @anirudhb11 thanks for the PR! Could you also removes instances of DiskSaver being imported in different files for example here, DiskSaver could remain unused. Thanks!

@anirudhb11
Copy link
Contributor Author

Hey @anirudhb11 thanks for the PR! Could you also removes instances of DiskSaver being imported in different files for example here, DiskSaver could remain unused. Thanks!

@Priyansi I have addressed this.

Copy link
Collaborator

@Priyansi Priyansi left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM!

@Priyansi Priyansi merged commit b42a5ab into pytorch-ignite:main Dec 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants