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

feat: add pytorch checkpoint hooks on load/save hooks [DET-5109] #2118

Merged
merged 13 commits into from
Mar 25, 2021
Merged

feat: add pytorch checkpoint hooks on load/save hooks [DET-5109] #2118

merged 13 commits into from
Mar 25, 2021

Conversation

hamidzr
Copy link
Member

@hamidzr hamidzr commented Mar 23, 2021 •
edited
Loading

Description

  • add two checkpoint callbacks to pytorch callbacks.
  • support LightningModule's on_load_checkpoint and on_save_checkpoint hooks

Test Plan

Commentary (optional)

no need for release notes as the lightning adapter is only being officially released at the same time.

Checklist

  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.

@hamidzr hamidzr requested a review from shiyuann as a code owner March 23, 2021 23:04
@cla-bot cla-bot bot added the cla-signed label Mar 23, 2021
shiyuann
shiyuann reviewed Mar 24, 2021
View reviewed changes
Copy link
Contributor

@shiyuann shiyuann left a comment

Choose a reason for hiding this comment

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

Looks good overall. No big comments. Just a few minor comments.

harness/determined/pytorch/_callback.py Outdated Show resolved Hide resolved
harness/determined/pytorch/_pytorch_trial.py Outdated
return

for callback in self.callbacks.values():
# QUESTION: should we encourage users to return new vals instead of
Copy link
Contributor

Choose a reason for hiding this comment

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

LightningModule used the original reference so that you could modify the checkpoint in place. But it looks like either way works.

harness/determined/pytorch/_pytorch_trial.py Outdated Show resolved Hide resolved
harness/tests/experiment/pytorch/test_pytorch_lightning.py Outdated Show resolved Hide resolved
harness/tests/experiment/pytorch/test_pytorch_lightning.py Outdated Show resolved Hide resolved
@shiyuann shiyuann assigned hamidzr and unassigned shiyuann Mar 24, 2021
@hamidzr hamidzr requested a review from shiyuann March 24, 2021 23:42
shiyuann
shiyuann approved these changes Mar 25, 2021
View reviewed changes
Copy link
Contributor

@shiyuann shiyuann left a comment

Choose a reason for hiding this comment

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

LGTM!

@hamidzr hamidzr changed the title feat: add checkpoint hooks to pytorch trial and lightning adapter [DET-5109] feat: add checkpoint hooks on load and save hooks [DET-5109] Mar 25, 2021
@hamidzr hamidzr changed the title feat: add checkpoint hooks on load and save hooks [DET-5109] feat: add checkpoint hooks on load/save hooks [DET-5109] Mar 25, 2021
@hamidzr hamidzr enabled auto-merge (squash) March 25, 2021 17:22
@hamidzr hamidzr changed the title feat: add checkpoint hooks on load/save hooks [DET-5109] feat: add pytorch checkpoint hooks on load/save hooks [DET-5109] Mar 25, 2021
@hamidzr hamidzr merged commit 59f48f3 into determined-ai:master Mar 25, 2021
@hamidzr hamidzr deleted the 5109-checkpoint-hook branch March 25, 2021 17:28
@dannysauer dannysauer added this to the 0.14.6 milestone Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shiyuann shiyuann shiyuann approved these changes

Assignees

@hamidzr hamidzr

Labels
cla-signed
Projects
None yet
Milestone
0.14.6
Development

Successfully merging this pull request may close these issues.
3 participants
@hamidzr @shiyuann @dannysauer