-
Notifications
You must be signed in to change notification settings - Fork 548
feat: add resp format, timeout, model override to conversation api #4129
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?
Conversation
…che works) for convo api Signed-off-by: Samantha Coyle <[email protected]>
CasperGN
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.
@sicoyle does this (or the linked pr) also carry bubbling out the model, tokens etc into the metadata api like we discussed?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
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.