consolidate callbacks and hooks #950

jeremyjordan · 2020-02-26T05:16:59Z

Before submitting

Was this discussed/approved via a Github issue? (no need for typos, doc improvements)
Did you read the contributor guideline?
Did you make sure to update the docs?
Did you write any new necessary tests?
If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Fixes #947

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 🙃

hadim · 2020-02-26T13:07:27Z

Thanks! Could you also update the changelog? I forgot to do it in the last PR.

jeremyjordan · 2020-02-26T14:19:52Z

@hadim sure thing :)

jeremyjordan · 2020-02-27T03:42:31Z

To do

Consolidate model hooks and callbacks system. Make the calls at the same moment: Callbacks continued #889 (comment)
Fix on_train_begin() being called in the early stopping callbacks: Callbacks continued #889 (comment)
Assert pl_module and trainer are excepted object instances in the callback system train tests: Callbacks continued #889 (comment)
Incorporate existing callbacks into the new callback system.
Edit the changelog

williamFalcon · 2020-02-27T22:27:47Z

@jeremyjordan let's get this into this release?

jeremyjordan · 2020-02-27T23:53:41Z

@williamFalcon for sure! would like to wrap this one up tonight

williamFalcon · 2020-02-28T02:22:44Z

idk if this is out of scope, but we have the other callbacks which are trainer specific.
(backward, init_ddp, etc...) that are deferred to the lightningModule.

Do we want to put all of those in the hooks file as well? or structure them differently?

No change to the user, just the docs

jeremyjordan · 2020-02-28T03:01:33Z

@williamFalcon that's a good question. i think those methods make sense where they are in the docs, but i do agree the doc page for LightningModule is pretty lengthy. i do it would really help for readability and quick reference if we could copy the "Shortcuts" panel that PyTorch has in their docs.

jeremyjordan · 2020-02-28T03:25:26Z

I'm thinking that I'll hold off integrating the existing callbacks for a subsequent PR, then we can have more discussion regarding the best way to address integration concerns (eg. #889 comment). For now we can manually invoke the callback events as we're doing currently.

Borda

LGTM 🚀, just move tests to a separate file

tests/trainer/test_trainer.py

pytorch_lightning/trainer/training_loop.py

Borda

it would be nice to unify name convention between model and trainer in particular is_function_implemented in trainer and is_overriden in model

pytorch_lightning/trainer/training_loop.py

tests/trainer/test_callbacks.py

jeremyjordan · 2020-03-02T03:19:12Z

it would be nice to unify name convention between model and trainer in particular is_function_implemented in trainer and is_overriden in model

is there a reason we check for is_overriden in model rather than just having the default hooks pass and always calling the hooks?

jeremyjordan · 2020-03-02T22:20:07Z

@williamFalcon are you still hoping to get this into the next release?

williamFalcon · 2020-03-02T22:23:21Z

yup! was focused on the .test thing. will merge today

Borda · 2020-03-02T22:58:16Z

just stay tuned, because with some extensive PRs we may come to solving complex collisions 🤖

* consolidate callbacks and hooks * ensure callbacks recieve proper arg types * remove model from init callback events * clean up early stopping event * update changelog * remove on_fit_start and on_fit_end * fix args for on_init_start and on_init_end * handle case where early stopping is not used * show all callback methods * wrap checkpoint callback logic into proper class * fix check for main process in checkpoint callback * move callbacks test to separate file * refactor arg checks * get model and call hook on same line * define trainer_options dict in one call * add more asserts to callback test

consolidate callbacks and hooks

caa5b41

Jeremy Jordan added 2 commits February 26, 2020 22:19

ensure callbacks recieve proper arg types

f4aa38d

remove model from init callback events

d50f4a1

Jeremy Jordan added 4 commits February 26, 2020 22:43

clean up early stopping event

a054dc7

Merge branch 'master' into feature/improve-callbacks

136afb8

update changelog

33f988d

remove on_fit_start and on_fit_end

4b253ea

Merge branch 'master' into feature/improve-callbacks

1e4effe

Jeremy Jordan added 2 commits February 27, 2020 21:25

fix args for on_init_start and on_init_end

b2b38be

handle case where early stopping is not used

9cc1df7

show all callback methods

fa50ecb

jeremyjordan changed the title ~~[WIP] consolidate callbacks and hooks~~ consolidate callbacks and hooks Feb 28, 2020

wrap checkpoint callback logic into proper class

d69cadc

jeremyjordan requested review from williamFalcon and Borda February 28, 2020 03:39

fix check for main process in checkpoint callback

f72591e

Borda added the feature Is an improvement or enhancement label Feb 28, 2020

Borda added this to the 0.7.0 milestone Feb 28, 2020

Borda approved these changes Feb 28, 2020

View reviewed changes

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

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

Jeremy Jordan added 2 commits February 28, 2020 13:07

move callbacks test to separate file

ae70c9b

refactor arg checks

6ad7c57

Borda reviewed Feb 29, 2020

View reviewed changes

pytorch_lightning/trainer/training_loop.py Show resolved Hide resolved

Borda reviewed Feb 29, 2020

View reviewed changes

pytorch_lightning/trainer/training_loop.py Show resolved Hide resolved

Borda reviewed Feb 29, 2020

View reviewed changes

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

Borda reviewed Feb 29, 2020

View reviewed changes

Jeremy Jordan added 3 commits March 1, 2020 22:22

get model and call hook on same line

ac91cb9

define trainer_options dict in one call

2267e6e

add more asserts to callback test

6a776a5

Borda added the ready PRs ready to be merged label Mar 2, 2020

williamFalcon merged commit 705e576 into Lightning-AI:master Mar 3, 2020

Borda mentioned this pull request Mar 4, 2020

Training metrics #100

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

consolidate callbacks and hooks #950

consolidate callbacks and hooks #950

jeremyjordan commented Feb 26, 2020 •

edited

Loading

hadim commented Feb 26, 2020

jeremyjordan commented Feb 26, 2020

jeremyjordan commented Feb 27, 2020 •

edited

Loading

williamFalcon commented Feb 27, 2020

jeremyjordan commented Feb 27, 2020

williamFalcon commented Feb 28, 2020 •

edited

Loading

jeremyjordan commented Feb 28, 2020

jeremyjordan commented Feb 28, 2020 •

edited

Loading

Borda left a comment

Borda left a comment

jeremyjordan commented Mar 2, 2020

jeremyjordan commented Mar 2, 2020

williamFalcon commented Mar 2, 2020

Borda commented Mar 2, 2020

consolidate callbacks and hooks #950

consolidate callbacks and hooks #950

Conversation

jeremyjordan commented Feb 26, 2020 • edited Loading

Before submitting

What does this PR do?

PR review

Did you have fun?

hadim commented Feb 26, 2020

jeremyjordan commented Feb 26, 2020

jeremyjordan commented Feb 27, 2020 • edited Loading

To do

williamFalcon commented Feb 27, 2020

jeremyjordan commented Feb 27, 2020

williamFalcon commented Feb 28, 2020 • edited Loading

jeremyjordan commented Feb 28, 2020

jeremyjordan commented Feb 28, 2020 • edited Loading

Borda left a comment

Choose a reason for hiding this comment

Borda left a comment

Choose a reason for hiding this comment

jeremyjordan commented Mar 2, 2020

jeremyjordan commented Mar 2, 2020

williamFalcon commented Mar 2, 2020

Borda commented Mar 2, 2020

jeremyjordan commented Feb 26, 2020 •

edited

Loading

jeremyjordan commented Feb 27, 2020 •

edited

Loading

williamFalcon commented Feb 28, 2020 •

edited

Loading

jeremyjordan commented Feb 28, 2020 •

edited

Loading