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

Allow returning thought in the tool call responses #5192

Open
osdemah opened this issue Jan 25, 2025 · 7 comments
Open

Allow returning thought in the tool call responses #5192

osdemah opened this issue Jan 25, 2025 · 7 comments
Labels
api-break-change Will break existing api and needs an migration for previous code to work needs-triage proj-agentchat proj-core proj-extensions
Milestone

Comments

@osdemah
Copy link

osdemah commented Jan 25, 2025

What feature would you like to be added?

Some models include an additional text message in the tool call response. for example:

{
  "role": "assistant",
  "content": [
    {
      "type": "text",
      "text": "<thinking>To answer this question, I will: 1. Use the get_weather tool to get the current weather in San Francisco. 2. Use the get_time tool to get the current time in the America/Los_Angeles timezone, which covers San Francisco, CA.</thinking>"
    },
    {
      "type": "tool_use",
      "id": "toolu_01A09q90qw90lq917835lq9",
      "name": "get_weather",
      "input": {"location": "San Francisco, CA"}
    }
  ]
}

We need to allow including that message in the response, so that it can be properly surfaced to agent and users.

The proposal in #5173 is to change the content field to Union[str, FunctionCalls] instead of Union[str, List[FunctionCall]] so that additional fields could be included in FunctionCalls:

@dataclass
class FunctionCalls:
    function_calls: List[FunctionCall]
    thought: Optional[str] = None

Why is this needed?

to include model response properly

@ekzhu ekzhu added this to the 0.4.x milestone Jan 25, 2025
@ekzhu ekzhu added api-break-change Will break existing api and needs an migration for previous code to work proj-core proj-extensions proj-agentchat labels Jan 25, 2025
@ekzhu
Copy link
Collaborator

ekzhu commented Jan 25, 2025

Thank you @osdemah , since this is an API breaking change, let's have a bit more discussion around this. We may take a bit longer on this. I do think it is important.

cc @jackgerrits

Is there a way to avoid an API breaking change? So that existing code won't break?

@ekzhu
Copy link
Collaborator

ekzhu commented Jan 25, 2025

@osdemah related #5161 , what do you think about supporting reasoning models together with supporting thoughts in tool calls?

@osdemah
Copy link
Author

osdemah commented Jan 27, 2025

@ekzhu yep, I think that is pretty much the same. essentially we need a text to be returned alongside of the list of tool calls. do you have a proposal for a non-breaking change? Initially I started with turning it into content: Union[str, List[Union[str, FunctionCall]]] instead of Union[str, FunctionCalls], but that is:

  1. still breaking change
  2. harder to read
  3. adds more complexity, as you'd have to preserve the ordering of text and tool calls.

I think unless if you're thinking of adding a field in the body of CreateResult (which is not cleanest, since you'd have to make more model changes to surfaced that - e.g. AssistantMessage and AssistantContent) a breaking change is inevitable here.

@osdemah
Copy link
Author

osdemah commented Jan 29, 2025

@ekzhu any update on this?

@ekzhu
Copy link
Collaborator

ekzhu commented Jan 29, 2025

@osdemah thanks for reaching out. Would you like to join the office hour today to discuss? #4059

I don't have an update at this moment except I have been hearing a lot issues similar to yours.

@ekzhu
Copy link
Collaborator

ekzhu commented Jan 30, 2025

@osdemah update, in #5262 , we would like to add a thought field in the CreateResult itself. So now this field can handle function calls with content. More work needed to incorporate the non-None content when tool calls is detected. Would love to have you contribute to that once #5262 is merged.

Though, to address the kind you API raw response you are seeing, we need a different client other than OpenAIChatCompletionClient.

@osdemah
Copy link
Author

osdemah commented Jan 30, 2025

sounds good, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-break-change Will break existing api and needs an migration for previous code to work needs-triage proj-agentchat proj-core proj-extensions
Projects
None yet
Development

No branches or pull requests

2 participants