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

[CLI] Extend training support to all trainers #2101

Open
lewtun opened this issue Sep 23, 2024 · 6 comments · May be fixed by #2157
Open

[CLI] Extend training support to all trainers #2101

lewtun opened this issue Sep 23, 2024 · 6 comments · May be fixed by #2157
Assignees
Labels
✨ enhancement New feature or request 👶 good first issue Good for newcomers

Comments

@lewtun
Copy link
Member

lewtun commented Sep 23, 2024

Feature request

The CLI currently supports training models with SFT/DPO/KTO:

SUPPORTED_COMMANDS = ["sft", "dpo", "chat", "kto"]

It would be good to extend this support to all trainers so that we have a consistent API and also learn which parts of our scripts need refactoring to support this usage.

This could be tackled in separate PRs to keep things lightweight, and I'll track here the trainers in terms of priority to add (based on Hub usage):

Motivation

It is somewhat annoying that one cannot train a model through the CLI as this is helpful for fast debugging / iterations.

Your contribution

Happy to open PRs, but this could be a good first issue for new contributors!

@lewtun lewtun added the 👶 good first issue Good for newcomers label Sep 23, 2024
@qgallouedec qgallouedec added the ✨ enhancement New feature or request label Sep 23, 2024
@qgallouedec
Copy link
Member

Duplicate #1811, closing it in favour of this one

@grumpyp
Copy link

grumpyp commented Sep 29, 2024

Hi, it looks like it'd be just the extension of the SUPPORTED_COMMANDS constant?

as for instance orpo would already be there https://github.com/huggingface/trl/blob/main/examples/scripts/orpo.py

I didn't check for the others but compared the scripts of kto with orpo for instance.

@lewtun
Copy link
Member Author

lewtun commented Sep 30, 2024

@grumpyp yes I think this is all one needs - a better solution would be to have an automated way to populate this constant by globbing all the _trainer.py files (e.g. make cli_commands). This way, anytime someone adds a new trainer, we automatically get support for it in the CLI :)

@grumpyp
Copy link

grumpyp commented Sep 30, 2024

@grumpyp yes I think this is all one needs - a better solution would be to have an automated way to populate this constant by globbing all the _trainer.py files (e.g. make cli_commands). This way, anytime someone adds a new trainer, we automatically get support for it in the CLI :)

yes definitely! If you want, assign the issue to me please. I'll try to get a PR out today.

@lewtun
Copy link
Member Author

lewtun commented Oct 1, 2024

assigned! thanks for the offer to help 🤗

@grumpyp grumpyp linked a pull request Oct 2, 2024 that will close this issue
5 tasks
@grumpyp
Copy link

grumpyp commented Oct 2, 2024

hi @lewtun

I didn't want to manipulate py-files via the Makefile so I went a slighly different approach.

It now creates the commands dynamically using a utility function which is cached.
EDIT: I deleted the caching as it's not executed anywhere else and terminated after running so the cache is not saved.

Let me know if that works for you or if it needs changes. Thanks for the opportunity to contribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement New feature or request 👶 good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants