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

fix: improve tool type, bump pydantic and outlines #1650

Merged
merged 8 commits into from
Mar 21, 2024

Conversation

drbh
Copy link
Collaborator

@drbh drbh commented Mar 18, 2024

This PR resolves a couple

  • adjusts the tool response to align with openai's tools response type
  • bumps pydantic to 2.6.4 in all apps (resolves dependency issue when running tests)
  • bump outlines version and fix import for new name

@drbh drbh force-pushed the adjust-tool-response-type branch from 13f2560 to 7ad4a62 Compare March 18, 2024 16:27
@drbh drbh marked this pull request as ready for review March 19, 2024 18:18
@drbh drbh changed the title fix: prefer tool call vector over object fix: improve tool type, bump pydantic and outlines Mar 19, 2024
@drbh
Copy link
Collaborator Author

drbh commented Mar 19, 2024

this PR resolves
#1624
#1653

router/src/lib.rs Outdated Show resolved Hide resolved
server/pyproject.toml Outdated Show resolved Hide resolved
Narsil
Narsil previously approved these changes Mar 20, 2024
Copy link
Collaborator

@Narsil Narsil left a comment

Choose a reason for hiding this comment

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

Some nits.

{
"function": {
"description": None,
"name": "tools",
Copy link

@joumenharzli joumenharzli Mar 20, 2024

Choose a reason for hiding this comment

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

the "name" here should be "get_current_weather"

{
"function": {
"description": None,
"name": "tools",

Choose a reason for hiding this comment

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

also here the "name" here should be "get_current_weather"

@joumenharzli
Copy link

joumenharzli commented Mar 20, 2024

Thanks @drbh for your PR, I've noticed that you changed the type of "tool_calls" from an object to a list, which is great.
Additionally, there's another issue where the returned function name is consistently "tools".
is this issue will be addressed in this PR?
the bug comes from here

name: "tools".to_string(),

Would it be feasible to to have the tool_grammar in the format of {name:"",arguments:{}} instead of {function:{}} ?

let tool_grammar = if let Some((req_tools, tool_choice)) = req.tools.zip(req.tool_choice) {

this would instruct the LLM to respond with the choosen function name and it's argument, and it can be parsed later here

let tool_calls = vec![ChatCompletionMessageToolCall {

I never worked with Rust, otherwise I would subbmited a PR!

@drbh
Copy link
Collaborator Author

drbh commented Mar 20, 2024

Hi @joumenharzli thanks for highlighting that issue! Unfortunately the current expected value is "name": "tools" because of the issue you noted above (the LLM is not specifying the function it chose in it's response).

Updating the tool's formatting is interesting, and in general the currently implementation is a bit rigid. However a change like that is outside of the scope of this PR and should be addressed in a new PR. I'll open a new issue to continue this conversation: #1657 and I'll open a new PR when we have a clear path forward

Copy link
Collaborator

@Narsil Narsil left a comment

Choose a reason for hiding this comment

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

LGTM

@drbh drbh merged commit de6cb15 into main Mar 21, 2024
7 of 8 checks passed
@drbh drbh deleted the adjust-tool-response-type branch March 21, 2024 16:45
kdamaszk pushed a commit to kdamaszk/tgi-gaudi that referenced this pull request Apr 29, 2024
This PR resolves a couple

- [X] adjusts the tool response to align with openai's tools response
type
- [X] bumps pydantic to `2.6.4` in all apps (resolves dependency issue
when running tests)
- [X] bump `outlines` version and fix import for new name
@aW3st aW3st mentioned this pull request Oct 16, 2024
4 tasks
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.

3 participants