Added strict=False for load_from_checkpoint #2819

shijianjian · 2020-08-04T10:10:02Z

What does this PR do?

Enabled functionality as

 model = system.load_from_checkpoint(ckpt_path, hparams_file=vars(hparams), strict=False)

Fixes #2629

Before submitting

Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
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?
Did you verify new and existing tests pass locally with your changes?
If you made a notable change (that affects users), did you update the CHANGELOG?

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 🙃

codecov · 2020-08-04T10:23:10Z

Codecov Report

Merging #2819 into master will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #2819   +/-   ##
======================================
  Coverage      90%     90%           
======================================
  Files          81      81           
  Lines        7553    7553           
======================================
+ Hits         6796    6797    +1     
+ Misses        757     756    -1

Borda

pls rebase on master and do not add removed/deprecated API

williamFalcon · 2020-08-04T12:53:48Z

@shijianjian can you add a test for this?

pytorch_lightning/core/saving.py

pep8speaks · 2020-08-04T17:54:47Z

Hello @shijianjian! Thanks for updating this PR.

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

Comment last updated at 2020-08-13 19:51:47 UTC

awaelchli · 2020-08-04T18:15:44Z

Do I understand correctly, this PR also adds unrelated to strict kwarg the possibility to have hparams_file a dict? Why do we need that? Isn't it the same as passing the dict as kwargs?

shijianjian · 2020-08-06T06:21:56Z

Do I understand correctly, this PR also adds unrelated to strict kwarg the possibility to have hparams_file a dict? Why do we need that? Isn't it the same as passing the dict as kwargs?

@awaelchli The only reason that I included that is because of the hparams modification by hparams['on_gpu'] = False. I will remove those code if anyone can confirm that this specific modification is not needed.

awaelchli · 2020-08-06T06:32:42Z

Not sure why we override the on_gpu=False, I guess it is assumed we always load the checkpoint to CPU, which is probably not necessary. But I think that requires a different solution. The reason why I am not so convinced with the hprams file as a dict is:

Now there are two different ways to do the same thing, which in my opinion is not a great design. Will the user know that there is no difference?
Two ways of doing the same thing will require also twice the amounts of tests, essentially duplicated, and twice the amount of maintenance effort.
The meaning of the argname "hparams_file" is now misleading if it can be a dict

shijianjian · 2020-08-06T07:01:24Z

@awaelchli I have no objections to remove it since it is only used for my own project, as long as you confirmed that the hparams['on_gpu'] = False is not needed here.

I will remove the corresponding code soon.

Thank you.

shijianjian · 2020-08-06T17:20:00Z

Should be able to be merged.

tests/trainer/test_trainer.py

mergify · 2020-08-06T18:26:01Z

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

Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>

rohitgr7 · 2020-08-13T16:49:20Z

still, some pep8 issues are there.

tests/models/test_restore.py

outdated

* Added strict=False and hparams_file accepcts dict * Apply suggestions from code review Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com> * Type check fix * Added tests * Linting & test fix * Removed redundant code & test * Added strict=False and hparams_file accepcts dict * Apply suggestions from code review Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com> * Type check fix * Added tests * Linting & test fix * Removed redundant code & test * Apply suggestions from code review * tests * tests * chlog * Update tests/models/test_restore.py Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com> * update test comments * Added docstring for the strict attribute * Added supplementary tests * Update saving.py * Apply suggestions from code review Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com> * pep8, removed extra func Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com> Co-authored-by: Jirka Borovec <jirka@pytorchlightning.ai> Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com> Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com> Co-authored-by: ananyahjha93 <ananya@pytorchlightning.ai>

mergify bot requested a review from a team August 4, 2020 10:10

Borda added the feature Is an improvement or enhancement label Aug 4, 2020

Borda requested changes Aug 4, 2020

View reviewed changes

mergify bot requested a review from a team August 4, 2020 10:42

justusschock previously requested changes Aug 4, 2020

View reviewed changes

pytorch_lightning/core/saving.py Outdated Show resolved Hide resolved

mergify bot requested a review from a team August 4, 2020 15:01

rohitgr7 reviewed Aug 4, 2020

View reviewed changes

pytorch_lightning/core/saving.py Outdated Show resolved Hide resolved

mergify bot requested a review from a team August 4, 2020 16:08

shijianjian requested review from rohitgr7, Borda and justusschock and removed request for a team August 6, 2020 17:19

mergify bot requested a review from a team August 6, 2020 17:20

Borda added the design Includes a design discussion label Aug 6, 2020

Borda reviewed Aug 6, 2020

View reviewed changes

tests/trainer/test_trainer.py Outdated Show resolved Hide resolved

tests/trainer/test_trainer.py Outdated Show resolved Hide resolved

mergify bot requested a review from a team August 6, 2020 18:03

Borda approved these changes Aug 6, 2020

View reviewed changes

Borda added the ready PRs ready to be merged label Aug 6, 2020

mergify bot requested a review from a team August 6, 2020 18:26

Borda requested review from ethanwharris and jeremyjordan August 6, 2020 18:34

shijianjian and others added 16 commits August 13, 2020 18:41

Added strict=False and hparams_file accepcts dict

83216d9

Apply suggestions from code review

222af93

Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>

Type check fix

ba5f890

Added tests

0fb0317

Linting & test fix

153d343

Removed redundant code & test

f1ade10

Apply suggestions from code review

518dc01

tests

3fcfa28

tests

6e51a1d

chlog

1a4ff06

Update tests/models/test_restore.py

0d9e3bb

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>

update test comments

16ce28a

Added docstring for the strict attribute

a82c52a

Added supplementary tests

3e25bf5

Update saving.py

9ac50cf

Apply suggestions from code review

4cae280

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>

Borda modified the milestones: 1.0.0, 0.9.x Aug 13, 2020

ananyahjha93 suggested changes Aug 13, 2020

View reviewed changes

tests/models/test_restore.py Outdated Show resolved Hide resolved

mergify bot requested a review from a team August 13, 2020 19:46

pep8, removed extra func

08f09a0

ananyahjha93 added the ready PRs ready to be merged label Aug 13, 2020

ananyahjha93 approved these changes Aug 13, 2020

View reviewed changes

ananyahjha93 merged commit 53f855c into Lightning-AI:master Aug 13, 2020

Borda modified the milestones: 0.9.x, 0.9.0 Aug 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added strict=False for load_from_checkpoint #2819

Added strict=False for load_from_checkpoint #2819

shijianjian commented Aug 4, 2020

codecov bot commented Aug 4, 2020 •

edited

Loading

Borda left a comment

williamFalcon commented Aug 4, 2020

pep8speaks commented Aug 4, 2020 •

edited

Loading

awaelchli commented Aug 4, 2020

shijianjian commented Aug 6, 2020

awaelchli commented Aug 6, 2020

shijianjian commented Aug 6, 2020

shijianjian commented Aug 6, 2020

mergify bot commented Aug 6, 2020

rohitgr7 commented Aug 13, 2020

Added strict=False for load_from_checkpoint #2819

Added strict=False for load_from_checkpoint #2819

Conversation

shijianjian commented Aug 4, 2020

What does this PR do?

Before submitting

PR review

Did you have fun?

codecov bot commented Aug 4, 2020 • edited Loading

Codecov Report

Borda left a comment

Choose a reason for hiding this comment

williamFalcon commented Aug 4, 2020

pep8speaks commented Aug 4, 2020 • edited Loading

Comment last updated at 2020-08-13 19:51:47 UTC

awaelchli commented Aug 4, 2020

shijianjian commented Aug 6, 2020

awaelchli commented Aug 6, 2020

shijianjian commented Aug 6, 2020

shijianjian commented Aug 6, 2020

mergify bot commented Aug 6, 2020

rohitgr7 commented Aug 13, 2020

codecov bot commented Aug 4, 2020 •

edited

Loading

pep8speaks commented Aug 4, 2020 •

edited

Loading