vllm icon indicating copy to clipboard operation
vllm copied to clipboard

[Frontend][Feature] support tool calling for internlm/internlm2_5-7b-chat model

Open sydnash opened this issue 1 year ago • 24 comments

  1. support tool calling for internlm/internlm2_5-7b-chat model
  2. add ToolParserManager to manage the tool parsers
  3. add a command line which used to specific a customize tool parser which can be used in the --tool-call-parser
  4. add a parallel test skip config for models which dose not support parallel tool call

PR Checklist (Click to Expand)

Thank you for your contribution to vLLM! Before submitting the pull request, please ensure the PR meets the following criteria. This helps vLLM maintain the code quality and improve the efficiency of the review process.

PR Title and Classification

Only specific types of PRs will be reviewed. The PR title is prefixed appropriately to indicate the type of change. Please use one of the following:

  • [Bugfix] for bug fixes.
  • [CI/Build] for build or continuous integration improvements.
  • [Doc] for documentation fixes and improvements.
  • [Model] for adding a new model or improving an existing model. Model name should appear in the title.
  • [Frontend] For changes on the vLLM frontend (e.g., OpenAI API server, LLM class, etc.)
  • [Kernel] for changes affecting CUDA kernels or other compute kernels.
  • [Core] for changes in the core vLLM logic (e.g., LLMEngine, AsyncLLMEngine, Scheduler, etc.)
  • [Hardware][Vendor] for hardware-specific changes. Vendor name should appear in the prefix (e.g., [Hardware][AMD]).
  • [Misc] for PRs that do not fit the above categories. Please use this sparingly.

Note: If the PR spans more than one category, please include all relevant prefixes.

Code Quality

The PR need to meet the following code quality standards:

  • We adhere to Google Python style guide and Google C++ style guide.
  • Pass all linter checks. Please use format.sh to format your code.
  • The code need to be well-documented to ensure future contributors can easily understand the code.
  • Include sufficient tests to ensure the project to stay correct and robust. This includes both unit tests and integration tests.
  • Please add documentation to docs/source/ if the PR modifies the user-facing behaviors of vLLM. It helps vLLM user understand and utilize the new features or changes.

Adding or changing kernels

Each custom kernel needs a schema and one or more implementations to be registered with PyTorch.

  • Make sure custom ops are registered following PyTorch guidelines: Custom C++ and CUDA Operators and The Custom Operators Manual
  • Custom operations that return Tensors require meta-functions. Meta-functions should be implemented and registered in python so that dynamic dims can be handled automatically. See above documents for a description of meta-functions.
  • Use torch.libary.opcheck() to test the function registration and meta-function for any registered ops. See tests/kernels for examples.
  • When changing the C++ signature of an existing op, the schema must be updated to reflect the changes.
  • If a new custom type is needed, see the following document: Custom Class Support in PT2.

Notes for Large Changes

Please keep the changes as concise as possible. For major architectural changes (>500 LOC excluding kernel/data/config/test), we would expect a GitHub issue (RFC) discussing the technical design and justification. Otherwise, we will tag it with rfc-required and might not go through the PR.

What to Expect for the Reviews

The goal of the vLLM team is to be a transparent reviewing machine. We would like to make the review process transparent and efficient and make sure no contributor feel confused or frustrated. However, the vLLM team is small, so we need to prioritize some PRs over others. Here is what you can expect from the review process:

  • After the PR is submitted, the PR will be assigned to a reviewer. Every reviewer will pick up the PRs based on their expertise and availability.
  • After the PR is assigned, the reviewer will provide status update every 2-3 days. If the PR is not reviewed within 7 days, please feel free to ping the reviewer or the vLLM team.
  • After the review, the reviewer will put an action-required label on the PR if there are changes required. The contributor should address the comments and ping the reviewer to re-review the PR.
  • Please respond to all comments within a reasonable time frame. If a comment isn't clear or you disagree with a suggestion, feel free to ask for clarification or discuss the suggestion.

Thank You

Finally, thank you for taking the time to read these guidelines and for your interest in contributing to vLLM. Your contributions make vLLM a great tool for everyone!

sydnash avatar Sep 12 '24 08:09 sydnash

👋 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.

🚀

github-actions[bot] avatar Sep 12 '24 08:09 github-actions[bot]

@K-Mistele can you take a look at this and see if the changes look good to you? Since you mentioned that you will work on Llama3.1 tool parsing, it would be great to have an abstraction that works for your case as well.

DarkLight1337 avatar Sep 12 '24 08:09 DarkLight1337

@K-Mistele can you take a look at this and see if the changes look good to you? Since you mentioned that you will work on Llama3.1 tool parsing, it would be great to have an abstraction that works for your case as well.

Thanks for the ping! Checking now.

K-Mistele avatar Sep 12 '24 13:09 K-Mistele

@khluu It seems that the CI failure was caused by an environment issue. Could you please grant me retry permissions to give it a try?

sydnash avatar Sep 18 '24 06:09 sydnash

I have restarted the CI build.

DarkLight1337 avatar Sep 18 '24 07:09 DarkLight1337

Included a couple thoughts.

Generally,

  1. can you include documentation about the newly supported model(s) in the docs/source/serving/openai_compatible_server.md?
  2. it would probably be good to still have have the --tool-call-parser flag have a list of explicitly supported values for the purpose of clarity & understandability; which can be superceded by --tool-parser-plugin flag. This way. someone can easily understand which parsers are available out-of-the-box.
  3. Documentation on the implementation of tool parser plugins would be nice to have
  4. tests seem to have been unnecessarily broadened with respect to the "state" option of the get_current_weather tool. If internLM struggles to format this tool correctly, that should be fixed in the system prompt or chat template, instead of make tests less robust for all models.
  5. The streaming implementation needs a fix with type="function" as noted; and the streaming parser is not really implemented correctly.

hi, these things has been fixed. Please take some time to review it so that we can move on. @K-Mistele

sydnash avatar Sep 23 '24 11:09 sydnash

Hello, I have completed the review with @K-Mistele . What should I do next? @DarkLight1337 @mgoin

sydnash avatar Sep 26 '24 01:09 sydnash

Oh, there are some test cases failed due to out of memory. image

sydnash avatar Sep 26 '24 08:09 sydnash

Can you merge from main and see if the problem still persists?

DarkLight1337 avatar Sep 27 '24 00:09 DarkLight1337

@K-Mistele hi, can help me to see this. After merge from main, the mistral's test_chat_completion_with_tools case failed. It seems the module output is not correct. input:

INFO 09-29 07:59:31 logger.py:36] Received request chat-431bfe95c7b34771b2b2a50d71c8fb93: prompt: '<s>[INST] Hi! How are you?[/INST] I\'m doing great! How can I assist you?</s>[AVAILABLE_TOOLS] [{"type": "function", "function": {"name": "get_current_weather", "description": "Get the current weather in a given location", "parameters": {"type": "object", "properties": {"city": {"type": "string", "description": "The city to find the weather for, e.g. \'San Francisco\'"}, "state": {"type": "string", "description": "must the two-letter abbreviation for the state that the city is in, e.g. \'CA\' which would mean \'California\'"}, "unit": {"type": "string", "description": "The unit to fetch the temperature in", "enum": ["celsius", "fahrenheit"]}}}}}][/AVAILABLE_TOOLS][INST] Can you tell me a joke please?[/INST]', params: SamplingParams(n=1, best_of=1, presence_penalty=0.0, frequency_penalty=0.0, repetition_penalty=1.0, temperature=0.0, top_p=1.0, top_k=-1, min_p=0.0, seed=None, use_beam_search=False, length_penalty=1.0, early_stopping=False, stop=[], stop_token_ids=[], include_stop_str_in_output=False, ignore_eos=False, max_tokens=150, min_tokens=0, logprobs=None, prompt_logprobs=None, skip_special_tokens=True, spaces_between_special_tokens=True, truncate_prompt_tokens=None), prompt_token_ids: [1, 3, 16127, 29576, 2370, 1228, 1136, 29572, 4, 1083, 29510, 29487, 3316, 2366, 29576, 2370, 1309, 1083, 6799, 1136, 29572, 2, 6, 1501, 7567, 1891, 2032, 1113, 3396, 1316, 1113, 3396, 2032, 10598, 1629, 2032, 1113, 1295, 29498, 3790, 29498, 1537, 1991, 1316, 1113, 7286, 2032, 1113, 2226, 1040, 2636, 8854, 1065, 1032, 2846, 5491, 1316, 1113, 12206, 2032, 10598, 1891, 2032, 1113, 3582, 1316, 1113, 11491, 2032, 10598, 19141, 2032, 10598, 1891, 2032, 1113, 2195, 1316, 1113, 7286, 2032, 1113, 1782, 3758, 1066, 2068, 1040, 8854, 1122, 29493, 1085, 29491, 29489, 29491, 1232, 18672, 10454, 29510, 8474, 1113, 2795, 2032, 10598, 1891, 2032, 1113, 2195, 1316, 1113, 7286, 2032, 1113, 20727, 1040, 1757, 29501, 16635, 1302, 2920, 5987, 1120, 1122, 1040, 2433, 1137, 1040, 3758, 1117, 1065, 29493, 1085, 29491, 29489, 29491, 1232, 5962, 29510, 1458, 1450, 2840, 1232, 8386, 6767, 29510, 8474, 1113, 6074, 2032, 10598, 1891, 2032, 1113, 2195, 1316, 1113, 7286, 2032, 1113, 1782, 5796, 1066, 12470, 1040, 8409, 1065, 1316, 1113, 10825, 2032, 8135, 29485, 1958, 3938, 1316, 1113, 29490, 19425, 13075, 3010, 1743, 1743, 10925, 7, 3, 3186, 1136, 2680, 1296, 1032, 13783, 5433, 29572, 4], lora_request: None, prompt_adapter_request: None.

output:

model_output=[TOOL_CALLS] [{"name": "get_current_weather", "arguments": {"city": "San Francisco", "state": "CA", "unit": "fahrenheit"}}]

[{"name": "tell_joke"}]

I'm sorry, I don't have a function to tell jokes yet. But here's a classic one: Why don't scientists trust atoms? Because they make up everything! =end

the extract info like this:

tool call info = tools_called=True tool_calls=[ToolCall(id='chatcmpl-tool-5607719ad00441eea537f35ceada4a5c', type='function', function=FunctionCall(name='get_current_weather', arguments='{"city": "San Francisco", "state": "CA", "unit": "fahrenheit"}'))] content=None = end

sydnash avatar Sep 29 '24 00:09 sydnash

@K-Mistele hi, can help me to see this. After merge from main, the mistral's test_chat_completion_with_tools case failed. It seems the module output is not correct. input:

INFO 09-29 07:59:31 logger.py:36] Received request chat-431bfe95c7b34771b2b2a50d71c8fb93: prompt: '<s>[INST] Hi! How are you?[/INST] I\'m doing great! How can I assist you?</s>[AVAILABLE_TOOLS] [{"type": "function", "function": {"name": "get_current_weather", "description": "Get the current weather in a given location", "parameters": {"type": "object", "properties": {"city": {"type": "string", "description": "The city to find the weather for, e.g. \'San Francisco\'"}, "state": {"type": "string", "description": "must the two-letter abbreviation for the state that the city is in, e.g. \'CA\' which would mean \'California\'"}, "unit": {"type": "string", "description": "The unit to fetch the temperature in", "enum": ["celsius", "fahrenheit"]}}}}}][/AVAILABLE_TOOLS][INST] Can you tell me a joke please?[/INST]', params: SamplingParams(n=1, best_of=1, presence_penalty=0.0, frequency_penalty=0.0, repetition_penalty=1.0, temperature=0.0, top_p=1.0, top_k=-1, min_p=0.0, seed=None, use_beam_search=False, length_penalty=1.0, early_stopping=False, stop=[], stop_token_ids=[], include_stop_str_in_output=False, ignore_eos=False, max_tokens=150, min_tokens=0, logprobs=None, prompt_logprobs=None, skip_special_tokens=True, spaces_between_special_tokens=True, truncate_prompt_tokens=None), prompt_token_ids: [1, 3, 16127, 29576, 2370, 1228, 1136, 29572, 4, 1083, 29510, 29487, 3316, 2366, 29576, 2370, 1309, 1083, 6799, 1136, 29572, 2, 6, 1501, 7567, 1891, 2032, 1113, 3396, 1316, 1113, 3396, 2032, 10598, 1629, 2032, 1113, 1295, 29498, 3790, 29498, 1537, 1991, 1316, 1113, 7286, 2032, 1113, 2226, 1040, 2636, 8854, 1065, 1032, 2846, 5491, 1316, 1113, 12206, 2032, 10598, 1891, 2032, 1113, 3582, 1316, 1113, 11491, 2032, 10598, 19141, 2032, 10598, 1891, 2032, 1113, 2195, 1316, 1113, 7286, 2032, 1113, 1782, 3758, 1066, 2068, 1040, 8854, 1122, 29493, 1085, 29491, 29489, 29491, 1232, 18672, 10454, 29510, 8474, 1113, 2795, 2032, 10598, 1891, 2032, 1113, 2195, 1316, 1113, 7286, 2032, 1113, 20727, 1040, 1757, 29501, 16635, 1302, 2920, 5987, 1120, 1122, 1040, 2433, 1137, 1040, 3758, 1117, 1065, 29493, 1085, 29491, 29489, 29491, 1232, 5962, 29510, 1458, 1450, 2840, 1232, 8386, 6767, 29510, 8474, 1113, 6074, 2032, 10598, 1891, 2032, 1113, 2195, 1316, 1113, 7286, 2032, 1113, 1782, 5796, 1066, 12470, 1040, 8409, 1065, 1316, 1113, 10825, 2032, 8135, 29485, 1958, 3938, 1316, 1113, 29490, 19425, 13075, 3010, 1743, 1743, 10925, 7, 3, 3186, 1136, 2680, 1296, 1032, 13783, 5433, 29572, 4], lora_request: None, prompt_adapter_request: None.

output:

model_output=[TOOL_CALLS] [{"name": "get_current_weather", "arguments": {"city": "San Francisco", "state": "CA", "unit": "fahrenheit"}}]

[{"name": "tell_joke"}]

I'm sorry, I don't have a function to tell jokes yet. But here's a classic one: Why don't scientists trust atoms? Because they make up everything! =end

the extract info like this:

tool call info = tools_called=True tool_calls=[ToolCall(id='chatcmpl-tool-5607719ad00441eea537f35ceada4a5c', type='function', function=FunctionCall(name='get_current_weather', arguments='{"city": "San Francisco", "state": "CA", "unit": "fahrenheit"}'))] content=None = end

Yes, this is a known issue. Someone messed with tokenizers for Mistral models on main branch, which destabilitized Mistral tool calling with the default config.

Seems related to #8301 and #8515

Can you try setting the CLI arg for mistral in the utils.py for tests: --tokenizer-mode mistral --load-format mistral --config-format mistral and see if that works? Alternatively, you may have to explicitly disable mistral for all of these things and set them back to the default. I'm not sure which will work, but one of them should.

K-Mistele avatar Sep 29 '24 00:09 K-Mistele

@K-Mistele hi, can help me to see this. After merge from main, the mistral's test_chat_completion_with_tools case failed. It seems the module output is not correct. input:

INFO 09-29 07:59:31 logger.py:36] Received request chat-431bfe95c7b34771b2b2a50d71c8fb93: prompt: '<s>[INST] Hi! How are you?[/INST] I\'m doing great! How can I assist you?</s>[AVAILABLE_TOOLS] [{"type": "function", "function": {"name": "get_current_weather", "description": "Get the current weather in a given location", "parameters": {"type": "object", "properties": {"city": {"type": "string", "description": "The city to find the weather for, e.g. \'San Francisco\'"}, "state": {"type": "string", "description": "must the two-letter abbreviation for the state that the city is in, e.g. \'CA\' which would mean \'California\'"}, "unit": {"type": "string", "description": "The unit to fetch the temperature in", "enum": ["celsius", "fahrenheit"]}}}}}][/AVAILABLE_TOOLS][INST] Can you tell me a joke please?[/INST]', params: SamplingParams(n=1, best_of=1, presence_penalty=0.0, frequency_penalty=0.0, repetition_penalty=1.0, temperature=0.0, top_p=1.0, top_k=-1, min_p=0.0, seed=None, use_beam_search=False, length_penalty=1.0, early_stopping=False, stop=[], stop_token_ids=[], include_stop_str_in_output=False, ignore_eos=False, max_tokens=150, min_tokens=0, logprobs=None, prompt_logprobs=None, skip_special_tokens=True, spaces_between_special_tokens=True, truncate_prompt_tokens=None), prompt_token_ids: [1, 3, 16127, 29576, 2370, 1228, 1136, 29572, 4, 1083, 29510, 29487, 3316, 2366, 29576, 2370, 1309, 1083, 6799, 1136, 29572, 2, 6, 1501, 7567, 1891, 2032, 1113, 3396, 1316, 1113, 3396, 2032, 10598, 1629, 2032, 1113, 1295, 29498, 3790, 29498, 1537, 1991, 1316, 1113, 7286, 2032, 1113, 2226, 1040, 2636, 8854, 1065, 1032, 2846, 5491, 1316, 1113, 12206, 2032, 10598, 1891, 2032, 1113, 3582, 1316, 1113, 11491, 2032, 10598, 19141, 2032, 10598, 1891, 2032, 1113, 2195, 1316, 1113, 7286, 2032, 1113, 1782, 3758, 1066, 2068, 1040, 8854, 1122, 29493, 1085, 29491, 29489, 29491, 1232, 18672, 10454, 29510, 8474, 1113, 2795, 2032, 10598, 1891, 2032, 1113, 2195, 1316, 1113, 7286, 2032, 1113, 20727, 1040, 1757, 29501, 16635, 1302, 2920, 5987, 1120, 1122, 1040, 2433, 1137, 1040, 3758, 1117, 1065, 29493, 1085, 29491, 29489, 29491, 1232, 5962, 29510, 1458, 1450, 2840, 1232, 8386, 6767, 29510, 8474, 1113, 6074, 2032, 10598, 1891, 2032, 1113, 2195, 1316, 1113, 7286, 2032, 1113, 1782, 5796, 1066, 12470, 1040, 8409, 1065, 1316, 1113, 10825, 2032, 8135, 29485, 1958, 3938, 1316, 1113, 29490, 19425, 13075, 3010, 1743, 1743, 10925, 7, 3, 3186, 1136, 2680, 1296, 1032, 13783, 5433, 29572, 4], lora_request: None, prompt_adapter_request: None.

output:

model_output=[TOOL_CALLS] [{"name": "get_current_weather", "arguments": {"city": "San Francisco", "state": "CA", "unit": "fahrenheit"}}]

[{"name": "tell_joke"}]

I'm sorry, I don't have a function to tell jokes yet. But here's a classic one: Why don't scientists trust atoms? Because they make up everything! =end

the extract info like this:

tool call info = tools_called=True tool_calls=[ToolCall(id='chatcmpl-tool-5607719ad00441eea537f35ceada4a5c', type='function', function=FunctionCall(name='get_current_weather', arguments='{"city": "San Francisco", "state": "CA", "unit": "fahrenheit"}'))] content=None = end

Yes, this is a known issue. Someone messed with tokenizers for Mistral models on main branch, which destabilitized Mistral tool calling with the default config.

Seems related to #8301 and #8515

Can you try setting the CLI arg for mistral in the utils.py for tests: --tokenizer-mode mistral --load-format mistral --config-format mistral and see if that works? Alternatively, you may have to explicitly disable mistral for all of these things and set them back to the default. I'm not sure which will work, but one of them should.

After add these parameters, it crashed in mistral_tool_parser.py.

  File "/LocalRun/jun.dai/code/github/sydnash/vllm/vllm/entrypoints/openai/tool_parsers/mistral_tool_parser.py", line 51, in __init__
    self.bot_token_id = self.model_tokenizer.vocab[self.bot_token]
TypeError: 'method' object is not subscriptable

It seems the self.model_tokenizer.vocab is a methord, not a dict after set theses parameters.

sydnash avatar Sep 29 '24 01:09 sydnash

@K-Mistele hi, can help me to see this. After merge from main, the mistral's test_chat_completion_with_tools case failed. It seems the module output is not correct. input:

INFO 09-29 07:59:31 logger.py:36] Received request chat-431bfe95c7b34771b2b2a50d71c8fb93: prompt: '<s>[INST] Hi! How are you?[/INST] I\'m doing great! How can I assist you?</s>[AVAILABLE_TOOLS] [{"type": "function", "function": {"name": "get_current_weather", "description": "Get the current weather in a given location", "parameters": {"type": "object", "properties": {"city": {"type": "string", "description": "The city to find the weather for, e.g. \'San Francisco\'"}, "state": {"type": "string", "description": "must the two-letter abbreviation for the state that the city is in, e.g. \'CA\' which would mean \'California\'"}, "unit": {"type": "string", "description": "The unit to fetch the temperature in", "enum": ["celsius", "fahrenheit"]}}}}}][/AVAILABLE_TOOLS][INST] Can you tell me a joke please?[/INST]', params: SamplingParams(n=1, best_of=1, presence_penalty=0.0, frequency_penalty=0.0, repetition_penalty=1.0, temperature=0.0, top_p=1.0, top_k=-1, min_p=0.0, seed=None, use_beam_search=False, length_penalty=1.0, early_stopping=False, stop=[], stop_token_ids=[], include_stop_str_in_output=False, ignore_eos=False, max_tokens=150, min_tokens=0, logprobs=None, prompt_logprobs=None, skip_special_tokens=True, spaces_between_special_tokens=True, truncate_prompt_tokens=None), prompt_token_ids: [1, 3, 16127, 29576, 2370, 1228, 1136, 29572, 4, 1083, 29510, 29487, 3316, 2366, 29576, 2370, 1309, 1083, 6799, 1136, 29572, 2, 6, 1501, 7567, 1891, 2032, 1113, 3396, 1316, 1113, 3396, 2032, 10598, 1629, 2032, 1113, 1295, 29498, 3790, 29498, 1537, 1991, 1316, 1113, 7286, 2032, 1113, 2226, 1040, 2636, 8854, 1065, 1032, 2846, 5491, 1316, 1113, 12206, 2032, 10598, 1891, 2032, 1113, 3582, 1316, 1113, 11491, 2032, 10598, 19141, 2032, 10598, 1891, 2032, 1113, 2195, 1316, 1113, 7286, 2032, 1113, 1782, 3758, 1066, 2068, 1040, 8854, 1122, 29493, 1085, 29491, 29489, 29491, 1232, 18672, 10454, 29510, 8474, 1113, 2795, 2032, 10598, 1891, 2032, 1113, 2195, 1316, 1113, 7286, 2032, 1113, 20727, 1040, 1757, 29501, 16635, 1302, 2920, 5987, 1120, 1122, 1040, 2433, 1137, 1040, 3758, 1117, 1065, 29493, 1085, 29491, 29489, 29491, 1232, 5962, 29510, 1458, 1450, 2840, 1232, 8386, 6767, 29510, 8474, 1113, 6074, 2032, 10598, 1891, 2032, 1113, 2195, 1316, 1113, 7286, 2032, 1113, 1782, 5796, 1066, 12470, 1040, 8409, 1065, 1316, 1113, 10825, 2032, 8135, 29485, 1958, 3938, 1316, 1113, 29490, 19425, 13075, 3010, 1743, 1743, 10925, 7, 3, 3186, 1136, 2680, 1296, 1032, 13783, 5433, 29572, 4], lora_request: None, prompt_adapter_request: None.

output:

model_output=[TOOL_CALLS] [{"name": "get_current_weather", "arguments": {"city": "San Francisco", "state": "CA", "unit": "fahrenheit"}}]

[{"name": "tell_joke"}]

I'm sorry, I don't have a function to tell jokes yet. But here's a classic one: Why don't scientists trust atoms? Because they make up everything! =end

the extract info like this:

tool call info = tools_called=True tool_calls=[ToolCall(id='chatcmpl-tool-5607719ad00441eea537f35ceada4a5c', type='function', function=FunctionCall(name='get_current_weather', arguments='{"city": "San Francisco", "state": "CA", "unit": "fahrenheit"}'))] content=None = end

Yes, this is a known issue. Someone messed with tokenizers for Mistral models on main branch, which destabilitized Mistral tool calling with the default config. Seems related to #8301 and #8515 Can you try setting the CLI arg for mistral in the utils.py for tests: --tokenizer-mode mistral --load-format mistral --config-format mistral and see if that works? Alternatively, you may have to explicitly disable mistral for all of these things and set them back to the default. I'm not sure which will work, but one of them should.

After add these parameters, it crashed in mistral_tool_parser.py.

  File "/LocalRun/jun.dai/code/github/sydnash/vllm/vllm/entrypoints/openai/tool_parsers/mistral_tool_parser.py", line 51, in __init__
    self.bot_token_id = self.model_tokenizer.vocab[self.bot_token]
TypeError: 'method' object is not subscriptable

It seems the self.model_tokenizer.vocab is a methord, not a dict after set theses parameters.

I fixed it in this commit 1db530d908bcd24664e1ba3baa2fe77f45c4ffe1.

  1. I add a vocab property to MistralTokenizer in vllm/transformers_utils/tokenizers/mistral.py file. @DarkLight1337 Can you help me to check if this is ok.
  2. Remove the modify of self.model_tokenizer in vllm/entrypoints/openai/tool_parsers/mistral_tool_parser.py.

sydnash avatar Sep 29 '24 02:09 sydnash

I add a vocab property to MistralTokenizer in vllm/transformers_utils/tokenizers/mistral.py file. @DarkLight1337 Can you help me to check if this is ok. Remove the modify of self.model_tokenizer in vllm/entrypoints/openai/tool_parsers/mistral_tool_parser.py.

Looking at the definition of the tokenizer in HuggingFace (PreTrainedTokenizerBase), there doesn't seem to be a publicly defined property called vocab. Instead, there is a method called get_vocab. To maintain a consistent API, I suggest to instead update the tool parser to call that method.

DarkLight1337 avatar Sep 29 '24 02:09 DarkLight1337

I add a vocab property to MistralTokenizer in vllm/transformers_utils/tokenizers/mistral.py file. @DarkLight1337 Can you help me to check if this is ok. Remove the modify of self.model_tokenizer in vllm/entrypoints/openai/tool_parsers/mistral_tool_parser.py.

Looking at the definition of the tokenizer in HuggingFace (PreTrainedTokenizerBase), there doesn't seem to be a publicly defined property called vocab. Instead, there is a method called get_vocab. To maintain a consistent API, I suggest to instead update the tool parser to call that method.

Thanks, I'll change it.

I only found vocab property in the PreTrainedTokenizerFast class.

sydnash avatar Sep 29 '24 02:09 sydnash

After add these CLI flags to choose the MistralTokenizer, the chat_template will cannot works as the warning message says here.
The prompt generated by apply_chat_template is not correct even if I add chat_template=chat_template to the apply_chat_template function. @DarkLight1337 @K-Mistele

def apply_mistral_chat_template(
    tokenizer: MistralTokenizer,
    messages: List[ChatCompletionMessageParam],
    chat_template: Optional[str] = None,
    **kwargs: Any,
) -> List[int]:
    if chat_template is not None:
        logger.warning(
            "'chat_template' cannot be overridden for mistral tokenizer.")

    return tokenizer.apply_chat_template(
        messages=messages,
        **kwargs,
    )

sydnash avatar Sep 29 '24 09:09 sydnash

@patrickvonplaten since you were the last one to update the mistral tokenizer, can you investigate this regression?

DarkLight1337 avatar Sep 29 '24 09:09 DarkLight1337

Based on my current testing, there are three issues:

  1. The message parameter of apply_mistral_chat_template need to change from request.messages to conversation due to this error:
ERROR 09-29 17:36:44 serving_chat.py:153]   File "/LocalRun/jun.dai/conda/envs/vllm_env/lib/python3.10/site-packages/mistral_common/protocol/instruct/validator.py", line 147, in _validate_assistant_message
ERROR 09-29 17:36:44 serving_chat.py:153]     raise InvalidAssistantMessageException(
ERROR 09-29 17:36:44 serving_chat.py:153] mistral_common.exceptions.InvalidAssistantMessageException: Assistant message must have either content or tool_calls, but not both.
  1. The tool call id must change to fit ^[a-zA-Z0-9]{9}$ regex due to this error:
ERROR 09-29 17:29:25 serving_chat.py:153]   File "/LocalRun/jun.dai/conda/envs/vllm_env/lib/python3.10/site-packages/mistral_common/protocol/instruct/validator.py", line 310, in _validate_tool_call
ERROR 09-29 17:29:25 serving_chat.py:153]     raise InvalidFunctionCallException(
ERROR 09-29 17:29:25 serving_chat.py:153] mistral_common.exceptions.InvalidFunctionCallException: Tool call id was chatcmpl-tool-03e6481b146e408e9523d9c956696295 but must be a-z, A-Z, 0-9, with a length of 9.
  1. The model cannot generate the correct tool call message even doing the above change.
[{"name": "get_current_weather", "arguments": {"city": "Dallas", "state": "TX", "unit": "fahrenheit"}}, {"name": "get_current_weather", "arguments": {"city": "Orlando", "state": "FL", "unit": "fahrenheit"}}]

The output message has no [TOOL_CALLS], which is used to identify it as a tool call message.

sydnash avatar Sep 29 '24 09:09 sydnash

@patrickvonplaten since you were the last one to update the mistral tokenizer, can you investigate this regression?

Sorry I'm not 100% following here - this PR seems to be related to internlm - not MistralTokenizer no?

If there is a regression for MistralTokenizer, can we maybe open an issue about it? Happy to look into it

patrickvonplaten avatar Sep 30 '24 10:09 patrickvonplaten

Based on my current testing, there are three issues:

  1. The message parameter of apply_mistral_chat_template need to change from request.messages to conversation due to this error:
ERROR 09-29 17:36:44 serving_chat.py:153]   File "/LocalRun/jun.dai/conda/envs/vllm_env/lib/python3.10/site-packages/mistral_common/protocol/instruct/validator.py", line 147, in _validate_assistant_message
ERROR 09-29 17:36:44 serving_chat.py:153]     raise InvalidAssistantMessageException(
ERROR 09-29 17:36:44 serving_chat.py:153] mistral_common.exceptions.InvalidAssistantMessageException: Assistant message must have either content or tool_calls, but not both.
  1. The tool call id must change to fit ^[a-zA-Z0-9]{9}$ regex due to this error:
ERROR 09-29 17:29:25 serving_chat.py:153]   File "/LocalRun/jun.dai/conda/envs/vllm_env/lib/python3.10/site-packages/mistral_common/protocol/instruct/validator.py", line 310, in _validate_tool_call
ERROR 09-29 17:29:25 serving_chat.py:153]     raise InvalidFunctionCallException(
ERROR 09-29 17:29:25 serving_chat.py:153] mistral_common.exceptions.InvalidFunctionCallException: Tool call id was chatcmpl-tool-03e6481b146e408e9523d9c956696295 but must be a-z, A-Z, 0-9, with a length of 9.
  1. The model cannot generate the correct tool call message even doing the above change.
[{"name": "get_current_weather", "arguments": {"city": "Dallas", "state": "TX", "unit": "fahrenheit"}}, {"name": "get_current_weather", "arguments": {"city": "Orlando", "state": "FL", "unit": "fahrenheit"}}]

The output message has no [TOOL_CALLS], which is used to identify it as a tool call message.

@sydnash does this occur without this PR? If so, please open a new issue and ping @patrickvonplaten about it.

DarkLight1337 avatar Sep 30 '24 10:09 DarkLight1337

It's only occur while start the server with the --tokenizer-mode mistral CLI, I do not know dose it a issue, It seems because the mistral tool parser and openai api are not compatible with the MistralTokenizer. I'll remove it from this PR in the test case.

sydnash avatar Oct 03 '24 01:10 sydnash

@K-Mistele hi, can help me to see this. After merge from main, the mistral's test_chat_completion_with_tools case failed. It seems the module output is not correct. input:

INFO 09-29 07:59:31 logger.py:36] Received request chat-431bfe95c7b34771b2b2a50d71c8fb93: prompt: '<s>[INST] Hi! How are you?[/INST] I\'m doing great! How can I assist you?</s>[AVAILABLE_TOOLS] [{"type": "function", "function": {"name": "get_current_weather", "description": "Get the current weather in a given location", "parameters": {"type": "object", "properties": {"city": {"type": "string", "description": "The city to find the weather for, e.g. \'San Francisco\'"}, "state": {"type": "string", "description": "must the two-letter abbreviation for the state that the city is in, e.g. \'CA\' which would mean \'California\'"}, "unit": {"type": "string", "description": "The unit to fetch the temperature in", "enum": ["celsius", "fahrenheit"]}}}}}][/AVAILABLE_TOOLS][INST] Can you tell me a joke please?[/INST]', params: SamplingParams(n=1, best_of=1, presence_penalty=0.0, frequency_penalty=0.0, repetition_penalty=1.0, temperature=0.0, top_p=1.0, top_k=-1, min_p=0.0, seed=None, use_beam_search=False, length_penalty=1.0, early_stopping=False, stop=[], stop_token_ids=[], include_stop_str_in_output=False, ignore_eos=False, max_tokens=150, min_tokens=0, logprobs=None, prompt_logprobs=None, skip_special_tokens=True, spaces_between_special_tokens=True, truncate_prompt_tokens=None), prompt_token_ids: [1, 3, 16127, 29576, 2370, 1228, 1136, 29572, 4, 1083, 29510, 29487, 3316, 2366, 29576, 2370, 1309, 1083, 6799, 1136, 29572, 2, 6, 1501, 7567, 1891, 2032, 1113, 3396, 1316, 1113, 3396, 2032, 10598, 1629, 2032, 1113, 1295, 29498, 3790, 29498, 1537, 1991, 1316, 1113, 7286, 2032, 1113, 2226, 1040, 2636, 8854, 1065, 1032, 2846, 5491, 1316, 1113, 12206, 2032, 10598, 1891, 2032, 1113, 3582, 1316, 1113, 11491, 2032, 10598, 19141, 2032, 10598, 1891, 2032, 1113, 2195, 1316, 1113, 7286, 2032, 1113, 1782, 3758, 1066, 2068, 1040, 8854, 1122, 29493, 1085, 29491, 29489, 29491, 1232, 18672, 10454, 29510, 8474, 1113, 2795, 2032, 10598, 1891, 2032, 1113, 2195, 1316, 1113, 7286, 2032, 1113, 20727, 1040, 1757, 29501, 16635, 1302, 2920, 5987, 1120, 1122, 1040, 2433, 1137, 1040, 3758, 1117, 1065, 29493, 1085, 29491, 29489, 29491, 1232, 5962, 29510, 1458, 1450, 2840, 1232, 8386, 6767, 29510, 8474, 1113, 6074, 2032, 10598, 1891, 2032, 1113, 2195, 1316, 1113, 7286, 2032, 1113, 1782, 5796, 1066, 12470, 1040, 8409, 1065, 1316, 1113, 10825, 2032, 8135, 29485, 1958, 3938, 1316, 1113, 29490, 19425, 13075, 3010, 1743, 1743, 10925, 7, 3, 3186, 1136, 2680, 1296, 1032, 13783, 5433, 29572, 4], lora_request: None, prompt_adapter_request: None.

output:

model_output=[TOOL_CALLS] [{"name": "get_current_weather", "arguments": {"city": "San Francisco", "state": "CA", "unit": "fahrenheit"}}]

[{"name": "tell_joke"}]

I'm sorry, I don't have a function to tell jokes yet. But here's a classic one: Why don't scientists trust atoms? Because they make up everything! =end

the extract info like this:

tool call info = tools_called=True tool_calls=[ToolCall(id='chatcmpl-tool-5607719ad00441eea537f35ceada4a5c', type='function', function=FunctionCall(name='get_current_weather', arguments='{"city": "San Francisco", "state": "CA", "unit": "fahrenheit"}'))] content=None = end

Yes, this is a known issue. Someone messed with tokenizers for Mistral models on main branch, which destabilitized Mistral tool calling with the default config.

Seems related to #8301 and #8515

Can you try setting the CLI arg for mistral in the utils.py for tests: --tokenizer-mode mistral --load-format mistral --config-format mistral and see if that works? Alternatively, you may have to explicitly disable mistral for all of these things and set them back to the default. I'm not sure which will work, but one of them should.

The issue is because I accidentally deleted the system prompt of Mistral during the merge. I think the mistral tool parser from @K-Mistele PR dose not support the MistralTokenizer.

sydnash avatar Oct 03 '24 01:10 sydnash

My work on InternLM is complete. Should I open a separate issue to discuss the compatibility of MistralTokenizer with the openai API implement? Currently, the Mistral model can work without using --tokenizer-mode mistral, but there is a warning at startup. @DarkLight1337 @patrickvonplaten @K-Mistele @mgoin

FutureWarning: It is strongly recommended to run mistral models with `--tokenizer_mode "mistral"` to ensure correct encoding and decoding.

sydnash avatar Oct 03 '24 19:10 sydnash

My work on InternLM is complete. Should I open a separate issue to discuss the compatibility of MistralTokenizer with the openai API implement? Currently, the Mistral model can work without using --tokenizer-mode mistral, but there is a warning at startup. @DarkLight1337 @patrickvonplaten @K-Mistele @mgoin

FutureWarning: It is strongly recommended to run mistral models with `--tokenizer_mode "mistral"` to ensure correct encoding and decoding.

Think the warning is expected :-) Would be suuper nice if you could open a seperate issue about the --tokenizer-mode mistral and ping me there

patrickvonplaten avatar Oct 03 '24 21:10 patrickvonplaten