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

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

Merged
merged 2 commits into from
Apr 8, 2024

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
Collaborator

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
21 checks passed
@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