Error if callbacks are passed wrongly #1544

SkafteNicki · 2020-04-21T11:25:43Z

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 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 #1524.
If the user pass a ModelCheckpoint to trainer argument callback instead of checkpoint_callback, it will give a non-informative error. This PR implement a simple check that callbacks passed to trainer argument callback are not instances of ModelCheckpoint or EarlyStopping since these need to be passed to other trainer args.

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 🙃

jeremyjordan · 2020-04-23T02:49:19Z

i think we actually want to allow this behavior, and instead limit the scope of checkpoint_callback and early_stopping_callback.

i think we want to standardize towards having all of the callbacks passed into the callbacks argument as a list (and remove cases where the Trainer is directly invoking callbacks). the checkpoint_callback and early_stopping_callback args should change to only accept bool where we can construct the default callback object and insert it into the callbacks list.

SkafteNicki · 2020-04-23T10:15:08Z

Right, that is a much better way of handling this in the future. @jeremyjordan should I go ahead and make the change that you propose? It would be a change that is not backward compatible so some DeprecationWarnings needs to be thrown, and in that case we need to settle for a future version where the option to pass callbacks to checkpoint_callback and early_stopping_callback is not valid anymore.

Borda · 2020-04-23T11:54:27Z

pytorch_lightning/trainer/callback_config.py

+        for callback in self.callbacks:
+            if isinstance(callback, ModelCheckpoint):
+                raise MisconfigurationException(
+                    'You passed a `ModelCheckpoint` callback to trainer argument `callback`, '


Suggested change

'You passed a `ModelCheckpoint` callback to trainer argument `callback`, '

'You passed a `ModelCheckpoint` callback to trainer argument `callback`,'

Borda · 2020-04-23T11:55:00Z

pytorch_lightning/trainer/callback_config.py

+
+            if isinstance(callback, EarlyStopping):
+                raise MisconfigurationException(
+                    'You passed a `EarlyStopping` callback to trainer argument `callback`, '


Suggested change

'You passed a `EarlyStopping` callback to trainer argument `callback`, '

'You passed a `EarlyStopping` callback to trainer argument `callback`,'

Borda · 2020-04-23T11:56:57Z

pytorch_lightning/trainer/callback_config.py

@@ -95,3 +96,15 @@ def configure_early_stopping(self, early_stop_callback):
        else:
            self.early_stop_callback = early_stop_callback
            self.enable_early_stop = True
+
+    def check_callback_config(self):
+        for callback in self.callbacks:


what about rather just filter and drop the wrong one meaning having [CB, CM, ES, CB] returns [CB, CB] and raise warnings that [CM, ES] was removed...

Borda · 2020-04-23T11:59:40Z

i think we want to standardize towards having all of the callbacks passed into the callbacks argument as a list (and remove cases where the Trainer is directly invoking callbacks). the checkpoint_callback and early_stopping_callback args should change to only accept bool where we can construct the default callback object and insert it into the callbacks list.

I agree about the standardize callbacks stack, but I would rather have in some protected variable and here (user input) keep filtering of callbacks only...
cc: @PyTorchLightning/core-contributors

williamFalcon · 2020-04-23T12:04:38Z

the pattern should be:
=True enable the default callback
=False disable the callback

pass in the actual callback to modify the behavior to the callback arg OR the list of callbacks. otherwise you end up having to have 2 arguments to turn on a callback.

=True, callbacks=[...]

instead of the easier
=Callback

mergify · 2020-04-23T15:52:07Z

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

jeremyjordan · 2020-04-24T01:38:53Z

@Borda can you elaborate?

I would rather have in some protected variable and here (user input) keep filtering of callbacks only...

@williamFalcon

pass in the actual callback to modify the behavior to the callback arg OR the list of callbacks.

if we allow users to pass in the callback object (eg. checkpoint_callback=Callback()) then we have to decide which order to insert the callbacks into the list. as much as we would desire that callback ordering should not matter, there are cases where it will. for example, if we have both early stopping and a model checkpoint callback, we'll want to have early stopping run before the model checkpoint (because the model checkpoint callback reaches into the early stopping to checkpoint it's state). i'm working on addressing that particular case here #1504 and want to avoid similar accidental regressions in the future.

Borda · 2020-04-24T10:02:15Z

@Borda can you elaborate?

I would rather have in some protected variable and here (user input) keep filtering of callbacks only...

I was thinking about having inter self._callback which would contain all callbacks-like objects including EarlyStop and CheckPoint... but maybe it does not have much/any benefit as I thought that this CallbacksCollection would be wrapped and called as LoggersCollection 🐰

if we allow users to pass in the callback object (eg. checkpoint_callback=Callback()) then we have to decide which order to insert the callbacks into the list. as much as we would desire that callback ordering should not matter, ...

Totally agree that this is not solved yet/well, the order may significantly affect the workflow... I have seen in other packages (like computing MultipleObjectTtracking metrics) introducing dependency so each callback has written what/if it depends on other callback... but I see that this can be nice for a user if we also provide dependency chart but a bit harder to implement...

cc: @PyTorchLightning/core-contributors thought?
maybe create a separate issue about callback architecture?

SkafteNicki · 2020-04-24T11:20:27Z

I think I will close this PR for now. It opened up a longer discussion that is worth taking, about rethinking some aspects of the callback interface. I have created issue #1592 for future discussion.

williamFalcon · 2020-04-24T12:32:35Z

@jeremyjordan
this is precisely why the early stopping and checkpoint callbacks are different. otherwise users WILL mess up the order and we’ll be inundated with GH issues.
Since those 2 are special callbacks they should remain separate IMHO.

for the callbacks list the user should know they should order callbacks.

Borda · 2020-04-24T12:40:53Z

@williamFalcon so you say that the executive order shall be:

callback if a standard type and they are in random order or order given in Trainer argument
earlystop
checkpoint

moi90 · 2020-10-27T06:33:06Z

I think this should be merged for now. It can be reverted if the discussion on callbacks came to a conclusion (which it didn't - it was closed by stale bot).

It is really annoying to learn only after one epoch that I passed the callbacks wrongly.

errors on wrong callback configuration

379f59e

mergify bot requested a review from a team April 21, 2020 11:26

Borda requested changes Apr 23, 2020

View reviewed changes

mergify bot requested a review from a team April 23, 2020 11:58

SkafteNicki mentioned this pull request Apr 24, 2020

[Discussion] Callback interface/architechture #1592

Closed

SkafteNicki closed this Apr 24, 2020

SkafteNicki deleted the bug/error_on_wrong_callback_passing branch May 25, 2020 08:18

Borda added the bug Something isn't working label May 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Error if callbacks are passed wrongly #1544

Error if callbacks are passed wrongly #1544

SkafteNicki commented Apr 21, 2020 •

edited

Loading

jeremyjordan commented Apr 23, 2020

SkafteNicki commented Apr 23, 2020

Borda Apr 23, 2020

Borda Apr 23, 2020

Borda Apr 23, 2020

Borda commented Apr 23, 2020

williamFalcon commented Apr 23, 2020 •

edited

Loading

mergify bot commented Apr 23, 2020

jeremyjordan commented Apr 24, 2020 •

edited

Loading

Borda commented Apr 24, 2020

SkafteNicki commented Apr 24, 2020

williamFalcon commented Apr 24, 2020 •

edited

Loading

Borda commented Apr 24, 2020

moi90 commented Oct 27, 2020

	'You passed a `ModelCheckpoint` callback to trainer argument `callback`, '
	'You passed a `ModelCheckpoint` callback to trainer argument `callback`,'

	'You passed a `EarlyStopping` callback to trainer argument `callback`, '
	'You passed a `EarlyStopping` callback to trainer argument `callback`,'

Error if callbacks are passed wrongly #1544

Error if callbacks are passed wrongly #1544

Conversation

SkafteNicki commented Apr 21, 2020 • edited Loading

Before submitting

What does this PR do?

PR review

Did you have fun?

jeremyjordan commented Apr 23, 2020

SkafteNicki commented Apr 23, 2020

Borda Apr 23, 2020

Choose a reason for hiding this comment

Borda Apr 23, 2020

Choose a reason for hiding this comment

Borda Apr 23, 2020

Choose a reason for hiding this comment

Borda commented Apr 23, 2020

williamFalcon commented Apr 23, 2020 • edited Loading

mergify bot commented Apr 23, 2020

jeremyjordan commented Apr 24, 2020 • edited Loading

Borda commented Apr 24, 2020

SkafteNicki commented Apr 24, 2020

williamFalcon commented Apr 24, 2020 • edited Loading

Borda commented Apr 24, 2020

moi90 commented Oct 27, 2020

SkafteNicki commented Apr 21, 2020 •

edited

Loading

williamFalcon commented Apr 23, 2020 •

edited

Loading

jeremyjordan commented Apr 24, 2020 •

edited

Loading

williamFalcon commented Apr 24, 2020 •

edited

Loading