265: unifying roles, setting default as user #269
Conversation
There was a problem hiding this comment.
This is great @tom-fitz 👏
I have left one comment to double check if all providers are compatible with OpenAI roles. If some of them not, we should try to remap Glide roles to the roles they expect.
pkg/providers/cohere/chat.go
Outdated
| @@ -127,7 +127,7 @@ func (c *Client) doChatRequest(ctx context.Context, payload *ChatRequest) (*sche | |||
| "responseId": cohereCompletion.ResponseID, | |||
| }, | |||
| Message: schemas.ChatMessage{ | |||
| Role: "assistant", | |||
| Role: schemas.RoleAssistant, | |||
There was a problem hiding this comment.
Seems like Cohere decided to deviate from user/assistant/system values and they use CHATBOT, SYSTEM, or USER (uppercase) (ref: https://docs.cohere.com/reference/chat).
This makes me think we need a thin mapper here to smooth this discrepancy.
Also, needs to check if other providers stick to OpenAI's version of roles.
There was a problem hiding this comment.
This makes me think we need a thin mapper here to smooth this discrepancy.
@roma-glushko I pushed a mapper here that takes the providerName and the role that should be mapped from. I'd like to get feedback before researching/implementing this to every provider.
Additionally, I have not looked yet, but is there going to be a need to map roles from requests? In this caswe it does not look like the role is coming in from the request from Cohere, but is that situation we want to handle as well?
There was a problem hiding this comment.
I'd like to get feedback before researching/implementing this to every provider.
Sure, let's sync on this not to do unneeded work 👍 And thank you for the example!
When you look at the current structure of the project, I was trying to consolidate all provider-specific stuff into providers/{provideName} modules. The reasoning behind this:
- I did not want to spread that logic across the whole service. There are potentially very large number of providers to integrate with (some projects claim that they support 100+ providers). So these single components that know how to work with each provider can grow potentially super big. Instead, Glide has an abstraction that it uses all over the places and feed it into provider's clients and only provider clients know how to translate it into their specific requests.
- Because the number of providers is huge at some point I think we will offload them into separate packages, create a registry for them and let users build their own instances of Glide with providers (and not only) they support.
That said, how do you feel about having role mappers in the providers/* modules where it's needed? It would be used on Glide -> Provider schema translation.
There was a problem hiding this comment.
@roma-glushko That makes a lot more sense. I was thinking to myself when making that example, a generic mapper will get out of hand pretty quickly. I'll move forward with provider specific mappers. 👍
There was a problem hiding this comment.
@roma-glushko after researching the providers that are listed in the codebase, I found that everyone uses the openai role model with the exception of cohere. Additionally, I found that the azureopenai uses two additional roles tool & function. https://learn.microsoft.com/en-us/azure/ai-services/openai/reference
Is this something you would like handled? If so, how?
There was a problem hiding this comment.
I found that everyone uses the openai role model with the exception of cohere
Excellent, thank you for double checking!
Additionally, I found that the azureopenai uses two additional roles tool & function. https://learn.microsoft.com/en-us/azure/ai-services/openai/reference
No, let's leave that out of the scope of this PR/task. Support for tools is a slightly bigger problem that I'm hopping us to tackle in scope of #246
Adding specific roles and setting the default to
userwhen a new chat is started.Closes #265