Skip to content

Fix additional callbacks #199

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

Conversation

VassilisVassiliadis
Copy link
Contributor

Description of the change

Fixes a small bug in the method of train() sft_trainer.py where trainer_callbacks would append an array of user provided callbacks instead of concatenating the 2 arrays.

Without this change you get this exception:

self = <transformers.trainer_callback.CallbackHandler object at 0x32b558fa0>
event = 'on_init_end'
args = TrainingArguments(output_dir='/var/folders/p_/lhp3_gn503l7djn80tdzkf3h0000gn/T/tmpu2qek8_i', overwrite_output_dir=Fals...t_modules=None, batch_eval_metrics=False, cache_dir=None, max_seq_length=4096, packing=False, trackers=['file_logger'])
state = TrainerState(epoch=None, global_step=0, max_steps=0, logging_steps=500, eval_steps=500, save_steps=500, train_batch_si..., 'should_epoch_stop': False, 'should_save': False, 'should_evaluate': False, 'should_log': False}, 'attributes': {}}})
control = TrainerControl(should_training_stop=False, should_epoch_stop=False, should_save=False, should_evaluate=False, should_log=False)
kwargs = {}
callback = [<transformers.trainer_callback.TrainerCallback object at 0x30f3508e0>]
result = None

    def call_event(self, event, args, state, control, **kwargs):
        for callback in self.callbacks:
>           result = getattr(callback, event)(
                args,
                state,
                control,
                model=self.model,
                tokenizer=self.tokenizer,
                optimizer=self.optimizer,
                lr_scheduler=self.lr_scheduler,
                train_dataloader=self.train_dataloader,
                eval_dataloader=self.eval_dataloader,
                **kwargs,
            )
E           AttributeError: 'list' object has no attribute 'on_init_end'

../venv/lib/python3.10/site-packages/transformers/trainer_callback.py:498: AttributeError

Related issue number

Contributes to #142

How to verify the PR

Invoke the train() method with an additional_callbacks parameter that's an array containing at least 1 callback.

Was the PR tested

  • I have added >=1 unit test(s) for every new method I have added.
  • I have ensured all unit tests pass

@michael-johnston
Copy link

@Ssukriti @anhuong This fix would really help the benchmarking effort as it removes the need for us to maintain a fork to work around it (and hence falling out of synch). Any help in getting it merged appreciated. :-)

@dushyantbehl
Copy link
Collaborator

LGTM. Thanks @VassilisVassiliadis I missed this check in my unit testing PR.

@VassilisVassiliadis
Copy link
Contributor Author

No problem!

…t_trainer:train()

Signed-off-by: Vassilis Vassiliadis <vassilis.vassiliadis@ibm.com>
Signed-off-by: Vassilis Vassiliadis <vassilis.vassiliadis@ibm.com>
Signed-off-by: Vassilis Vassiliadis <vassilis.vassiliadis@ibm.com>
@VassilisVassiliadis VassilisVassiliadis force-pushed the fix_additional_callbacks branch from 4d414d1 to 8905904 Compare June 25, 2024 13:53
@michael-johnston
Copy link

@anhuong @Ssukriti @alex-jw-brooks Just a bump on this as we'd like to get rid of the need for a fork ASAP and this is just a single line change :-)

Copy link
Collaborator

@anhuong anhuong left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in reviewing this PR, appreciate catching the bug and creating a unit test that covers the bug.

@anhuong anhuong merged commit 8ddd68c into foundation-model-stack:main Jun 25, 2024
7 checks passed
@VassilisVassiliadis VassilisVassiliadis deleted the fix_additional_callbacks branch June 25, 2024 15:29
@VassilisVassiliadis
Copy link
Contributor Author

no worries!

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.

4 participants