[Frontend] Generate valid tool call IDs when using `tokenizer-mode=mistral` #12332

rafvasq · 2025-01-22T22:31:05Z

This PR fixes two cases when using tokenizer-mode=mistral to do with tool call IDs incompatible with Mistral.

When a request includes the generated tool call message with an id that isn't length 9, results in the error

mistral_common.exceptions.InvalidFunctionCallException: Tool call id was chatcmpl-tool-e5add885dbb342de950be95dd89b71e7 but must be a-z, A-Z, 0-9, with a length of 9.

When a request is sent with tool_choice set to request a specific function, the request returns an invalid tool_id:

"tool_calls": [{
  "id": "chatcmpl-tool-64aa8ec82efa4007b5fbf1ea885dea00",
  "type": "function",
  "function": {
    "name": "get_current_weather",
    "arguments": "{ \"city\": \"Dallas\", \"state\": \"TX\", \"unit\": \"celsius\" }"
  }
}]

This PR introduces

Handling invalid tool_ids by truncating and validating them to the required 9 characters.
Generating a valid 9-character tool_id when tool_choice is set when using a Mistral model
Specify comments about mistral's ID requirement for exactly 9 characters.

Signed-off-by: Rafael Vasquez <[email protected]>

github-actions · 2025-01-22T22:32:02Z

👋 Hi! Thank you for contributing to the vLLM project.
Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can do one of these:

Add ready label to the PR
Enable auto-merge.

🚀

tjohnson31415

Thanks for looking at this @rafvasq! I know it is still a WIP, but I left some suggestions on the current state of the PR.

tjohnson31415 · 2025-01-23T15:44:04Z

vllm/utils.py

@@ -2206,3 +2211,8 @@ def run_method(obj: Any, method: Union[str, bytes, Callable], args: Tuple[Any],
    else:
        func = partial(method, obj)  # type: ignore
    return func(*args, **kwargs)
+
+def generate_valid_mistral_tool_id():
+    # Mistral Tool Call Ids must be alphanumeric with a maximum length of 9.


The mistral requirement is exactly 9 characters.

Suggested change

# Mistral Tool Call Ids must be alphanumeric with a maximum length of 9.

# Mistral Tool Call Ids must be alphanumeric with a length of 9.

Fixed this comment in a couple of places.

tjohnson31415 · 2025-01-23T18:06:48Z

vllm/entrypoints/openai/serving_chat.py

+            if isinstance(tokenizer, MistralTokenizer):
+                for tool_call in message.tool_calls:
+                    tool_call.id = generate_valid_mistral_tool_id()
+                    logger.warning(f"Assigned new tool_id: {tool_call.id} for tool: {tool_call}")


In the tool_choice: auto case, the MistralTokenizer should be generating valid tool ids itself and not need this to override it. So I think this logic can be moved to the branch of the condition above that checks for type(request.tool_choice) is ChatCompletionNamedToolChoiceParam since that is the case where a tool_id is generated that does not meet the conditions for Mistral.

tjohnson31415 · 2025-01-23T18:09:57Z

vllm/transformers_utils/tokenizers/mistral.py

@@ -61,6 +61,8 @@ def maybe_serialize_tool_calls(request: ChatCompletionRequest):
            while True:
                try:
                    tool_call = next(tool_calls_validator)  # type: ignore
+                    tool_call['id'] = generate_valid_mistral_tool_id()
+                    logger.warning(f"Assigned new tool_id: {tool_call['id']} for tool: {tool_call}")


It looks like this change will modify the tool_id of every tool_call message in the incoming request with a new random id such that the id will change with each subsequent step in the conversation. I don't think we want that to happen.

Instead, tool_ids that are valid can be passed-through as-is and tool_ids that are not valid for mistral should have a consistent mapping to one that is valid.

Makes sense to me. I added a check here (using mistral common's validation) so that the id will only be generated/changed if it's invalid.

Signed-off-by: Rafael Vasquez <[email protected]>

tjohnson31415 · 2025-01-29T17:39:05Z

vllm/utils.py

+def generate_valid_mistral_tool_id():
+    # Mistral Tool Call Ids must be alphanumeric with a length of 9.
+    # https://github.com/mistralai/mistral-common/blob/21ee9f6cee3441e9bb1e6ed2d10173f90bd9b94b/src/mistral_common/protocol/instruct/validator.py#L299
+    return "".join(choices(ALPHANUMERIC, k=9))


Ah, we can just use the static function MistralToolCall.generate_random_id() instead of writing a new function.

tjohnson31415 · 2025-01-29T17:41:40Z

vllm/entrypoints/openai/serving_chat.py

@@ -668,6 +669,10 @@ async def chat_completion_full_generator(
                            arguments=output.text))
                    ])

+                if isinstance(tokenizer, MistralTokenizer):
+                    for tool_call in message.tool_calls:
+                        tool_call.id = generate_valid_mistral_tool_id()


I just noticed that there is a MistralToolCall class that overrides the id generation. It would be a bit cleaner to just use that class, eg. could make this change above:

tool_call_class = MistralToolCall if isinstance(tokenizer, MistralTokenizer) else ToolCall message = ChatMessage( role=role, content="", tool_calls=[ tool_call_class(function=FunctionCall( name=request.tool_choice.function.name, arguments=output.text)) ])

tjohnson31415 · 2025-01-29T17:55:35Z

vllm/transformers_utils/tokenizers/mistral.py

@@ -62,6 +62,8 @@ def maybe_serialize_tool_calls(request: ChatCompletionRequest):
                try:
                    tool_call = next(tool_calls_validator)  # type: ignore
                    validated_tool_calls.append(tool_call)
+                    if not re.match(r"^[a-zA-Z0-9]{9}$", tool_call['id']):
+                        tool_call['id'] = generate_valid_mistral_tool_id()


This will update the tool_call entries id to a valid id, but if the chat history also includes a tool response message with an id, this code will not adjust it, eg.

"messages":[ { "role": "user", "content": "What is the weather in Dallas Texas?" }, { "role": "assistant", "content": "", "tool_calls": [ { "id": "chatcmpl-asdf", "type": "function", "function": { "name": "get_current_weather", "arguments": "{ \"city\": \"Dallas\", \"state\": \"TX\", \"unit\": \"celsius\" }" } } ] }, { "role": "tool", "tool_call_id": "chatcmpl-asdf", "name": "get_current_weather", "content": "90 degrees, partly cloudy" } ],

Also, we'll need to make sure that the id is mapped to the same corrected id in both places. In the example chat template, the ids are truncated to the last 9 characters. For consistency, I think we should do the same when using the MistralTokenizer. Note that, then, a tool_id with less than 9 characters should still be rejected.

Signed-off-by: Rafael Vasquez <[email protected]>

rafvasq · 2025-01-30T15:29:38Z

Thanks for the guidance @tjohnson31415, I made another attempt at it.

I'm truncating IDs (if len > 9) and checking that they're mistral-valid, raising an error if it still isn't. Truncating is the only adjustment, I didn't know whether to go as far as dealing with non-alphanumeric chars too (e.g. chatcmpl-asdf truncates to cmpl-asdf but it's not alphanumeric and still invalid) so it'll get rejected by the validation step.

Signed-off-by: Rafael Vasquez <[email protected]>

tjohnson31415

The maybe_serialize_tool_calls function is meant to work around an issue with how the request is validated by Pydantic; it also has a TODO to be removed after it is fixed in Pydantic. Because of that, I'm thinking that the truncation of the tool call ids should be moved to its own function.

tjohnson31415 · 2025-02-04T20:02:02Z

vllm/transformers_utils/tokenizers/mistral.py

+                    if not re.match(r"^[a-zA-Z0-9]{9}$", tool_call["id"]):
+                        raise ValueError(
+                            "Invalid tool_call ID: %s",
+                            "(must be exactly 9 alphanumeric characters)",
+                            tool_call["id"],
+                        )


Validation of the tool ids is also done in Mistral Common. We don't need to duplicate the check here.

Removed the validation checks.

Signed-off-by: Rafael Vasquez <[email protected]>

mergify · 2025-02-04T22:15:41Z

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @rafvasq.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

Signed-off-by: Rafael Vasquez <[email protected]>

… into fix-mistral-tool-call

Signed-off-by: Rafael Vasquez <[email protected]>

Adds valid tool id generation

fe57236

Signed-off-by: Rafael Vasquez <[email protected]>

mergify bot added the frontend label Jan 22, 2025

tjohnson31415 reviewed Jan 23, 2025

View reviewed changes

rafvasq added 3 commits January 24, 2025 09:52

Fixes mistral id wording, adjusts logic, adds check for id

d63a884

Signed-off-by: Rafael Vasquez <[email protected]>

Move imports

8125f31

Signed-off-by: Rafael Vasquez <[email protected]>

Lints

c491a17

Signed-off-by: Rafael Vasquez <[email protected]>

rafvasq marked this pull request as ready for review January 24, 2025 20:02

rafvasq requested a review from tjohnson31415 January 24, 2025 20:02

Merge branch 'main' into fix-mistral-tool-call

4968fa3

tjohnson31415 reviewed Jan 29, 2025

View reviewed changes

rafvasq added 2 commits January 29, 2025 22:15

Reuse MistralToolParser, add truncating ID logic and checks

588f0cc

Signed-off-by: Rafael Vasquez <[email protected]>

Remove logging

5c41728

Signed-off-by: Rafael Vasquez <[email protected]>

rafvasq requested a review from tjohnson31415 January 30, 2025 03:25

rafvasq added 2 commits February 4, 2025 11:38

Fix assignments, valuerror

7a91290

Signed-off-by: Rafael Vasquez <[email protected]>

Modify valueerror msg

2d25e3a

Signed-off-by: Rafael Vasquez <[email protected]>

tjohnson31415 reviewed Feb 4, 2025

View reviewed changes

Refactor tool call id handling, remove validation

803acff

Signed-off-by: Rafael Vasquez <[email protected]>

mergify bot added the needs-rebase label Feb 4, 2025

Merge branch 'main' into fix-mistral-tool-call

d0d2118

mergify bot removed the needs-rebase label Feb 4, 2025

rafvasq added 5 commits February 4, 2025 17:18

Fix docstring quotes

b61a391

Signed-off-by: Rafael Vasquez <[email protected]>

Fix docstring

133be5e

Signed-off-by: Rafael Vasquez <[email protected]>

Merge branch 'fix-mistral-tool-call' of https://github.com/rafvasq/vllm…

97439f0

… into fix-mistral-tool-call

Remove whitespace

1119c83

Signed-off-by: Rafael Vasquez <[email protected]>

Fix arg

a62b036

Signed-off-by: Rafael Vasquez <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Frontend] Generate valid tool call IDs when using `tokenizer-mode=mistral` #12332

[Frontend] Generate valid tool call IDs when using `tokenizer-mode=mistral` #12332

rafvasq commented Jan 22, 2025 •

edited by github-actions bot

Loading

github-actions bot commented Jan 22, 2025

tjohnson31415 left a comment

tjohnson31415 Jan 23, 2025

rafvasq Jan 24, 2025

tjohnson31415 Jan 23, 2025

tjohnson31415 Jan 23, 2025

rafvasq Jan 24, 2025

tjohnson31415 Jan 29, 2025

tjohnson31415 Jan 29, 2025

tjohnson31415 Jan 29, 2025

rafvasq commented Jan 30, 2025

tjohnson31415 left a comment

tjohnson31415 Feb 4, 2025

rafvasq Feb 4, 2025

mergify bot commented Feb 4, 2025

	# Mistral Tool Call Ids must be alphanumeric with a maximum length of 9.
	# Mistral Tool Call Ids must be alphanumeric with a length of 9.

[Frontend] Generate valid tool call IDs when using tokenizer-mode=mistral #12332

Are you sure you want to change the base?

[Frontend] Generate valid tool call IDs when using tokenizer-mode=mistral #12332

Conversation

rafvasq commented Jan 22, 2025 • edited by github-actions bot Loading

github-actions bot commented Jan 22, 2025

tjohnson31415 left a 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

rafvasq commented Jan 30, 2025

tjohnson31415 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Feb 4, 2025

[Frontend] Generate valid tool call IDs when using `tokenizer-mode=mistral` #12332

[Frontend] Generate valid tool call IDs when using `tokenizer-mode=mistral` #12332

rafvasq commented Jan 22, 2025 •

edited by github-actions bot

Loading