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 support for response format in tools #74

Closed
wants to merge 1 commit into from

Conversation

OskarStark
Copy link
Contributor

@OskarStark OskarStark commented Oct 1, 2024

closes #49

I avoided to add logic for closures for now

@OskarStark OskarStark self-assigned this Oct 1, 2024
src/ToolBox/AsTool.php Outdated Show resolved Hide resolved
@@ -41,6 +41,8 @@ public function processOutput(Output $output): mixed
$messages[] = Message::ofToolCall($toolCall, $result);
}

// we need to check if the tool has a response format defined and apply that to the following call
Copy link
Contributor Author

@OskarStark OskarStark Oct 1, 2024

Choose a reason for hiding this comment

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

TODO, but before doing it, I wanted to make sure the current state off ToolBox\ChainProcessor works as expected

@@ -11,13 +11,15 @@
{
/**
* @param ParameterDefinition|null $parameters
* @param array<mixed>|null $responseFormat
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The resolving of the responseFormat should be done before, so that we just have to deal with null (no response format) or a valid response format (array) here in the Metadata class, I hope you agree on that

src/ToolBox/Metadata.php Outdated Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

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

reworked that test a bit and added two more tests for response format

@OskarStark OskarStark added the enhancement New feature or request label Oct 1, 2024
@OskarStark OskarStark requested a review from chr-hertel October 1, 2024 09:56
@OskarStark OskarStark changed the title Add support for response_format in tools Add support for response format in tools Oct 1, 2024
@OskarStark OskarStark force-pushed the feature/tool-response-format branch from 1d79abf to e73ea9b Compare October 2, 2024 08:11
@OskarStark OskarStark force-pushed the feature/tool-response-format branch from e73ea9b to 929c05d Compare October 2, 2024 08:46
@OskarStark OskarStark mentioned this pull request Oct 2, 2024
@OskarStark OskarStark force-pushed the feature/tool-response-format branch from 3e3f931 to 2cd0309 Compare October 2, 2024 09:28
@OskarStark OskarStark force-pushed the feature/tool-response-format branch from 2cd0309 to 73cbc38 Compare October 4, 2024 09:47
@OskarStark
Copy link
Contributor Author

Rebased

@chr-hertel
Copy link
Member

does it make sense to bring in an example here?

@OskarStark
Copy link
Contributor Author

Yes, 👍🏻

@@ -33,4 +33,5 @@
$messages = new MessageBag(Message::ofUser('What date and time is it?'));
$response = $chain->call($messages);

echo $response.PHP_EOL;
// Clock tool forced a structured output
dump(json_decode($response, true));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CleanShot 2024-10-04 at 22 35 10@2x

This is now working @chr-hertel, but maybe you can help me polish the part in ChainProcessor. I am also not that deep in the ChainProcessor thing and I really don't get the difference between response_format and output_structure there. Thanks

@OskarStark OskarStark force-pushed the feature/tool-response-format branch from b4750bc to 5bd4054 Compare October 4, 2024 20:36
Comment on lines 41 to 67
foreach ($response->getToolCalls() as $toolCall) {
foreach ($this->toolBox->getMap() as $metadata) {
if ($metadata->name === $toolCall->name
&& null !== $metadata->responseFormat
) {
$options = array_merge($options, [
'response_format' => [
'type' => 'json_schema',
'json_schema' => [
'schema' => $metadata->responseFormat,
'name' => $metadata->name,
],
],
]);
break;
}
}

$result = $this->toolBox->execute($toolCall);
$messages[] = Message::ofToolCall($toolCall, $result);
}

$response = $output->llm->call($messages, $output->options);
$response = $output->llm->call($messages, $options);
Copy link
Member

Choose a reason for hiding this comment

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

this cannot work like this. or maybe it does with the example, but

a) multiple tool calls will interfere with each other
b) the original output format get overwritten

Copy link
Member

Choose a reason for hiding this comment

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

just realized why my assumption was wrong. the processor calls the llm not the chain - this is what makes it hard.

@chr-hertel
Copy link
Member

chr-hertel commented Oct 5, 2024

please have a look at

and

if they provide what you were looking for

@OskarStark OskarStark force-pushed the feature/tool-response-format branch from 5bd4054 to aa39f4b Compare October 5, 2024 18:50
@chr-hertel
Copy link
Member

not sure this is still needed after #118 - guess the format is tackled and additional messages or option changes should be possible with custom processor - or do i miss something?

@OskarStark
Copy link
Contributor Author

not sure this is still needed after #118 - guess the format is tackled and additional messages or option changes should be possible with custom processor - or do i miss something?

What do you mean by "is not needed"? The change in the chain processor?

Feel free to overtake the PR, I "steh ein bisschen auf dem Schlauch" 😄

Extra messages can be ignored for now, but I think yes, they are doable by your other PR, showcasing the TestProcessor

@OskarStark OskarStark force-pushed the feature/tool-response-format branch from aa39f4b to 3bccbaf Compare October 8, 2024 20:39
examples/toolbox-clock.php Outdated Show resolved Hide resolved
@OskarStark OskarStark force-pushed the feature/tool-response-format branch from 76f398e to d1810a2 Compare October 8, 2024 20:44
@OskarStark
Copy link
Contributor Author

So the current state of my PR is still working after a rebase and provides me with the correct result. However, after removing my logic from the ChainProcessor it doesn't work anymore.

I would really appreciate if you could take over this PR, as I am bit lost here 😄 Thanks! 🙏

@chr-hertel
Copy link
Member

Let's have a call maybe friday or sth when I'm back home

@DZunke
Copy link
Contributor

DZunke commented Nov 14, 2024

Hey, did your call result in something? Just out if curiousity because otherwise i would maybe have an idea to try out because i have also Customizing of it in progres.

I could think of something like with the other processors.

@OskarStark
Copy link
Contributor Author

OskarStark commented Nov 14, 2024

Hey, my use case is:

In my function I call an API and I get an array with products like:

[
  {
    "name": "foo",
    "url": "/foo",    
  },
  {
    "name": "bar",
    "url": "/bar",    
  }
]

but I want to have dedicated response format, so what I do is, I just give it back as the response to the function call and in the same time I force a response format:

Demo code (this is not how a json schema should look like 😄):

{
  "product_name": string,
  "target_url": string,    
}

So I don't want to do the mapping myself, but the LLM should do it for me.
Again, but only if the answer is from THIS function.

I hope it is clear now

@chr-hertel
Copy link
Member

chr-hertel commented Nov 15, 2024

@OskarStark The issue is that function calls are handled as collection. there can be multiple of them with one model response and this makes it hard to "understand" which response_format we should use, see https://github.com/php-llm/llm-chain/blob/main/src/ToolBox/ChainProcessor.php#L50C1-L55C81

the alternative that i can see is some kind of decorator or processor that isolates the extraction that you want as extra feature.

@OskarStark
Copy link
Contributor Author

Where you able to mange 2 function calls at once ?

@DZunke
Copy link
Contributor

DZunke commented Nov 15, 2024

Ah ok. So kind of what i have done, as i want to customize tool settings like description for the function and description of method parameters at runtime from users application settings, is to create my own toolbox with the help of the toolbox interface and overwrite the Metadata that are responded by the getMap functionality. So the tool box itself is already customizable for own purposes to give back own interpretation of metadata objects.

What i take from your answer and code change is that you want to reach a changed tools mapping from metadata objects to array in some cases?

What my first simple idea was to extend the toolbox itself to build the mapping for the chain processors. This way it could also be possible to have totally different tool definitions beside the attributes that are mapped to metadata objects. This would maybe also shrink the need with all the schema definitions combined within the classes with the simple string behavor as the metadata objects could be left out as a contract between platform and toolbox.

I would understand this as a possibility to have a diverse set of custom interpreations available. Also to build complex json schema communication within the tools with own decisions based on the own use case instead of having the library to decide how to merge those complex JSON Schema together to a valid thingy.

Maybe it should also be possible to have and extended tool box interface with methods for "render map as information which tools are available" and "optionaly render map for tools execution" as Input and Output in ChainProcessor seems target to be different?

My use case is also currently working with the interface in place, as i mentioned, but i had those thoughts in general for an improvement after i had a revisit to this pull request.

What do you think? Maybe i am totally wrong as i am not utilizing the json schema approach and never tried the structured output in LLMs. Just wanted to bring this a bit further with sharing my thoughts.

@chr-hertel
Copy link
Member

@OskarStark this one is obsolete, isn't it?

@OskarStark OskarStark closed this Jan 4, 2025
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.

Feature Idea: Structured Output for Tools
3 participants