Skip to content

Trainer / Core : Do not change init signature order#30126

Merged
younesbelkada merged 2 commits intomainfrom
younesbelkada-patch-1
Apr 8, 2024
Merged

Trainer / Core : Do not change init signature order#30126
younesbelkada merged 2 commits intomainfrom
younesbelkada-patch-1

Conversation

@younesbelkada
Copy link
Contributor

@younesbelkada younesbelkada commented Apr 8, 2024

What does this PR do?

Currently the TRL CI is broken on transformers main due to the addition of image_processors in the middle of trainer's init signature 🤯

On TRL (and probably other libs such as axolotl), we do subclass the trainer and call:

        super().__init__(
            model,
            args,
            data_collator,
            train_dataset,
            eval_dataset,
            tokenizer,
            model_init,
            compute_metrics,
            callbacks,
            optimizers,
            preprocess_logits_for_metrics,
        )

Without explicit pos arguments

#29896

To be on the safe zone I propose to simply put image_processors at the very end of trainer's init

cc @amyeroberts @NielsRogge

@amyeroberts
Copy link
Contributor

amyeroberts commented Apr 8, 2024

We can add this quick patch - which will be eventually be subsumed by #30102

So that we can make sure things don't break in the future - is there a reason the init is called with positional arguments rather than kwargs?

@younesbelkada
Copy link
Contributor Author

good point, I will also update that in TRL ! actually there is no strong reason to not use kwargs :/
I think other libs might possibly do the same thing as what we did in TRL

@younesbelkada younesbelkada merged commit a71def0 into main Apr 8, 2024
@younesbelkada younesbelkada deleted the younesbelkada-patch-1 branch April 8, 2024 14:57
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

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

Comments