Support checkpoint hooks on data module #3563

ananthsub · 2020-09-19T22:21:15Z

What does this PR do?

Add a new class CheckpointHooks which both the LightningModule and LightningDataModule inherit
Call trainer.datamodule.on_{save/load}_checkpoint inside

I'd appreciate comments as to whether the order here matters for saving/loading. Ideally the checkpoint state is independent across data and model, but if there are cases where the model depends on the data, should we prefer to call one's save/load before the other?

TODO:

Add tests

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 🙃

pep8speaks · 2020-09-19T22:21:20Z

Hello @ananthsub! Thanks for updating this PR.

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

Comment last updated at 2020-09-28 23:30:16 UTC

pytorch_lightning/core/hooks.py

mergify · 2020-09-20T01:48:01Z

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

codecov · 2020-09-20T03:05:11Z

Codecov Report

Merging #3563 into master will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #3563   +/-   ##
======================================
- Coverage      91%     91%   -0%     
======================================
  Files         110     110           
  Lines        8206    8211    +5     
======================================
- Hits         7463    7457    -6     
- Misses        743     754   +11

pytorch_lightning/callbacks/model_checkpoint.py

Borda

Can we pls stop using meaningless Black formatting, e.g replacing " and ' or inconsistently splitting argument lines... this makes the PR terribly long even for minimal changes
add to you your Black config -S

pytorch_lightning/core/datamodule.py

Split out changes from Lightning-AI#3563 to make that PR easier to review

Split out changes from Lightning-AI#3563 to make that PR easier to review. This formats the file according to the Black formatter

…view. This formats the file according to the Black formatter

* Split out changes from #3563 to make that PR easier to review. This formats the file according to the Black formatter * Store a reference to the trainer on the datamodule Fixes #3682 * Update data_connector.py * Update data_connector.py * Update test_datamodules.py

… more storage backends (#3694) * Split out changes from #3563 to make that PR easier to review. This formats the file according to the Black formatter * Store a reference to the trainer on the datamodule Fixes #3682 * Update data_connector.py * Update data_connector.py * Update test_datamodules.py * Support more storage backends in trainer.test using best weights Similar to #3692 * Update trainer.py * Update trainer.py use cloud_io load directly

mergify · 2020-09-28T23:11:12Z

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

…view. This formats the file according to the Black formatter

refactor on_{save/load}_checkpoint to a separate hook class that both the lightning module and data module inherit add spots in callback connector to call new datamodule hooks if available

checkout upstream/master

nateraw

wrote up some local scripts to play around with this. it works as expected.

LGTM 🚀

…view. This formats the file according to the Black formatter

* Split out changes from #3563 to make that PR easier to review. This formats the file according to the Black formatter * Store a reference to the trainer on the datamodule Fixes #3682 * Update data_connector.py * Update data_connector.py * Update test_datamodules.py * Add attribute to datamodule for trainer

…view. This formats the file according to the Black formatter

ananthsub changed the title ~~Data ckpt~~ Support checkpoint hooks on data module Sep 19, 2020

mergify bot requested a review from a team September 19, 2020 22:22

awaelchli reviewed Sep 19, 2020

View reviewed changes

pytorch_lightning/core/hooks.py Show resolved Hide resolved

mergify bot requested a review from a team September 19, 2020 23:56

ananthsub force-pushed the data-ckpt branch 3 times, most recently from b20dc2c to f1f90b5 Compare September 20, 2020 01:47

ananthsub force-pushed the data-ckpt branch 4 times, most recently from 5bb71e2 to 0a7346d Compare September 20, 2020 02:43

ananthsub commented Sep 20, 2020

View reviewed changes

pytorch_lightning/callbacks/model_checkpoint.py Outdated Show resolved Hide resolved

ananthsub force-pushed the data-ckpt branch from 0a7346d to aa6b96a Compare September 20, 2020 03:57

Borda added the feature Is an improvement or enhancement label Sep 20, 2020

Borda reviewed Sep 20, 2020

View reviewed changes

mergify bot requested a review from a team September 20, 2020 08:45

Borda reviewed Sep 20, 2020

View reviewed changes

pytorch_lightning/core/datamodule.py Show resolved Hide resolved

mergify bot requested review from a team September 20, 2020 08:59

ananthsub added a commit to ananthsub/pytorch-lightning that referenced this pull request Sep 20, 2020

Black format pytorch_lightning/core/datamodule.py

c335115

Split out changes from Lightning-AI#3563 to make that PR easier to review

ananthsub mentioned this pull request Sep 20, 2020

Black format pytorch_lightning/core/datamodule.py #3574

Merged

7 tasks

ananthsub added a commit to ananthsub/pytorch-lightning that referenced this pull request Sep 20, 2020

Black format pytorch_lightning/core/hooks.py

330dadc

Split out changes from Lightning-AI#3563 to make that PR easier to review. This formats the file according to the Black formatter

ananthsub mentioned this pull request Sep 20, 2020

Black format pytorch_lightning/core/hooks.py #3575

Merged

7 tasks

ananthsub added a commit to ananthsub/pytorch-lightning that referenced this pull request Sep 20, 2020

Update lightning.py

aad6577

Split out changes from Lightning-AI#3563 to make that PR easier to review. This formats the file according to the Black formatter

ananthsub mentioned this pull request Sep 20, 2020

Black format pytorch_lightning/core/lightning.py #3576

Merged

7 tasks

ananthsub added a commit to ananthsub/pytorch-lightning that referenced this pull request Sep 20, 2020

Split out changes from Lightning-AI#3563 to make that PR easier to re…

fd5c85c

…view. This formats the file according to the Black formatter

ananthsub mentioned this pull request Sep 21, 2020

Black format pytorch_lightning/trainer/connectors/checkpoint_connector.py #3579

Closed

7 tasks

ananthsub added 2 commits September 27, 2020 16:13

Update data_connector.py

6513166

Update test_datamodules.py

0f3c670

ananthsub added 2 commits September 27, 2020 18:47

Merge branch 'trainer-dm'

cc16b35

Merge remote-tracking branch 'upstream/master'

b56cdef

ananthsub added 15 commits September 28, 2020 16:28

Merge remote-tracking branch 'upstream/master'

9ae5a79

Split out changes from Lightning-AI#3563 to make that PR easier to re…

b47136f

…view. This formats the file according to the Black formatter

support checkpoint hooks for datamodule

61c92bd

refactor on_{save/load}_checkpoint to a separate hook class that both the lightning module and data module inherit add spots in callback connector to call new datamodule hooks if available

hooks formatting

0c4bfa8

Update hooks.py

10133b2

Update checkpoint_connector.py

6209b21

Update lightning.py

2baf2a7

update based on upstream/master

47972ae

checkout upstream/master

Update checkpoint_connector.py

d3351da

add tests

4914334

undo format revert

62067c8

Updated CHANGELOG.md

ced6d27

add checkpoint hooks

1cfacaa

add Dict type

1c14b5e

import CheckpointHooks

e3bec6c

ananthsub force-pushed the data-ckpt branch from ccec667 to e3bec6c Compare September 28, 2020 23:30

nateraw approved these changes Sep 29, 2020

View reviewed changes

awaelchli approved these changes Sep 29, 2020

View reviewed changes

awaelchli merged commit 3dcf713 into Lightning-AI:master Sep 29, 2020

ananthsub added a commit to ananthsub/pytorch-lightning that referenced this pull request Sep 30, 2020

Split out changes from Lightning-AI#3563 to make that PR easier to re…

604e072

…view. This formats the file according to the Black formatter

Borda pushed a commit to ananthsub/pytorch-lightning that referenced this pull request Oct 2, 2020

Split out changes from Lightning-AI#3563 to make that PR easier to re…

14b2cc4

…view. This formats the file according to the Black formatter

Borda added this to the 0.10.0 milestone Oct 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support checkpoint hooks on data module #3563

Support checkpoint hooks on data module #3563

ananthsub commented Sep 19, 2020 •

edited

Loading

pep8speaks commented Sep 19, 2020 •

edited

Loading

mergify bot commented Sep 20, 2020

codecov bot commented Sep 20, 2020 •

edited

Loading

Borda left a comment •

edited

Loading

mergify bot commented Sep 28, 2020

nateraw left a comment

Support checkpoint hooks on data module #3563

Support checkpoint hooks on data module #3563

Conversation

ananthsub commented Sep 19, 2020 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

pep8speaks commented Sep 19, 2020 • edited Loading

Comment last updated at 2020-09-28 23:30:16 UTC

mergify bot commented Sep 20, 2020

codecov bot commented Sep 20, 2020 • edited Loading

Codecov Report

Borda left a comment • edited Loading

Choose a reason for hiding this comment

mergify bot commented Sep 28, 2020

nateraw left a comment

Choose a reason for hiding this comment

ananthsub commented Sep 19, 2020 •

edited

Loading

pep8speaks commented Sep 19, 2020 •

edited

Loading

codecov bot commented Sep 20, 2020 •

edited

Loading

Borda left a comment •

edited

Loading