-
Notifications
You must be signed in to change notification settings - Fork 28
[WIP] Use interfaces for Tool definitions #57
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
base: main
Are you sure you want to change the base?
[WIP] Use interfaces for Tool definitions #57
Conversation
Thinking about this. |
return jsonschema.For[HiArgs]() | ||
} | ||
|
||
func (h *HiArgs) SetParams(raw json.RawMessage) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would this be any different? In other words, can we drop the interface method and just do this automatically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that some users might want to control the parsing more specifically, so the pure generic version was not flexible enough, it made the assumption that you would always parse the args into the type in this way - and that may not always be the case.
@@ -46,22 +62,25 @@ type ServerTool struct { | |||
// schema may be customized using the [Input] option. | |||
// | |||
// TODO(jba): check that structured content is set in response. | |||
func NewServerTool[In, Out any](name, description string, handler ToolHandlerFor[In, Out], opts ...ToolOption) *ServerTool { | |||
func NewServerTool[In ToolInput, Out ToolOutput](name, description string, handler ToolHandlerFor[In, Out], opts ...ToolOption) *ServerTool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main concern is that this forces all the input and output types to implement the methods.
This would only be worth it if the type is used for more than one tool.
But how likely is that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would only be worth it if the type is used for more than one tool.
It's worth it for being able to define how the parsing and schema generation occurs. An example in the discussion is a request was to be able to order the schema in a specific way to encourage the LLM to provide the arguments in the same order. That kind of flexibility is not possible with the totally generic approach.
Also, for example I could have list GitHub Issues, and Search GitHub Issues, and could provide the same out type for both tools?
Pretty much the entire dance here was about inverting control back to the author to be able to change how the tool is represented directly.
@jba I think supporting both would be totally reasonable, this was my attempt to express the idea and show it concretely and see how we felt. I hope I did a good enough job of imagining how it could be. Not strongly tied to it - just wanted to have something to throw a stick at. My personal goal is that I would like to be able to keep a concrete type in the return value of a handler, so handlers can wrap other handlers and not lose data in between. Too many SDKs assume that tools will always return the value, instead of a type that can serialize the value, but it makes composing new server tools out of existing server tools more difficult. |
Motivation and Context
This PR is an experiment to see how using interfaces could look, so that the
in
andout
types are in control of how their schema is generated, how their params are parsed and how their results are returned.Related discussion and issues:
The primary change is to update
NewServerTool
andToolHandlerFor
to work with interfaces rather than pure generics:Note: this change is not fully fleshed out yet, as of opening this PR it is the minimum change to get everything building, and doesn't finish fleshing out all the ideas that become possible with this change, nor does it fully sell the benefits.
How Has This Been Tested?
All test cases have been updated
Breaking Changes
Yes, this is an extremely breaking change.
Types of changes
Checklist
Additional context