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

Tool fine-tuning support DPO #2479

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

August-murr
Copy link
Collaborator

@August-murr August-murr commented Dec 14, 2024

What does this PR do?

adding tool support for function calling models in the DPOTrainer

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? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines.
  • Did you write any new necessary tests?

Who can review?

@qgallouedec

@August-murr
Copy link
Collaborator Author

Before adding it to all the trainers, what do you think of the overall structure? Is it okay to include the tools in each trainer configuration?

@qgallouedec
Copy link
Member

Thanks for this addition!

Let's keep things as separate as possible, and keep this PR for DPO only.

The code as is looks good to me. The only question is: can this type (Optional[list[Union[dict, Callable]]]) being parsed. I'll try.

@qgallouedec
Copy link
Member

qgallouedec commented Dec 14, 2024

That's why I thought:

from trl import DPOConfig, TrlParser

parser = TrlParser((DPOConfig,))
parser.parse_args_and_config()
$ python 2479.py --output_dir out --tools "{'type': 'function', 'function': {'name': 'multiply', 'description': 'A function that multiplies two numbers', 'parameters': {'type': 'object', 'properties': {'a': {'type': 'number', 'description': 'The first number to multiply'}, 'b': {'type': 'number', 'description': 'The second number to multiply'}}, 'required': ['a', 'b']}}}"
[...]
2479.py: error: argument --tools: invalid Union value: "{'type': 'function', 'function': {'name': 'multiply', 'description': 'A function that multiplies two numbers', 'parameters': {'type': 'object', 'properties': {'a': {'type': 'number', 'description': 'The first number to multiply'}, 'b': {'type': 'number', 'description': 'The second number to multiply'}}, 'required': ['a', 'b']}}}"

I'm not sure what the best way to handle it right now, I'll sleep on it.

@August-murr
Copy link
Collaborator Author

Let's keep things as separate as possible, and keep this PR for DPO only.

a different PR for each trainer then?

can this type (Optional[list[Union[dict, Callable]]]) being parsed.

Adding tools to the CLI would be quite complicated. It wouldn't be practical to add all the tools into the CLI. My best guess is to read the functions from another source, like another script, if there’s a request for it later.

@August-murr August-murr changed the title Tool fine-tuning support Tool fine-tuning support DPO Dec 16, 2024
@August-murr
Copy link
Collaborator Author

does this need anything else? test or docs?

@August-murr August-murr self-assigned this Dec 25, 2024
@August-murr
Copy link
Collaborator Author

I also wanted to add it to SFTTrainer but it doesn't use maybe_apply_chat_template

@August-murr August-murr marked this pull request as ready for review December 29, 2024 12:17
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.

2 participants