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.

Sign up for GitHub

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

Jump to bottom

Add ModelCheckpoint.to_yaml method #3048

Merged
merged 9 commits into from
Sep 27, 2020
Merged

Add ModelCheckpoint.to_yaml method #3048

merged 9 commits into from
Sep 27, 2020

Conversation

mpariente
Copy link
Contributor

What does this PR do

Add simple to_json() method to ModelCheckpoint.
In the current code, it requires three lines, and it could be automated by lightning.
Fixes #3047 (disclaimer: my issue with no answer yet)

Before submitting

  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@pep8speaks
Copy link

pep8speaks commented Aug 19, 2020 •
edited
Loading

Hello @mpariente! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-09-27 10:53:00 UTC

@mergify mergify bot requested a review from a team August 19, 2020 10:20
@mpariente mpariente mentioned this pull request Aug 19, 2020
Closed
@mergify
Copy link
Contributor

mergify bot commented Aug 20, 2020

This pull request is now in conflict... :(

@Borda Borda added this to the 0.9.x milestone Aug 20, 2020
@mergify mergify bot requested a review from a team August 23, 2020 09:59
@Borda Borda added the feature Is an improvement or enhancement label Aug 24, 2020
@justusschock
Copy link
Member

Actually I'd prefer not to use json for this. If we really want this (which still needs to be considered), I'd prefer to go with YAML instead of JSON here, since we use yaml for most other human readable serialization and IMO should not mix file formats to much.

@rohitgr7
Copy link
Contributor

why not both or maybe more? to_some_format(format='YAML' or 'JSON' or ...)?
only if we really want this (which still needs to be considered)

@mpariente
Copy link
Contributor Author

If we really want this (which still needs to be considered)

I can make the required changes, but I'd like to know if you want it or not beforehand.

@Borda
Copy link
Member

Borda commented Aug 25, 2020

personally I would prefer YAML as it is readable and it can be used as an architecture draft, but we can have both...
any other thoughts on the API side? @PyTorchLightning/core-contributors

@Borda Borda requested a review from justusschock August 25, 2020 09:12
@Borda Borda added design Includes a design discussion discussion In a discussion stage labels Aug 25, 2020
@justusschock
Copy link
Member

I would strongly recommend not to do both, since this is really not necessary here. It's just so that you know which checkpoints to use and to consider. Doing both is a typical overkill and we still have to keep the effort to maintain in mind (probably not much here, but it increases with every little bit).

@mpariente
Copy link
Contributor Author

YAML only would be ok. It's just to have a one-liner to dump the saved models history.

I think there is a point to this feature because having this file is useful after training, for stochastic weight averaging or ensemble methods etc.. And if we don't dump this file, we have to open the checkpoint to have it's validation performance, which is not efficient.

@rohitgr7
Copy link
Contributor

yeah, YAML sounds good. Let's do YAML then :).

@edenlightning edenlightning modified the milestones: 0.9.x, 1.1 Sep 23, 2020
Borda
Borda reviewed Sep 25, 2020
View reviewed changes
pytorch_lightning/callbacks/model_checkpoint.py Outdated Show resolved Hide resolved
tests/callbacks/test_model_checkpoint.py Outdated Show resolved Hide resolved
pytorch_lightning/callbacks/model_checkpoint.py Outdated Show resolved Hide resolved
tests/callbacks/test_model_checkpoint.py Outdated Show resolved Hide resolved
@mergify mergify bot requested a review from a team September 25, 2020 11:55
@Borda Borda changed the title Add ModelCheckpoint.to_json method Add ModelCheckpoint.to_yaml method Sep 25, 2020
@Borda Borda requested review from Borda and rohitgr7 September 25, 2020 11:55
Borda
Borda approved these changes Sep 25, 2020
View reviewed changes
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify mergify bot requested a review from a team September 25, 2020 11:56
justusschock
justusschock approved these changes Sep 25, 2020
View reviewed changes
pytorch_lightning/callbacks/model_checkpoint.py Outdated Show resolved Hide resolved
pytorch_lightning/callbacks/model_checkpoint.py Outdated Show resolved Hide resolved
@mergify mergify bot requested a review from a team September 25, 2020 12:50
@mpariente
Copy link
Contributor Author

Sorry, I got absorbed by something else and forgot the PR.
Thanks for pushing this through @Borda !

@Borda
Copy link
Member

Borda commented Sep 25, 2020

@mpariente seems that with the update to YAML the test is not correct, mind have look at it...

@mpariente
Copy link
Contributor Author

This week end, I won't have time.
But I'll have a look on Monday.

@mergify
Copy link
Contributor

mergify bot commented Sep 27, 2020

This pull request is now in conflict... :(

@Borda
Copy link
Member

Borda commented Sep 27, 2020

This week end, I won't have time.
But I'll have a look on Monday.

It seems you did it, thank you :]

@Borda Borda added the ready PRs ready to be merged label Sep 27, 2020
@Borda Borda removed the ready PRs ready to be merged label Sep 27, 2020
@mpariente
Copy link
Contributor Author

Yeah, finally ^^
What happening with the tests, you rebased master? The failures are not related right?

@rohitgr7
Copy link
Contributor

good to go now @Borda

@Borda Borda added the ready PRs ready to be merged label Sep 27, 2020
@Borda Borda merged commit 3d76f60 into Lightning-AI:master Sep 27, 2020
@mpariente mpariente deleted the patch-1 branch September 27, 2020 14:47
@Borda Borda modified the milestones: 1.1, 0.9.x Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rohitgr7 rohitgr7 rohitgr7 approved these changes

@Borda Borda Borda approved these changes

@justusschock justusschock justusschock approved these changes

Assignees
No one assigned
Labels
design Includes a design discussion discussion In a discussion stage feature Is an improvement or enhancement ready PRs ready to be merged
Projects
None yet
Milestone
pl:0.10
Development

Successfully merging this pull request may close these issues.

ModelCheckpoint instance to JSON
6 participants
@mpariente @pep8speaks @justusschock @rohitgr7 @Borda @edenlightning