Callbacks continued

[hadim]

This PR is based on #849.

A new callback mechanism is added where a user can pass a list of callbacks to the trainer that is called during the various steps of the trainer's life.

from pytorch_lightning.callbacks import Callback

class PrintCallback(Callback):
    def on_fit_begin(self):
        print("Training is started!")

    def on_fit_end(self):
        print(f"Training is done. The logs are: {self.trainer.logs}")

# a list of callbacks
callbacks = [PrintCallback()]
trainer = Trainer(callbacks=callbacks)

It allows inserting custom logic in the training loop outside of the LightningModule.

Let me know what do you guys think.

[pep8speaks]

Hello @hadim! Thanks for updating this PR.

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

Comment last updated at 2020-02-26 00:59:24 UTC

[hadim]

@williamFalcon about immutability, I had a look and I don't think this is something easy todo in Python without heavy hacks. See https://discuss.python.org/t/immutability-in-python-is-really-hard/2536/2

I think we should just make it very clear in the doc/docstring that callbacks MUST NOT modify the pl module and the trainer.

[williamFalcon]

why do we want to add this limitation? shouldn’t the user be ok with messing around with the trainer or model if they want to in a callback?

[williamFalcon]

also, should we rethink the name “Callback”?

this ALWAYS ends up needing a lengthy explanation about what it is. why not call it something more intuitive?

Maybe something like:
“FunctionBlock”
“CodeBlock”
“Snippet”

just brainstorming here lol. i just think it’s super confusing for newcomers and non engineers.

[hadim]

I haven't totally incorporated the 3 already existant callbacks in the new system (they still use the new Callback class). I think the merge of the existant callbacks should be done in another PR to not make this one too large in terms of changes.

About the name, I like Callback but I don't have strong feelings about it so whatever you decide is good for me.

[jeremyjordan]

I haven't totally incorporated the 3 already existant callbacks in the new system (they still use the new Callback class). I think the merge of the existant callbacks should be done in another PR to not make this one too large in terms of changes.

Regarding this, should we update the early_stopping and model_checkpointing args to serve as simple flags to include the callbacks (or whatever we call them) in the list?

Trainer(..., checkpoint_callback=True)  # use default
Trainer(..., checkpoint_callback=False)  # disable

checkpoint_callback = ModelCheckpoint(filepath='my_path')
Trainer(..., checkpoint_callback=checkpoint_callback)  # warn user to pass via callbacks arg
Trainer(..., callbacks=[checkpoint_callback])  # use checkpoint callback with user provided spec

[jeremyjordan]

why do we want to add this limitation? shouldn’t the user be ok with messing around with the trainer or model if they want to in a callback?

Yes, but if the user can modify the trainer then the callback quickly transitions from non-essential to essential for reproducibility.

However, there are cases when it would be desirable to modify objects in the trainer. For example, fast.ai has a callback for a learning rate finder which is quite useful. This would be something that the user would want to enable when first building a model but then disable once they find a good range and want to start training.

[Borda]

Regarding this, should we update the early_stopping and model_checkpointing args to serve as simple flags to include the callbacks (or whatever we call them) in the list?

Then we shall check that they are not duplicated, meaning when they are added via simple flag and later among callbacks?

[jeremyjordan]

Ah yes, good point 😄 Perhaps in that case, we notice that an instance of ModelCheckpoint was provided in the callback list and issue a warning that the user provided instance will be used in place of the default instance.

checkpoint_callback = ModelCheckpoint(filepath='my_path')

Trainer(..., checkpoint_callback=True)  # use default
Trainer(..., callbacks=[checkpoint_callback])  # use checkpoint callback with user provided spec
Trainer(..., checkpoint_callback=True, callbacks=[checkpoint_callback])  # use checkpoint callback with user provided spec and alert user via a warning

What do you think? Does this resolve any potential confusion?

[Borda]

I think that the question is very hard because you do not have much clues if double callback (e.g. CheckPoint) is mistake or desired state... Maybe you would need to match also parameters of the ChP to determine whether or not they are identical...

[hadim]

What do you think if we finish the design of the callback system and discuss the way we integrate the existing callbacks in a new PR/issue?

Do you have any comments about the one implemented in this PR?

[jeremyjordan]

nice work on this! just a few comments

[jeremyjordan]

should we also assert that the trainer and pl_module arg values are an instance of the expected class?

[jeremyjordan]

it's weird for a callback to invoke one of its own methods. i know this is how the EarlyStopping callback was previously written, but do you happen to know why this is the case? i would expect on_train_begin to be called when the TrainerCallbackHookMixin invokes it.

[hadim]

I agree it's weird and should be fixed.

[jeremyjordan]

@hadim could we consolidate the code such that model hooks and callbacks are invoked at the same time, and within the context of the a profiler?

with self.profiler.profile('on_epoch_start'):
    self.on_epoch_start()  # callbacks
    if self.is_function_implemented('on_epoch_start'):
        model = self.get_model()
        model.on_epoch_start()  # model hook

[hadim]

Yes, we should. Not sure when I'll have time to work on this PR this week. So we could merge it as it is and improve it in other PRs or if someone wants to push some commits here, feel free (I can add you to my fork).

[williamFalcon]

@hadim why don't we merge this asap so we can get it in the release.
@jeremyjordan maybe open a new PR to implement your suggestions? i do like the profiler addition here.

Great job everyone!

[hadim]

I agree @williamFalcon

Will rebase soon.

[hadim]

Rebasing wasn't a very funny moment here!

Please merge whenever you can @williamFalcon!

I'll open a new issue for what's next.

[jeremyjordan]

happy to pick up the work on a separate PR

[hadim]

@jeremyjordan look at #947

[Borda]

@hadim @jeremyjordan GREAT job! Pls could you update also the change log...