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

[see #10061 instead] Unify checkpoint load paths #9693

Closed
wants to merge 15 commits into from
Closed

[see #10061 instead] Unify checkpoint load paths #9693

wants to merge 15 commits into from

Conversation

jjenniferdai
Copy link
Contributor

@jjenniferdai jjenniferdai commented Sep 24, 2021 •
edited
Loading

What does this PR do?

Fixes #9405

Does your PR introduce any breaking changes? If yes, please list them.

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 list all the breaking changes introduced by this pull request?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is welcome to review the PR.
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 🙃

ananthsub
ananthsub reviewed Sep 25, 2021
View reviewed changes
Copy link
Contributor

@ananthsub ananthsub left a comment

Choose a reason for hiding this comment

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

this is looking really promising @jjenniferdai ! unifying these paths will make the API consistent and help us simplify the trainer internals

fyi @carmocca who was interested in this too from #9405

pytorch_lightning/trainer/trainer.py Show resolved Hide resolved
pytorch_lightning/trainer/trainer.py

# restore optimizers, etc.
self.checkpoint_connector.restore_training_state()
if self.state.fn == TrainerFn.FITTING:
self.checkpoint_connector.restore_training_state()
Copy link
Contributor

Choose a reason for hiding this comment

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

restore training state includes things which can be resumed even if not fitting, such as the loop state.

imo we shouldn't add the check for fitting here, but rather inside the select parts inside of restore_training_state

Copy link
Contributor Author

@jjenniferdai jjenniferdai Sep 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

(above comment?)

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, we do restore loops there and now some other attributes as well. I'd suggest waiting for this one to get merged: #9413

pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved
@ananthsub ananthsub added design Includes a design discussion checkpointing Related to checkpointing refactor labels Sep 25, 2021
@ananthsub ananthsub added this to the v1.5 milestone Sep 25, 2021
carmocca
carmocca reviewed Sep 27, 2021
View reviewed changes
pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/connectors/checkpoint_connector.py Outdated Show resolved Hide resolved
@jjenniferdai jjenniferdai changed the title [wip] Unify checkpoint load paths Unify checkpoint load paths Sep 28, 2021
@jjenniferdai jjenniferdai marked this pull request as ready for review September 28, 2021 18:11
@mergify mergify bot removed the has conflicts label Sep 30, 2021
ananthsub
ananthsub reviewed Sep 30, 2021
View reviewed changes
pytorch_lightning/trainer/trainer.py

def _fit_impl(
self,
model: "pl.LightningModule",
train_dataloaders: Optional[Union[TRAIN_DATALOADERS, LightningDataModule]] = None,
val_dataloaders: Optional[EVAL_DATALOADERS] = None,
datamodule: Optional[LightningDataModule] = None,
ckpt_path: Optional[str] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

should this also be typed as _PATH ?

pytorch_lightning/trainer/trainer.py
if self.state.fn == TrainerFn.FITTING:
self.checkpoint_connector.restore_training_state()

self.checkpoint_connector.resume_end()
Copy link
Contributor

Choose a reason for hiding this comment

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

n00b question: why is this bumped up to here vs in _pre_training_routine ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now that this calls resume_start for all (not only fitting), similarly resume_end for all as well

@awaelchli awaelchli mentioned this pull request Oct 3, 2021
Closed
@mergify mergify bot added the has conflicts label Oct 6, 2021
@tchaton
Copy link
Contributor

tchaton commented Oct 12, 2021

Hey @jjenniferdai,

Would you mind resolving the conflicts ?

Best,
T.C

@tchaton
Copy link
Contributor

tchaton commented Oct 19, 2021

Dear @@jjenniferdai,

Any updates ?

Best,
T.C

This was referenced Oct 20, 2021
Merged
Merged
@jjenniferdai jjenniferdai changed the title Unify checkpoint load paths [see #10061 instead] Unify checkpoint load paths Oct 21, 2021
@jjenniferdai
Copy link
Contributor Author

sorry all for the git issues :( please see #10061 instead

tchaton pushed a commit that referenced this pull request Oct 25, 2021
@jjenniferdai
Unify checkpoint load paths [redo #9693] (#10061)
6d79184
ninginthecloud pushed a commit to ninginthecloud/pytorch-lightning that referenced this pull request Oct 27, 2021
@jjenniferdai @ninginthecloud
Unify checkpoint load paths [redo Lightning-AI#9693] (Lightning-AI#10061
2db5dd0 
)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carmocca carmocca carmocca left review comments

@ananthsub ananthsub ananthsub left review comments

@awaelchli awaelchli Awaiting requested review from awaelchli

@Borda Borda Awaiting requested review from Borda Borda is a code owner

@justusschock justusschock Awaiting requested review from justusschock justusschock is a code owner

@kaushikb11 kaushikb11 Awaiting requested review from kaushikb11

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

@tchaton tchaton Awaiting requested review from tchaton tchaton is a code owner

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

@rohitgr7 rohitgr7 Awaiting requested review from rohitgr7

Assignees
No one assigned
Labels
checkpointing Related to checkpointing design Includes a design discussion has conflicts refactor
Projects
None yet
Milestone
pl:1.5
Development

Successfully merging this pull request may close these issues.

[checkpoint] Resolve 2 different checkpoint loading paths across fit vs validate/test/predict
6 participants
@jjenniferdai @tchaton @ananthsub @awaelchli @carmocca @rohitgr7