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

Add tools to api_server for InternLM2 model #1763

Merged
merged 19 commits into from
Jul 9, 2024
Merged

Conversation

AllentDan
Copy link
Collaborator

No description provided.

@lvhan028
Copy link
Collaborator

Pls fix the UT test error and the PR test error

@lvhan028 lvhan028 added the enhancement New feature or request label Jun 13, 2024
@lvhan028 lvhan028 requested review from irexyc and lvhan028 June 20, 2024 20:42
}
]
messages = [{"role": "user", "content": "What's the weather like in Boston today?"}]
tool_choice={"type": "function", "function": {"name": "get_current_weather"}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we support tool_choice?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only supports none or specifying a particular tool in json format.

@@ -434,10 +451,21 @@ async def chat_completions_v1(request: ChatCompletionRequest,
stop_words=request.stop,
skip_special_tokens=request.skip_special_tokens)

tools = None
if request.tools and request.tool_choice != 'none':
Copy link
Collaborator

@lvhan028 lvhan028 Jun 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we make error response to the client when request.tool_choice is not None?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made it in the check_request function.

@lvhan028
Copy link
Collaborator

Do we support "finish_reason": "tool_calls"?

@AllentDan
Copy link
Collaborator Author

Do we support "finish_reason": "tool_calls"?

No, we did not yet.

@AllentDan
Copy link
Collaborator Author

@Harold-lkk May help review this PR。

@lvhan028 lvhan028 requested a review from Harold-lkk June 25, 2024 06:16
@AllentDan
Copy link
Collaborator Author

I got this prompt string if I used the code snippet in our doc.

<|im_start|>system\nYou are an AI assistant whose name is InternLM (书生·浦语).\n- InternLM (书生·浦语) is a conversational language model that is developed by Shanghai AI Laboratory (上海人工智能实验室). It is designed to be helpful, honest, and harmless.\n- InternLM (书生·浦语) can understand and communicate fluently in the language chosen by the user such as English and 中文.\n<|im_end|>\n<|im_start|>system name=<|plugin|>\n[{"description": "Get the current weather in a given location", "name": "get_current_weather", "parameters": {"type": "object", "properties": {"location": {"type": "string", "description": "The city and state, e.g. San Francisco, CA"}, "unit": {"type": "string", "enum": ["celsius", "fahrenheit"]}}, "required": ["location"]}}]<|im_end|>\n<|im_start|>user\nWhat\'s the weather like in Boston today?<|im_end|>\n<|im_start|>assistant\n

And only internlm2-chat1_8b return the desired output:

ChatCompletion(id='1', choices=[Choice(finish_reason='tool_calls', index=0, logprobs=None, message=ChatCompletionMessage(content='', role='assistant', function_call=None, tool_calls=[ChatCompletionMessageToolCall(id='1', function=Function(arguments={'location': 'Boston'}, name='get_current_weather'), type='function')]))], created=1719306194, model='/nvme/shared_data/InternLM/internlm2-chat-1_8b', object='chat.completion', system_fingerprint=None, usage=CompletionUsage(completion_tokens=23, prompt_tokens=209, total_tokens=232))

@AllentDan
Copy link
Collaborator Author

It turns out that we have to pass skip_special_tokens=False in the client.

@lvhan028
Copy link
Collaborator

@zhulinJulia24 we should add test cases for internlm2 function_call

@lvhan028 lvhan028 requested a review from RunningLeon July 1, 2024 06:32

class ToolCall(BaseModel):
"""Tool call response."""
id: str
Copy link
Collaborator

@lvhan028 lvhan028 Jul 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the openai spec, id refers to "The ID of the tool call".
In my test, it increases by 1 automatically.
I view the id as the index of the invoked tool in the tool list. But I am not sure about it.
@Harold-lkk, can you clarify it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GPT supports multi-function calls at the same time, so the id is used to identify which function call of the response

Copy link
Collaborator Author

@AllentDan AllentDan Jul 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GPT supports multi-function calls at the same time, so the id is used to identify which function call of the response

Does that mean we have to return 0 for all requests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, we have to find out the index of the returned function in tools list.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if two functions share the same name? Can it happen or do we have to consider this possibility? @Harold-lkk

@lvhan028
Copy link
Collaborator

lvhan028 commented Jul 1, 2024

The test_messages2prompt4internlm2_chat didn't cover the if tools statement. Please add UT for the if tools branch

top_p=0.8,
stream=False,
tools=tools,
extra_body={'skip_special_tokens': False})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we hide the extra_body in the implementation instead of the API?
I don't find it in the openai API spec.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra_body is common when calling openai api. There are documentations in vllm too. https://docs.vllm.ai/en/latest/serving/openai_compatible_server.html#extra-parameters

if request.tools and request.tool_choice != 'none':
if request.stream is True:
logger.warning('Set stream to False for tools')
request.stream = False
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why change it to False?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it only supports stream=False. In streaming mode, how can client call the function? And it would be hard to extract content inside too.

Copy link
Collaborator

@lvhan028 lvhan028 Jul 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the client call the function when the finish_reason is tool_calls?

@lvhan028
Copy link
Collaborator

lvhan028 commented Jul 1, 2024

There is warning for each request

[TM][WARNING] [ProcessInferRequests] [1] total sequence length (209 + 65335) exceeds `session_len` (65544), `request_output_len` is truncated to 65334

I tried the get_current_weather example

lmdeploy/model.py Outdated Show resolved Hide resolved
@@ -173,6 +173,10 @@ for message in messages:
print(item)
```

### Tools
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to add the md file to index.rst since pr #1880 is merged. May need to sync with main branch.

AllentDan added 2 commits July 4, 2024 16:13
Conflicts:
	examples/vl/qwen_model.py
	examples/vl/xcomposer_model.py
@lvhan028
Copy link
Collaborator

lvhan028 commented Jul 4, 2024

@zhulinJulia24 may add TC

Conflicts:
	lmdeploy/model.py
@lvhan028 lvhan028 merged commit c12786b into InternLM:main Jul 9, 2024
3 of 5 checks passed
@zhulinJulia24 zhulinJulia24 mentioned this pull request Jul 16, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants