-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add stateless, streaming and async tool annotations #489
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 stateless, streaming and async tool annotations #489
Conversation
33ac31d to
90ce94d
Compare
docs/docs/concepts/tools.mdx
Outdated
| | `destructiveHint` | boolean | true | If true, the tool may perform destructive updates (only meaningful when `readOnlyHint` is false) | | ||
| | `idempotentHint` | boolean | false | If true, calling the tool repeatedly with the same arguments has no additional effect (only meaningful when `readOnlyHint` is false) | | ||
| | `openWorldHint` | boolean | true | If true, the tool may interact with an "open world" of external entities | | ||
| | `statelessHint` | boolean | true | If true, this tool does not maintain state based on previous requests | |
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.
Does it make sense for the default to be true? I think it should either be undefined or false, as it's not possible to maintain correctness for existing tools if the default is true - as a trivial example, we can't suggest that filesystem tools are stateless just because they don't declare otherwise.
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.
Or, alternatively - rename it to statefulHint instead of statelessHint, and update the description accordingly.
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 think you are right. This should be
falseby default, since being marked stateful is more forgiving (based on description, if statelessHint is false the tool might maintain state of the previous interactions). Will change this. - I would like to stick with
statelessHintbecause we usestatelessin server session as well. Would prefer to avoid confusion by having both stateful and stateless terms.
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.
- Err, other way around - you have it
trueby default here already, as in: "assume this tool is stateless". Making itfalseby default would be "assume this tool is stateful", unless I'm misinterpreting the description. - That's reasonable, though I think the double-negative in something potentially not being stateless is just as confusing (I think we're seeing that in this very discussion) 😅
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.
- Corrected my comment. Meant
falseby default. - I do agree about the double negative. However, since the current decision is to use stateless for the server session with False as the default value, I think we should stick with it for the tool as well.
LucaButBoring
left a comment
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.
Comments resolved, will cede the floor to maintainers 😄
|
This change looks good to me - should we wait on async hint until that capability is added? |
I think having it in there preemptively is not bad either. asyncHint can be set to false for all tools until the support has been added (I see async support as inevitable). I am fine with waiting but there are tools that could currently benefit from statelessHint and streamingHint annotations. |
4a2c177 to
7d4f467
Compare
|
This change would now require an SEP. Your summary says "client can make use of these annotations when deciding which tool to use", but it's not clear to me exactly how the client would behave differently when presented with these annotations, so you may want to discuss that in your Rationale. Also, depending on how specialized a client must be to benefit from these annotations, there is an argument for implementing them as (unofficial) fields in |
|
Agreed. Will rethink this idea, keeping in mind the new SEPs being prioritized, and will create a SEP for it if it still makes sense. |
Motivation and Context
This change adds 3 new tool annotations: stateless, streaming and asynchronous, that would be useful to convey the impact and nature of the tools to the client. Based on discussion #344. The client can make use of these annotations when deciding which tool to use:
statelessHint: If true, this tool does not maintain state based on previous requests. If false, the tool may maintain state base on previous interactions and the context from the previous requests might influence the next response.streamingHint: If true, this tool streams its responses.asyncHint: If true, this tool accepts a request and continues to work asynchronously to generate responses.Breaking Changes
None
Types of changes
Checklist