PR 2634 CI - Fix the tool_choice format for named choice by adapting OpenAIs scheme #2645

drbh · 2024-10-14T12:30:40Z

This PR re opens #2634 to run CI

drbh · 2024-10-14T17:49:04Z

@linusbierhoff thank you for the contribution! 🙏 I've opened this PR to run CI, and add some other tool calling improvements along with your changes.

This PR now removes the old ToolType in favor of a more simple ChatCompletionToolChoiceOption. This new struct includes the addition of TypedChoice and correctly allows a user to specify the following tools as strings or as an object. Additionally this PR includes support for the required tool_choice, which forces the model to use one of the user provided tools. Lastly the new struct also improves the openapi spec and makes the tool choice a oneOf which improves the spec readability

Narsil · 2024-10-15T08:29:46Z

router/src/lib.rs

+
+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, ToSchema, Default)]
+#[serde(from = "ToolTypeDeserializer")]
+pub enum ChatCompletionToolChoiceOption {


Can we avoid java naming please ?

yea I can revert the name change if thats better.

The name change is to align with the naming that OpenAI is using in their spec. https://github.com/openai/openai-openapi/blob/d033c364c6574068ee89f3d5f845a4830bddd503/openapi.yaml#L9581, I aligned the names because I thought it may be helpful in automatically checking the spec, however it is a very long name..

I thought it may be helpful in automatically checking the spec

Then let's wait for it to be actually relevant, it's not usually a good thing to do things "because we might need it". Either you need it or you don't.

Personally I don't think it has any relevance to use their names.

got it that make sense, I've changed the name to ToolChoice in the latest commit. (its much cleaner 👍)

Narsil · 2024-10-15T08:30:19Z

router/src/lib.rs

    NoTool,
+    /// Means the model must call one or more tools.
+    #[schema(rename = "required")]


schema rename is not what you want, you want serde rename most likely.

oh I actually included it to improve the generated schema

this macro changes the enum on the schema to show the expected lowercase text instead of Required

{ "type": "string", "description": "Means the model must call one or more tools.", "enum": [ "required" ] },

Yes, and serde(renamed) actually modifies what the input is from the user.

Actually a better serde(rename) on the whole struct should fix every field (both schema and actual JSON parsing)

oh yea thats much more concise 🙏, updated in the latest commit

Narsil · 2024-10-15T08:31:24Z

router/src/lib.rs

    /// Forces the model to call a specific tool.
    #[schema(rename = "function")]
+    #[serde(alias = "function")]


Can we have link in rustdoc to where all these attributes are explained, possibly in the original openai spec (or anthropic)

yep, just added a useful link above the enum that explains all 4 of the options in OpenAI's docs this link will also be included in the openapi schema for the ChatCompletionToolChoiceOption description

Narsil · 2024-10-15T08:33:43Z

router/src/lib.rs

+
+        let named = r#"{"tool_choice":"myfn"}"#;
+        let de_named: TestRequest = serde_json::from_str(named).unwrap();
+        assert_eq!(de_named.tool_choice, ref_choice);


Suggested change

assert_eq!(de_named.tool_choice, ref_choice);

assert_eq!(de_named.tool_choice,ChatCompletionToolChoiceOption::Function(FunctionName {

name: "myfn".to_string(),

}) );

Please avoid indirection in tests. Tests are about being as dumb as possible, so a reader can easily assess if the test is correct, or outdated. Every indirection makes that harder to reason about.

great points, indirection removed in the latest commit

Narsil · 2024-10-15T08:36:15Z

router/src/infer/tool_grammar.rs

-        tools.push(no_tool);
+        // add the no_tool function to the tools as long as we are not required to use a specific tool
+        if tool_choice != ChatCompletionToolChoiceOption::Required {
+            let no_tool = Tool {


Isn't this only ever used in the auto branch ? Wouldn't putting it in the match branch make more sense ?

yep good point. Just moved into the match branch and removed the let mut tools and only mutate tools if the Auto option is chosen. thanks

Narsil · 2024-10-15T08:37:50Z

router/src/infer/tool_grammar.rs

-            ToolType::OneOf => tools.clone(),
-            ToolType::NoTool => return Ok((tools, None)),
+            ChatCompletionToolChoiceOption::Required => tools.clone(),
+            ChatCompletionToolChoiceOption::Auto => tools.clone(),


Why is this .clone needed ? I know it was there before, but since we're upgrading this, couldn't we get rid of it ?

yes agreed they should be removed. Just removed all of the tool clones above and improved the match in general

Narsil · 2024-10-15T08:39:03Z

router/src/infer/tool_grammar.rs

-            ToolType::NoTool => return Ok((tools, None)),
+            ChatCompletionToolChoiceOption::Required => tools.clone(),
+            ChatCompletionToolChoiceOption::Auto => tools.clone(),
+            ChatCompletionToolChoiceOption::NoTool => return Ok((tools, None)),


Also I think early returns in match branches are bug nests, it's easy to overlook those branches, returning an empty slice and letting the rest roll seems slightly better (code reading wise)

good point, updated in the latest to commit to discard the tools and continue executing the function normally

Narsil · 2024-10-15T08:39:50Z

router/src/lib.rs

-    #[schema(nullable = true, example = "null")]
-    pub tool_choice: ToolChoice,
+    #[schema(nullable = true, default = "null", example = "null")]
+    pub tool_choice: Option<ChatCompletionToolChoiceOption>,


Why the change to Option you have the NoTool choice, there shouldn't be a need to Option wrapping is there ?

Option is needed so the user can send a http request without a tool_choice param included. If tool_choice is None (user did not include a tool_choice in the request), ChatCompletionToolChoiceOption defaults to NoTool

Doesn't a default take care of that ? And shouldn't the default be "auto"?

Narsil · 2024-10-15T08:40:01Z

router/src/lib.rs

@@ -906,8 +906,18 @@ impl ChatRequest {
        let (inputs, grammar, using_tools) = prepare_chat_input(
            infer,
            response_format,
-            tools,
-            tool_choice,
+            tools.clone(),


removed, it was previously required because of a check of the tools vec that was below this line (moved it up and removed the clone). thanks

Narsil · 2024-10-15T08:41:05Z

router/src/lib.rs

-            tool_choice,
+            tools.clone(),
+            // unwrap or default (use "auto" if tools are present, and "none" if not)
+            tool_choice.map_or_else(


That should be already done in the deserialization, there are about 3 different function calls to preprare_chat_input.

good point, in the latest commit i've moved the logic from prepare_chat_input into try_into_generate and remove the prepare_chat_input all together.

Narsil · 2024-10-15T08:43:49Z

required is that an OpenAI spec ?

Can you point to the original references for the things we're adding.
Also dropping the old code is a pretty bad breaking change, it should be in big bold red !.

Why are we dropping support for the old way, if it was there, is it documented anywhere ? Should we update the old doc ?

This PR is also seriously lacking integration tests for all of these. The deserialization tests are nice, but not enough.

drbh · 2024-10-15T14:04:56Z

required is that an OpenAI spec ?

yea required is supported by OpenAI, see docs here: https://platform.openai.com/docs/guides/function-calling/configuring-function-calling-behavior-using-the-tool_choice-parameter. Note** OpenAI added this parameter after TGI's initial tool implementation (hence why it was missing), additionally prior to #2614 auto and required had the same functionality in TGI, however now its more pressing/great to have the api's aligned.

Also dropping the old code is a pretty bad breaking change, it should be in big bold red !.

yea regarding the removed code - as the original PR noted, the function tool choice was expecting the incorrect format. I believe I confused some of the function calling formats and ended up adding one that was not aligned with any major LLM api.

regarding flagging the breaking changes, would it be best to include the old API but throw a helpful error if it is used? Currently I've removed the path in favor of the corrected one.

Why are we dropping support for the old way, if it was there, is it documented anywhere ? Should we update the old doc ?

we're dropping support since it was incorrectly implemented, we do not have documentation specific to this tool choice, however I'm going to push another commit with updated docs for the supported tool choice types

This PR is also seriously lacking integration tests for all of these. The deserialization tests are nice, but not enough.

just added a test for required and one for a function object in test_tools_llama.py

HuggingFaceDocBuilderDev · 2024-10-15T17:14:26Z

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.

Narsil · 2024-10-16T10:13:03Z

router/src/lib.rs

+                ChatCompletionToolChoiceOption::Auto
+            } else {
+                ChatCompletionToolChoiceOption::NoTool
+            }


Shoudln't Auto be enough in all cases ?

What happens if tool_choice is "required" and no tools are sent ?

("auto", tools = []) we should just ignore the grammar right ?

great point! ToolChoice::Auto should be enough and in the case there are no tools we'll ignore the grammar/tool logic. updated in the latest commit

Narsil · 2024-10-16T10:16:37Z

integration-tests/models/test_tools_llama.py

+            "tools": tools,
+            "tool_choice": {
+                "type": "function",
+                "function": {"name": "get_current_weather"},


Wouldn't choosing the other tools be a better test ? (Forcing to check that the other tool was used that in the required test case ?)

agreed, just updated to choose get_n_day_weather_forecast instead

Narsil · 2024-10-16T10:16:54Z

integration-tests/models/test_tools_llama.py

+                last_response = response
+
+    assert count == 30
+    print(tool_calls_generated)


oops 😬, removed in latest commit. thanks!

Narsil · 2024-10-17T13:07:12Z

router/src/infer/tool_grammar.rs

        if tools.is_empty() {
-            return Ok((tools, None));
+            return Ok((Vec::with_capacity(0), None));


updated in latest commit

I don't see it.

let tools_to_use = match tool_choice was updated to return a vec![] rather than a Vec::with_capacity(0), and the conditional was moved below tools_to_use to return none in both cases (no tools supplied, or if NoTools specified)

text-generation-inference/router/src/infer/tool_grammar.rs

Line 55 in 16e4c38

ToolChoice::NoTool => vec![],

Narsil · 2024-10-17T13:07:49Z

router/src/lib.rs

@@ -850,13 +850,13 @@ pub(crate) struct ChatRequest {
    /// A specific tool to use. If not provided, the model will default to use any of the tools provided in the tools parameter.
    #[serde(default)]
    #[schema(nullable = true, default = "null", example = "null")]
-    pub tool_choice: Option<ChatCompletionToolChoiceOption>,
+    pub tool_choice: Option<ToolChoice>,


Shouldn't default "auto" allow removing the Option here ?

oh yes that worked, I was originally under the impression the Option was still needed to handle all http request but this is much cleaner. Thank you

Narsil · 2024-10-17T13:08:05Z

router/src/lib.rs


    /// Response format constraints for the generation.
    ///
    /// NOTE: A request can use `response_format` OR `tools` but not both.
    #[serde(default)]
-    #[schema(nullable = true, default = "null", example = "null")]
+    #[schema(nullable = true, default = "auto", example = "auto")]


I think you made the change to the wrong item, no ?

yes thank you, updated the correct argument in the latest commit

Narsil · 2024-10-17T13:09:34Z

router/src/lib.rs

    Auto,
    /// Means the model will not call any tool and instead generates a message.
    #[schema(rename = "none")]
-    #[default]
    NoTool,


Isn't this no_tool with snake_case ?This should mean a rename of this property or None, no ?

I don't think schema(rename) imples a serde rename.

good catch, updated to serde(rename) and added a test to ensure that the "none" values is respected correctly

…pport and refactor

…control flow

drbh · 2024-11-19T18:21:49Z

merging optimistically as CI is green and issues have been resolved. Also mentioned in the internal #text-generation-inference Slack channel.

Will watch for any regressions and revert if needed

drbh force-pushed the pr-2634-ci-branch branch from 37a1239 to 970d8dc Compare October 14, 2024 19:08

Narsil requested changes Oct 15, 2024

View reviewed changes

drbh force-pushed the pr-2634-ci-branch branch from e1d1706 to 79a67aa Compare October 15, 2024 16:36

Narsil reviewed Oct 16, 2024

View reviewed changes

Narsil reviewed Oct 17, 2024

View reviewed changes

mottoslo mentioned this pull request Oct 25, 2024

tgi server :: tool_choice="auto" behaves like tool_choice="required" from OpenAI spec #2549

Open

4 tasks

linusbierhoff and others added 13 commits October 29, 2024 09:32

add OpenAI like tool_choice for named choice

f979ff1

add tests

151f950

fix: run linter and bump api docs

2c172a2

fix: consolidate changes and remove old tool type

209f841

feat: improve, simplify and rename tool choice struct add required su…

f53c805

…pport and refactor

fix: simplify tool choice logic, improve tests, openapi and rust docs

b2db107

fix: refactor away prepare_chat_input and improve tool grammar apply …

daa1c62

…control flow

feat: update docs and add tool choice configuration section

dd759e7

fix: simplify naming, tool choice default and improve test

b5bf5b3

fix: adjust tool choice none logic, add test and small refactors

4075317

fix: add missing snapshot file

1ce1cf2

fix: adjust tool choice type in test

905d503

fix: adjust default when json tool choice is

c1eab6c

drbh force-pushed the pr-2634-ci-branch branch from 193ad66 to c1eab6c Compare October 29, 2024 13:34

fix: remove trailing space lint after rebase

daf7d97

fix: remove mostly mocked unit test

f9f34a5

drbh requested a review from Narsil October 30, 2024 14:23

drbh merged commit 5489406 into main Nov 19, 2024
12 of 13 checks passed

drbh deleted the pr-2634-ci-branch branch November 19, 2024 18:32

drbh mentioned this pull request Nov 21, 2024

Fix the tool_choice format for named choice by adapting OpenAIs scheme #2634

Closed

-        assert_eq!(de_named.tool_choice, ref_choice);
+        assert_eq!(de_named.tool_choice,ChatCompletionToolChoiceOption::Function(FunctionName {
+            name: "myfn".to_string(),
+        }) );

PR 2634 CI - Fix the tool_choice format for named choice by adapting OpenAIs scheme #2645

PR 2634 CI - Fix the tool_choice format for named choice by adapting OpenAIs scheme #2645

Conversation

drbh commented Oct 14, 2024

drbh commented Oct 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drbh Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narsil commented Oct 15, 2024

drbh commented Oct 15, 2024

HuggingFaceDocBuilderDev commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drbh commented Nov 19, 2024

drbh Oct 16, 2024 •

edited

Loading