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

Backward compatibility for checkpoint loading #1132

Merged
merged 3 commits into from
Mar 17, 2020

Conversation

amoudgl
Copy link
Contributor

@amoudgl amoudgl commented Mar 12, 2020

Before submitting

  • Was this discussed/approved via a Github issue? (discussed on slack)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

It adds backward compatibility for checkpoint loading. Checkpoint loading is not backward compatible in 0.7.1. Checkpoints saved with previous pl versions (0.6 in my case) do not have hparams_type in checkpoint dictionary due to which hparams are not converted to namespace (L1399). Thus, pl raises error while loading model class as hparams is passed as dictionary to model class not namespace.

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 Mar 12, 2020

Hello @amoudgl! Thanks for updating this PR.

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

Comment last updated at 2020-03-17 13:56:42 UTC

@Borda Borda added bug Something isn't working feature Is an improvement or enhancement labels Mar 13, 2020
@Borda Borda added this to the 0.7.2 milestone Mar 13, 2020
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.

it would be nice to add a test

  • pls add note to changelog...

pytorch_lightning/core/lightning.py Outdated Show resolved Hide resolved
@Borda Borda added the priority: 0 High priority task label Mar 13, 2020
@Borda
Copy link
Member

Borda commented Mar 14, 2020

@amoudgl pls add note to changelog... then we can go :]

amoudgl added a commit to amoudgl/pytorch-lightning that referenced this pull request Mar 14, 2020
@amoudgl
Copy link
Contributor Author

amoudgl commented Mar 14, 2020

@Borda I have made updates to CHANGELOG, could you please review?

@Borda Borda added the ready PRs ready to be merged label Mar 16, 2020
@Borda
Copy link
Member

Borda commented Mar 16, 2020

this shall be fixed in #1163

@amoudgl
Copy link
Contributor Author

amoudgl commented Mar 16, 2020

I see, everything is all set in this PR then?

@Borda
Copy link
Member

Borda commented Mar 17, 2020

@amoudgl could you pls update from master?

@amoudgl
Copy link
Contributor Author

amoudgl commented Mar 17, 2020

@Borda I have rebased the PR.

@codecov
Copy link

codecov bot commented Mar 17, 2020

Codecov Report

Merging #1132 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1132   +/-   ##
======================================
  Coverage      89%     89%           
======================================
  Files          62      62           
  Lines        3163    3163           
======================================
  Hits         2809    2809           
  Misses        354     354

@williamFalcon williamFalcon merged commit 73a9118 into Lightning-AI:master Mar 17, 2020
@amoudgl amoudgl deleted the ckpt-fix branch March 17, 2020 23:14
tullie pushed a commit to tullie/pytorch-lightning that referenced this pull request Apr 3, 2020
* check if hparams_type exists in checkpoint dictionary for backward compatibility

* concisely maintain backward compatibility for hparams type

* Bug fix in checkpoint loading (Lightning-AI#1132)
@Borda Borda modified the milestones: v0.7., v0.7.x Apr 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feature Is an improvement or enhancement priority: 0 High priority task ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants