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.

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

Fix a bug for CallbackHandler.callback_list #8052

Merged
merged 3 commits into from
Oct 27, 2020

Conversation

harupy
Copy link
Contributor

@harupy harupy commented Oct 26, 2020

What does this PR do?

Fix a bug where CallbackHandler.callback_list fails when given callbacks are duplicated:

---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-40-9605b122f4d1> in <module>()
      2 from transformers.trainer import DEFAULT_CALLBACKS
      3 
----> 4 CallbackHandler(DEFAULT_CALLBACKS + [MLflowCallback], "model", "optimizer", "lr_scheduler")

2 frames
/usr/local/lib/python3.6/dist-packages/transformers/trainer_callback.py in __init__(self, callbacks, model, optimizer, lr_scheduler)
    277         self.callbacks = []
    278         for cb in callbacks:
--> 279             self.add_callback(cb)
    280         self.model = model
    281         self.optimizer = optimizer

/usr/local/lib/python3.6/dist-packages/transformers/trainer_callback.py in add_callback(self, callback)
    299                 f"You are adding a {cb_class} to the callbacks of this Trainer, but there is already one. The current"
    300                 + "list of callbacks is\n:"
--> 301                 + self.callback_list
    302             )
    303         self.callbacks.append(cb)

/usr/local/lib/python3.6/dist-packages/transformers/trainer_callback.py in callback_list(self)
    326     @property
    327     def callback_list(self):
--> 328         return "\n".join(self.callbacks)
    329 
    330     def on_init_end(self, args: TrainingArguments, state: TrainerState, control: TrainerControl):

TypeError: sequence item 0: expected str instance, DefaultFlowCallback found

Code to reproduce the bug:

from transformers.trainer_callback import CallbackHandler
from transformers.trainer import DEFAULT_CALLBACKS

CallbackHandler(DEFAULT_CALLBACKS + [MLflowCallback], "model", "optimizer", "lr_scheduler")

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to the it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

@sgugger

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors which may be interested in your PR.

@harupy
Copy link
Contributor Author

harupy commented Oct 26, 2020

Do I need to add a test in tests/test_trainer_callback.py verifying that instantiating a trainer with duplicated callbacks doesn't fail?

# this should not fail
trainer = self.get_trainer(
    callbacks=[MyTestTrainerCallback, MyTestTrainerCallback],
)

@LysandreJik LysandreJik requested a review from sgugger October 26, 2020 17:17
Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Looking good, thanks for fixing! Any test you want to add is more than welcome as this new API is not super well tested yet.

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
@harupy
Copy link
Contributor Author

harupy commented Oct 27, 2020

@sgugger Thanks for the approval. I just added a test that verifies the following:

  1. Trainer can be instantiated with duplicated callacks.
  2. A warning is emitted for duplicated callbacks.

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
trainer = self.get_trainer(
callbacks=[MyTestTrainerCallback, MyTestTrainerCallback],
)
assert str(MyTestTrainerCallback) in warn_mock.call_args[0][0]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The warning message emitted here looks like:

You are adding a <class 'tests.test_trainer_callback.MyTestTrainerCallback'> to
the callbacks of this Trainer, but there is already one. The currentlist of callbacks is
:DefaultFlowCallback
TensorBoardCallback
MyTestTrainerCallback

Copy link
Contributor Author

@harupy harupy Oct 27, 2020

Choose a reason for hiding this comment

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

call_args[0][0] corresponds to the first positional argument of logger.warn

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good!

@harupy harupy requested a review from sgugger October 27, 2020 12:24
@sgugger sgugger requested a review from LysandreJik October 27, 2020 13:16
@LysandreJik LysandreJik merged commit 7bff0af into huggingface:master Oct 27, 2020
@harupy harupy deleted the fix-callback_list branch October 27, 2020 14:47
fabiocapsouza pushed a commit to fabiocapsouza/transformers that referenced this pull request Nov 15, 2020
* Fix callback_list

* Add test

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

* Fix test

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
fabiocapsouza added a commit to fabiocapsouza/transformers that referenced this pull request Nov 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants