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

General fixes for tool calling #2954

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

Conversation

Trofleb
Copy link

@Trofleb Trofleb commented Jan 24, 2025

General fixes for tool calling

Tool calling was incompatible with the openai api on both the stream and non stream implementation.

Fixes:

  • tool calling should return a string in arguments
  • tool calling streaming should return an array as tool_calls
  • tool calling streaming was not streaming the arguments in a readable way (more on that further down)

Small changes

  • Better differentiation in tool types
  • arguments/properies differentiation

Fixes the following issues I think:
#2461
#2480
#2864

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 or the forum? 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, and
    here are tips on formatting docstrings.
  • [~] Did you write any new necessary tests?

Details

Arguments are now strings, as per openai's API

Open AI - non stream

"tool_calls": [
  {
    "id": "call_7c9bnn08VbcmznYinyg5H9Do",
    "type": "function",
    "function": {
      "name": "get_weather",
      "arguments": "{\"location\":\"Zurich, France\"}"
    }
  }
],

Before - non stream

"tool_calls": [
  {
    "id": "0",
    "type": "function",
    "function": {
      "description": null,
      "name": "get_weather",
      "arguments": {
        "location": "Zurich, France"
      }
    }
  }
]

Now - non stream

"tool_calls": [
          {
            "id": "0",
            "type": "function",
            "function": {
              "name": "get_weather",
              "arguments": "{\"location\":\"Zurich, France\"}"
            }
          }
        ]

Streaming has a particular behaviour for tool calling, I did my best to conform to what the openai api was giving out.

Open AI - stream

{"role":"assistant","content":null,"tool_calls":[{"index":0,"id":"call_TCt8rccpnTreOYMxv16lefdg","type":"function","function":{"name":"get_weather","arguments":""}}],"refusal":null}
{"tool_calls":[{"index":0,"function":{"arguments":"{\""}}]}
{"tool_calls":[{"index":0,"function":{"arguments":"location"}}]}
{"tool_calls":[{"index":0,"function":{"arguments":"\":\""}}]}
{"tool_calls":[{"index":0,"function":{"arguments":"Zuri"}}]}
{"tool_calls":[{"index":0,"function":{"arguments":"ch"}}]}
{"tool_calls":[{"index":0,"function":{"arguments":","}}]}
{"tool_calls":[{"index":0,"function":{"arguments":" France"}}]}
{"tool_calls":[{"index":0,"function":{"arguments":"\"}"}}]}
{}

Before - stream

{"role":"assistant","tool_calls":{"index":0,"id":"","type":"function","function":{"name":null,"arguments":"{\""}}}
{"role":"assistant","tool_calls":{"index":0,"id":"","type":"function","function":{"name":null,"arguments":"function"}}}
{"role":"assistant","tool_calls":{"index":0,"id":"","type":"function","function":{"name":null,"arguments":"\":"}}}
{"role":"assistant","tool_calls":{"index":0,"id":"","type":"function","function":{"name":null,"arguments":" {\""}}}
{"role":"assistant","tool_calls":{"index":0,"id":"","type":"function","function":{"name":null,"arguments":"_name"}}}
{"role":"assistant","tool_calls":{"index":0,"id":"","type":"function","function":{"name":null,"arguments":"\":"}}}
{"role":"assistant","tool_calls":{"index":0,"id":"","type":"function","function":{"name":null,"arguments":" \""}}}
{"role":"assistant","tool_calls":{"index":0,"id":"","type":"function","function":{"name":null,"arguments":"get"}}}
{"role":"assistant","tool_calls":{"index":0,"id":"","type":"function","function":{"name":null,"arguments":"_weather"}}}
{"role":"assistant","tool_calls":{"index":0,"id":"","type":"function","function":{"name":null,"arguments":"\","}}}
{"role":"assistant","tool_calls":{"index":0,"id":"","type":"function","function":{"name":null,"arguments":" \""}}}
{"role":"assistant","tool_calls":{"index":0,"id":"","type":"function","function":{"name":null,"arguments":"location"}}}
{"role":"assistant","tool_calls":{"index":0,"id":"","type":"function","function":{"name":null,"arguments":"\":"}}}
{"role":"assistant","tool_calls":{"index":0,"id":"","type":"function","function":{"name":null,"arguments":" \""}}}
{"role":"assistant","tool_calls":{"index":0,"id":"","type":"function","function":{"name":null,"arguments":"Zur"}}}
{"role":"assistant","tool_calls":{"index":0,"id":"","type":"function","function":{"name":null,"arguments":"ich"}}}
{"role":"assistant","tool_calls":{"index":0,"id":"","type":"function","function":{"name":null,"arguments":","}}}
{"role":"assistant","tool_calls":{"index":0,"id":"","type":"function","function":{"name":null,"arguments":" France"}}}
{"role":"assistant","tool_calls":{"index":0,"id":"","type":"function","function":{"name":null,"arguments":"\"}}"}}}
{"role":"assistant","tool_calls":{"index":0,"id":"","type":"function","function":{"name":null,"arguments":"<|eot_id|>"}}}

Now - stream

{"role":"assistant","tool_calls":[{"index":0,"id":"","type":"function","function":{"name":"get_weather","arguments":"{"}}]}
{"role":"assistant","tool_calls":[{"index":0,"id":"","type":"function","function":{"name":null,"arguments":" \""}}]}
{"role":"assistant","tool_calls":[{"index":0,"id":"","type":"function","function":{"name":null,"arguments":"location"}}]}
{"role":"assistant","tool_calls":[{"index":0,"id":"","type":"function","function":{"name":null,"arguments":"\":"}}]}
{"role":"assistant","tool_calls":[{"index":0,"id":"","type":"function","function":{"name":null,"arguments":" \""}}]}
{"role":"assistant","tool_calls":[{"index":0,"id":"","type":"function","function":{"name":null,"arguments":"Zuri"}}]}
{"role":"assistant","tool_calls":[{"index":0,"id":"","type":"function","function":{"name":null,"arguments":"ch"}}]}
{"role":"assistant","tool_calls":[{"index":0,"id":"","type":"function","function":{"name":null,"arguments":","}}]}
{"role":"assistant","tool_calls":[{"index":0,"id":"","type":"function","function":{"name":null,"arguments":" France"}}]}
{"role":"assistant","tool_calls":[{"index":0,"id":"","type":"function","function":{"name":null,"arguments":"\"}"}}]}

Testing

The test I could run seem to still pass ( I don't have a premium hugging face account - yet ;). I adapted the tests that needed to be adapted but I'm not sure how to add test for streaming. I welcome any help I can get on that front.

I did some tests by doing the following:

  1. Ran a simple tool calling model on local
text-generation-launcher --model-id MadeAgents/Hammer2.1-0.5b --port 8080
  1. Tested tool calling without streaming using langchain_openai
  2. Tested tool calling with streaming using langchain_openai

Other PR

A PR was created before I could make mine. As I went bit further I still wanted to finish mine. Of course I'm open to closing mine, or group our changes.

Review

Not sure who to tag here.
FYI: I'm by no means a rust expert and welcomes criticism/improvements.

Necessary to keep compatibility with openai. The usage of tgi with openai compatible libraries for function calling was broken.
The streaming API for tool calling now starts when the name is parsed and then send arguments as token are generated and stops properly.
@Datta0
Copy link

Datta0 commented Jan 24, 2025

@Trofleb
Thanks for this.

Do you happen to have the response handy for the non streaming aka stream=False case? We noticed that TGI returns a JSON while OpenAI spec expects string...
It'd be great if you can add a sample response here for reference

@Trofleb
Copy link
Author

Trofleb commented Jan 24, 2025

@Datta0 We had the same issue, I added the samples for the non stream version to my initial message.

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