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

Fix for load_from_checkpoint #2776

Merged
merged 21 commits into from
Oct 5, 2020
Merged

Fix for load_from_checkpoint #2776

merged 21 commits into from
Oct 5, 2020

Conversation

jbschiratti
Copy link
Contributor

@jbschiratti jbschiratti commented Jul 31, 2020 •
edited by Borda
Loading

What does this PR do?

Aims at fixes #2550 and fixes #2769

This PR should allow users to load model weights from checkpoints even though self.save_hyperparameters() may not have been called before training. A test was added reproduce the problem described below.

Context

In #2550, the user defines his own LightningModule called MNISTModel . In the __init__ method of MNISTModel, the call to self.save_hyperparameters() is missed. Still, the model is trained and a model checkpoint is created. Later, the user wishes to restore model weights from this checkpoint using:

model = MNISTModel.load_from_checkpoint('mnist.ckpt', net='cnn', h_dim=200)

However, this call to load_from_checkpoint raises an error. As described here, this error is raised because, in load_from_checkpoint, the model to be restored receives multiple values for its class arguments. This problem is also described in #2769

PR review

Anyone in the community is free to review the PR!

cc @Borda @rohitgr7

@mergify mergify bot requested a review from a team July 31, 2020 09:19
@Borda Borda added allowed_pre_1.0 bug Something isn't working priority: 0 High priority task labels Jul 31, 2020
@ananyahjha93 ananyahjha93 self-requested a review July 31, 2020 19:00
@jbschiratti jbschiratti changed the title Fix for load_from_checkpoint [WIP] Fix for load_from_checkpoint Aug 3, 2020
@jbschiratti
Copy link
Contributor Author

@Borda : test_no_val_module (with url_ckpt=True) is passing locally but keeps failing on the CI. Do you have any idea why? Shall I add a pip freeze here?

@Borda
Copy link
Member

Borda commented Aug 3, 2020 •
edited by rohitgr7
Loading

@Borda : test_no_val_module (with url_ckpt=True) is passing locally but keeps failing on the CI. Do you have any idea why? Shall I add a pip freeze here?

mind rather use one of our devel images - pytorchlightning/pytorch_lightning:cuda-extras-py3.7-torch1.6.0

@edenlightning edenlightning linked an issue Aug 3, 2020 that may be closed by this pull request
load_from_checkpoint raises TypeError when **kwargs is provided #2612
Closed
@jbschiratti jbschiratti changed the title [WIP] Fix for load_from_checkpoint Fix for load_from_checkpoint Aug 4, 2020
@jbschiratti
Copy link
Contributor Author

@Borda Thanks! I found the problem and fixed it. The PR should be ready to be reviewed.

Borda
Borda reviewed Aug 4, 2020
View reviewed changes
@mergify mergify bot requested a review from a team August 4, 2020 15:40
@Borda
Copy link
Member

Borda commented Aug 4, 2020 •
edited
Loading

Borda Thanks! I found the problem and fixed it. The PR should be ready to be reviewed.

cool, mind share what was the problem?

@Borda Borda added this to the 0.9.0 milestone Aug 6, 2020
@pep8speaks
Copy link

pep8speaks commented Aug 6, 2020 •
edited
Loading

Hello @jbschiratti! Thanks for updating this PR.

Line 178:121: E501 line too long (124 > 120 characters)

Line 541:50: W605 invalid escape sequence '('
Line 541:52: W605 invalid escape sequence ')'
Line 543:1: W391 blank line at end of file

Comment last updated at 2020-10-05 15:25:01 UTC

@Borda
Copy link
Member

Borda commented Aug 6, 2020

@jbschiratti seems like I made a mistake in rebasing, mind help to get it back... probably just yous your local version...

@mergify
Copy link
Contributor

mergify bot commented Aug 12, 2020

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

@awaelchli
Copy link
Contributor

Hi @jbschiratti, I just looked at this PR to try and fix the failed tests, but I find it hard to understand all of your changes. I understand the issue in #2550 and why it happens, and I can verify that your PR fixes the problem, but unfortunately the current state seems to fail other tests.

@mergify
Copy link
Contributor

mergify bot commented Aug 13, 2020

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

@williamFalcon
Copy link
Contributor

williamFalcon commented Aug 15, 2020 •
edited
Loading

@jbschiratti @awaelchli ... the added test also passes on master. Not sure what this is fixing now.

Either the test is wrong, or this is not a bug on master

williamFalcon
williamFalcon requested changes Aug 15, 2020
View reviewed changes
Copy link
Contributor

@williamFalcon williamFalcon left a comment

Choose a reason for hiding this comment

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

not sure it's a real bug

@mergify mergify bot requested a review from a team August 15, 2020 13:05
@mergify
Copy link
Contributor

mergify bot commented Aug 20, 2020

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

@edenlightning edenlightning removed this from the 0.9.0 milestone Aug 20, 2020
jbschiratti and others added 20 commits October 5, 2020 08:19
@jbschiratti @Borda
Fix.
cca6eb4
@jbschiratti @Borda
Fix Lightning-AI#2550: allow to load model from checkpoint if self.sa…
ac390ba 
…ve_hyperparameters() was not called.
@jbschiratti @Borda
Fix? Cleaner way of not calling self.save_hyperparameters in EvalMode…
47ddbbf 
…lTemplate.
@jbschiratti @Borda
Fix? _load_model_state cleanup
4a1284c
@jbschiratti @Borda
Fix?
fb2108f
@jbschiratti @Borda
Fix Lightning-AI#2550: allow to load model from checkpoint if self.sa…
6771ebc 
…ve_hyperparameters() was not called.
@jbschiratti @Borda
Fix.
59970c9
@jbschiratti @Borda
Fix? Cleaner way of not calling self.save_hyperparameters in EvalMode…
3f2bdf1 
…lTemplate.
@jbschiratti @Borda
Fix? _load_model_state cleanup
05bcc4a
@jbschiratti @Borda
Fixed side effect in test_load_model_from_checkpoint_extra_args.
40672f9
@Borda
Apply suggestions from code review
76f5d80
@Borda
fix
cc7857c
@Borda
try
c4bc743
@williamFalcon @Borda
fixed missing arg in evalmodel
1de769c
@williamFalcon @Borda
fixed missing arg in evalmodel
22bf685
@Borda
fix
abaaa6e
@Borda
update
765debe
@Borda
fix loading
16420da
@Borda
add test
f468900
@Borda
prune
91fdd65
@Borda Borda added the ready PRs ready to be merged label Oct 5, 2020
@mergify mergify bot requested a review from a team October 5, 2020 07:44
@mergify
Copy link
Contributor

mergify bot commented Oct 5, 2020

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

@williamFalcon williamFalcon merged commit cea5f1f into Lightning-AI:master Oct 5, 2020
@Borda Borda modified the milestones: 1.0, 0.10.0 Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Borda Borda Borda approved these changes

@justusschock justusschock justusschock approved these changes

@rohitgr7 rohitgr7 rohitgr7 approved these changes

@ananyahjha93 ananyahjha93 Awaiting requested review from ananyahjha93

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

@awaelchli awaelchli Awaiting requested review from awaelchli

@nateraw nateraw Awaiting requested review from nateraw

@SkafteNicki SkafteNicki Awaiting requested review from SkafteNicki

@teddykoker teddykoker Awaiting requested review from teddykoker

@yukw777 yukw777 Awaiting requested review from yukw777

Assignees

@Borda Borda

Labels
bug Something isn't working checkpointing Related to checkpointing priority: 0 High priority task ready PRs ready to be merged
Projects
None yet
Milestone
pl:0.10
8 participants
@jbschiratti @Borda @pep8speaks @awaelchli @williamFalcon @justusschock @rohitgr7 @edenlightning