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

Load ckpt path when model provided in validate/test/predict #8352

Merged
merged 40 commits into from
Jul 28, 2021

Conversation

SeanNaren
Copy link
Contributor

@SeanNaren SeanNaren commented Jul 9, 2021

What does this PR do?

Closes #8347

Behaviour now is for test, validate and predict:

trainer.test(model, ckpt_path=None) # use provided model
trainer.test(model, ckpt_path='best') # load best model
trainer.test(model, ckpt_path='my_path') # load path
trainer.fit(model)
# then
trainer.test(ckpt_path=None) # load best model
trainer.test(ckpt_path='my_path') # load path

The biggest change is that if anyone did trainer.test(ckpt_path=None) to load the latest model, they would now need to change this to trainer.test(model) and pass the model reference directly (same for validate/predict).

Before submitting

  • Was this discussed/approved via a GitHub issue? (not for typos and docs)
  • 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?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)
  • Did you list all the breaking changes introduced by this pull request?

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

@SeanNaren SeanNaren changed the title Change trainer loading behaviour for validate/test/predict Change ckpt_path loading behaviour for validate/test/predict Jul 9, 2021
@SeanNaren SeanNaren changed the title Change ckpt_path loading behaviour for validate/test/predict Change ckpt_path behaviour for validate/test/predict Jul 9, 2021
@codecov
Copy link

codecov bot commented Jul 9, 2021

Codecov Report

Merging #8352 (33f8917) into master (b256d6a) will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #8352   +/-   ##
======================================
  Coverage      92%     92%           
======================================
  Files         218     218           
  Lines       14398   14407    +9     
======================================
+ Hits        13296   13305    +9     
  Misses       1102    1102           

@SeanNaren SeanNaren added the feature Is an improvement or enhancement label Jul 12, 2021
@SeanNaren SeanNaren self-assigned this Jul 12, 2021
@SeanNaren SeanNaren added this to the v1.4 milestone Jul 12, 2021
@SeanNaren SeanNaren changed the title Change ckpt_path behaviour for validate/test/predict Load ckpt path when model provided in validate/test/predict Jul 12, 2021
@SeanNaren SeanNaren marked this pull request as ready for review July 12, 2021 09:35
Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

LGTM !

pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved
@SeanNaren SeanNaren disabled auto-merge July 12, 2021 15:24
@SeanNaren SeanNaren requested a review from edenlightning as a code owner July 12, 2021 15:31
@SeanNaren SeanNaren added this to the v1.5 milestone Jul 12, 2021
@SeanNaren SeanNaren added breaking change Includes a breaking change checkpointing Related to checkpointing labels Jul 12, 2021
docs/source/common/test_set.rst Outdated Show resolved Hide resolved
pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved
@mergify mergify bot added ready PRs ready to be merged has conflicts and removed has conflicts labels Jul 27, 2021
@mergify mergify bot removed the has conflicts label Jul 28, 2021
@tchaton tchaton merged commit aadd2a9 into master Jul 28, 2021
@tchaton tchaton deleted the feat/ckpt_load branch July 28, 2021 10:12
leezu pushed a commit to leezu/pytorch-lightning that referenced this pull request Sep 30, 2021
…g-AI#8352)

* Change trainer loading behaviour for validate/test/predict

* Fix

* Fix/add tests

* remove

* Cleanups

* Space

* cleanups

* Add CHANGELOG.md

* Move after setup

* Cleanups on logic

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Remve

* fix test

* feedback

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update pytorch_lightning/trainer/properties.py

Co-authored-by: Carlos Mocholí <[email protected]>

* Feedback

* Same fix

* Same fix

* Add test for behaviour, modify based on feedback

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Wording

* Apply suggestions from code review

Co-authored-by: Kaushik B <[email protected]>
Co-authored-by: Carlos Mocholí <[email protected]>

* Cleanup docs

* Update pytorch_lightning/trainer/trainer.py

Co-authored-by: Kaushik B <[email protected]>

* feedback

* Fixes to test API

* Add carlos description

* Move logic further

* Move checkpoint connector logic

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Carlos Mocholí <[email protected]>
Co-authored-by: Kaushik B <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Includes a breaking change checkpointing Related to checkpointing feature Is an improvement or enhancement ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Load ckpt_path when given to test/validate/predict
4 participants