feat: add resp format and timeout to conversation api#4129
Conversation
…che works) for convo api Signed-off-by: Samantha Coyle <[email protected]>
The model was the only thing that I can remember that we want bubbled out into the metadata api. I can look to PR that later today or tomorrow :) |
Signed-off-by: Samantha Coyle <[email protected]>
…r ollama Signed-off-by: Samantha Coyle <[email protected]>
…lama Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
| Parameters map[string]*anypb.Any `json:"parameters"` | ||
| ConversationContext string `json:"conversationContext"` | ||
| Temperature float64 `json:"temperature"` | ||
|
|
||
| // from metadata | ||
| Key string `json:"key"` | ||
| Model string `json:"model"` | ||
| Endpoints []string `json:"endpoints"` | ||
| Policy string `json:"loadBalancingPolicy"` |
There was a problem hiding this comment.
The “from metadata” fields were bubbling the Metadata field from the Conversations API (see https://github.com/dapr/dapr/blob/master/dapr/proto/runtime/v1/ai.proto#L43
), which allowed all of these fields to be passed through the request. This is an incorrect use of metadata. Components already expose their own metadata field, so there’s no need to bubble this information up from the API—contrib already handles it.
As a result, we can remove the following fields: Key, Endpoints, and Policy.
I also removed Parameters and ConversationContext, since these were surfaced but never actually used. All of this traces back to the original implementation, which I’m now cleaning up.
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
|
ignore this please as I am WIP splitting this into different PRs |
sicoyle
changed the title
Signed-off-by: Samantha Coyle <[email protected]>
sicoyle
changed the title
sicoyle
changed the title
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
…nents-contrib into feat-convo-api-more-fields
Signed-off-by: Samantha Coyle <[email protected]>
| - name: idleConnectionTimeout | ||
| required: false | ||
| description: | | ||
| Maximum duration an idle HTTP connection can remain open before being closed. | ||
| type: duration | ||
| example: '30s' | ||
| default: '90s' |
There was a problem hiding this comment.
Why would a user want to set this to something other than a default of 30s?
There was a problem hiding this comment.
Users have seen a timeout error where the conversation was ended due to long requests, such as those using Thinking capabilities. Even something like 90s would be low depending on a conversation request. This is needed to directly unblock dapr agent users who see their connections deemed idle and dropped
There was a problem hiding this comment.
Go HTTP transport deems a connection idle only when there is no request-response in progress but there is a open connection with keep alives. The idle connection timeout should not effect the length of conversation result responses as the request-response is active so the connection is not idle.
There was a problem hiding this comment.
thanks for questioning this @JoshVanL . I added this based on timeout issues I heard of but didn't have the full err logs so went with my best guess. I actually think the resiliency policy timeout bubbling over is sufficient for what we need, and if I need anything else then can add another PR in follow up.
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
|
merging even with flakey state.mysql cert test as my PR has no impact on state components at all |
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]> Co-authored-by: Cassie Coyle <[email protected]>
5 participants
Description
This PR adds:
API PR: dapr/dapr#9241
I need this PR merged first to then merge my dapr/dapr one.
I tested conformance test on: mistral, openai, anthropic.
I have a few fixes left for ollama, and aws (this one was broken to begin with...).
Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #[issue number]
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list:
Note: We expect contributors to open a corresponding documentation PR in the dapr/docs repository. As the implementer, you are the best person to document your work! Implementation PRs will not be merged until the documentation PR is opened and ready for review.