Skip to content

265: unifying roles, setting default as user #269

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.

Sign up for GitHub

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

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
Open

265: unifying roles, setting default as user #269

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
94d0ea5
265: unifying roles, setting default as user
tom-fitz Jun 11, 2024
5f6c31f
265: fix missing assistant role updates
tom-fitz Jun 11, 2024
c6b4650
265: updating swagger docs
tom-fitz Jun 11, 2024
a491e62
265: creating mapper for interal role to provider role
tom-fitz Jun 13, 2024
3d1bf87
265: adding tests for role mapping function
tom-fitz Jun 13, 2024
0b2c8c6
265: handling role mapping for cohere provider. adding in tests
tom-fitz Jun 14, 2024
ed57b3e
256: fixing tests
tom-fitz Jun 14, 2024
516457b
265: removing unused role mapping func. handling role set from payloa…
tom-fitz Jun 15, 2024
201ef76
265: pushing wip. Need clarification on role response logic
tom-fitz Jun 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion docs/docs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,11 @@ const docTemplate = `{
},
"role": {
"description": "The role of the author of this message. One of system, user, or assistant.",
"type": "string"
"allOf": [
{
"$ref": "#/definitions/schemas.Role"
}
]
}
}
},
Expand Down Expand Up @@ -308,6 +312,19 @@ const docTemplate = `{
}
}
},
"schemas.Role": {
"type": "string",
"enum": [
"system",
"user",
"assistant"
],
"x-enum-varnames": [
"RoleSystem",
"RoleUser",
"RoleAssistant"
]
},
"schemas.RouterListSchema": {
"type": "object",
"properties": {
Expand Down
19 changes: 18 additions & 1 deletion docs/swagger.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,11 @@
},
"role": {
"description": "The role of the author of this message. One of system, user, or assistant.",
"type": "string"
"allOf": [
{
"$ref": "#/definitions/schemas.Role"
}
]
}
}
},
Expand Down Expand Up @@ -305,6 +309,19 @@
}
}
},
"schemas.Role": {
"type": "string",
"enum": [
"system",
"user",
"assistant"
],
"x-enum-varnames": [
"RoleSystem",
"RoleUser",
"RoleAssistant"
]
},
"schemas.RouterListSchema": {
"type": "object",
"properties": {
Expand Down
13 changes: 12 additions & 1 deletion docs/swagger.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,10 @@ definitions:
description: The content of the message.
type: string
role:
allOf:
- $ref: '#/definitions/schemas.Role'
description: The role of the author of this message. One of system, user,
or assistant.
type: string
required:
- content
- role
Expand Down Expand Up @@ -77,6 +78,16 @@ definitions:
token_usage:
$ref: '#/definitions/schemas.TokenUsage'
type: object
schemas.Role:
enum:
- system
- user
- assistant
type: string
x-enum-varnames:
- RoleSystem
- RoleUser
- RoleAssistant
schemas.RouterListSchema:
properties:
routers:
Expand Down
12 changes: 10 additions & 2 deletions pkg/api/schemas/chat.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func (r *ChatRequest) Params(modelID string, modelName string) *ChatParams {
func NewChatFromStr(message string) *ChatRequest {
return &ChatRequest{
Message: ChatMessage{
"user",
RoleUser,
message,
},
}
Expand Down Expand Up @@ -93,10 +93,18 @@ type TokenUsage struct {
TotalTokens int `json:"total_tokens"`
}

type Role string

const (
RoleSystem Role = "system"
RoleUser Role = "user"
RoleAssistant Role = "assistant"
)

// ChatMessage is a message in a chat request.
type ChatMessage struct {
// The role of the author of this message. One of system, user, or assistant.
Role string `json:"role" validate:"required"`
Role Role `json:"role" validate:"required"`
// The content of the message.
Content string `json:"content" validate:"required"`
}
2 changes: 1 addition & 1 deletion pkg/api/schemas/chat_stream.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func NewChatStreamFromStr(message string) *ChatStreamRequest {
return &ChatStreamRequest{
ChatRequest: &ChatRequest{
Message: ChatMessage{
"user",
RoleUser,
message,
},
},
Expand Down
18 changes: 9 additions & 9 deletions pkg/api/schemas/chat_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func TestChatRequest_DefaultParams(t *testing.T) {

chatReq := ChatRequest{
Message: ChatMessage{
Role: "user",
Role: RoleUser,
Content: defaultMessage,
},
MessageHistory: []ChatMessage{
Expand All @@ -42,7 +42,7 @@ func TestChatRequest_DefaultParams(t *testing.T) {
OverrideParams: &map[string]ModelParamsOverride{
modelID: {
Message: ChatMessage{
Role: "user",
Role: RoleUser,
Content: myModelMessage,
},
},
Expand All @@ -66,7 +66,7 @@ func TestChatRequest_ModelIDOverride(t *testing.T) {

chatReq := ChatRequest{
Message: ChatMessage{
Role: "user",
Role: RoleUser,
Content: defaultMessage,
},
MessageHistory: []ChatMessage{
Expand All @@ -78,7 +78,7 @@ func TestChatRequest_ModelIDOverride(t *testing.T) {
OverrideParams: &map[string]ModelParamsOverride{
modelID: {
Message: ChatMessage{
Role: "user",
Role: RoleUser,
Content: myModelMessage,
},
},
Expand All @@ -102,7 +102,7 @@ func TestChatRequest_ModelNameOverride(t *testing.T) {

chatReq := ChatRequest{
Message: ChatMessage{
Role: "user",
Role: RoleUser,
Content: defaultMessage,
},
MessageHistory: []ChatMessage{
Expand All @@ -114,7 +114,7 @@ func TestChatRequest_ModelNameOverride(t *testing.T) {
OverrideParams: &map[string]ModelParamsOverride{
modelName: {
Message: ChatMessage{
Role: "user",
Role: RoleUser,
Content: myModelMessage,
},
},
Expand All @@ -139,7 +139,7 @@ func TestChatRequest_ModelNameIDOverride(t *testing.T) {

chatReq := ChatRequest{
Message: ChatMessage{
Role: "user",
Role: RoleUser,
Content: defaultMessage,
},
MessageHistory: []ChatMessage{
Expand All @@ -151,13 +151,13 @@ func TestChatRequest_ModelNameIDOverride(t *testing.T) {
OverrideParams: &map[string]ModelParamsOverride{
modelName: {
Message: ChatMessage{
Role: "user",
Role: RoleUser,
Content: myModelNameMessage,
},
},
modelID: {
Message: ChatMessage{
Role: "user",
Role: RoleUser,
Content: myModelIDMessage,
},
},
Expand Down
2 changes: 1 addition & 1 deletion pkg/providers/anthropic/chat.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ func (c *Client) doChatRequest(ctx context.Context, payload *ChatRequest) (*sche
ModelResponse: schemas.ModelResponse{
Metadata: map[string]string{},
Message: schemas.ChatMessage{
Role: completion.Type,
Role: schemas.Role(completion.Type),
Content: completion.Text,
},
TokenUsage: schemas.TokenUsage{
Expand Down
4 changes: 2 additions & 2 deletions pkg/providers/azureopenai/chat_stream_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ func TestAzureOpenAIClient_ChatStreamRequest(t *testing.T) {
require.NoError(t, err)

chatParams := schemas.ChatParams{Messages: []schemas.ChatMessage{{
Role: "user",
Role: schemas.RoleUser,
Content: "What's the capital of the United Kingdom?",
}}}

Expand Down Expand Up @@ -140,7 +140,7 @@ func TestAzureOpenAIClient_ChatStreamRequestInterrupted(t *testing.T) {
require.NoError(t, err)

chatParams := schemas.ChatParams{Messages: []schemas.ChatMessage{{
Role: "user",
Role: schemas.RoleUser,
Content: "What's the biggest animal?",
}}}

Expand Down
4 changes: 2 additions & 2 deletions pkg/providers/azureopenai/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func TestAzureOpenAIClient_ChatRequest(t *testing.T) {
require.NoError(t, err)

chatParams := schemas.ChatParams{Messages: []schemas.ChatMessage{{
Role: "user",
Role: schemas.RoleUser,
Content: "What's the capital of the United Kingdom?",
}}}

Expand Down Expand Up @@ -116,7 +116,7 @@ func TestDoChatRequest_ErrorResponse(t *testing.T) {
require.NoError(t, err)

chatParams := schemas.ChatParams{Messages: []schemas.ChatMessage{{
Role: "user",
Role: schemas.RoleUser,
Content: "What's the dealio?",
}}}

Expand Down
2 changes: 1 addition & 1 deletion pkg/providers/bedrock/chat.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ func (c *Client) doChatRequest(ctx context.Context, payload *ChatRequest) (*sche
Cached: false,
ModelResponse: schemas.ModelResponse{
Message: schemas.ChatMessage{
Role: "assistant",
Role: schemas.RoleAssistant,
Content: modelResult.OutputText,
},
TokenUsage: schemas.TokenUsage{
Expand Down
2 changes: 1 addition & 1 deletion pkg/providers/bedrock/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func TestBedrockClient_ChatRequest(t *testing.T) {
require.NoError(t, err)

chatParams := schemas.ChatParams{Messages: []schemas.ChatMessage{{
Role: "user",
Role: schemas.RoleUser,
Content: "What's the biggest animal?",
}}}

Expand Down
2 changes: 1 addition & 1 deletion pkg/providers/bedrock/testdata/chat.req.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"model": "amazon.titan-text-express-v1",
"messages": [
{
"role": "user",
"role": schemas.RoleUser,
"content": "What's the biggest animal?"
}
],
Expand Down
2 changes: 1 addition & 1 deletion pkg/providers/cohere/chat.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tom-fitz

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tom-fitz

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

Content: cohereCompletion.Text,
},
TokenUsage: schemas.TokenUsage{
Expand Down
2 changes: 1 addition & 1 deletion pkg/providers/octoml/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ func TestDoChatRequest_ErrorResponse(t *testing.T) {

// Create a chat request payload
chatParams := schemas.ChatParams{Messages: []schemas.ChatMessage{{
Role: "user",
Role: schemas.RoleUser,
Content: "What's the dealeo?",
}}}

Expand Down
2 changes: 1 addition & 1 deletion pkg/providers/ollama/chat.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ func (c *Client) doChatRequest(ctx context.Context, payload *ChatRequest) (*sche
Cached: false,
ModelResponse: schemas.ModelResponse{
Message: schemas.ChatMessage{
Role: ollamaCompletion.Message.Role,
Role: schemas.Role(ollamaCompletion.Message.Role),
Content: ollamaCompletion.Message.Content,
},
TokenUsage: schemas.TokenUsage{
Expand Down
8 changes: 4 additions & 4 deletions pkg/providers/ollama/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func TestOllamaClient_ChatRequest(t *testing.T) {
require.NoError(t, err)

chatParams := schemas.ChatParams{Messages: []schemas.ChatMessage{{
Role: "user",
Role: schemas.RoleUser,
Content: "What's the biggest animal?",
}}}

Expand Down Expand Up @@ -85,7 +85,7 @@ func TestOllamaClient_ChatRequest_Non200Response(t *testing.T) {
require.NoError(t, err)

chatParams := schemas.ChatParams{Messages: []schemas.ChatMessage{{
Role: "user",
Role: schemas.RoleUser,
Content: "What's the capital of the United Kingdom?",
}}}

Expand Down Expand Up @@ -122,14 +122,14 @@ func TestOllamaClient_ChatRequest_SuccessfulResponse(t *testing.T) {
require.NoError(t, err)

chatParams := schemas.ChatParams{Messages: []schemas.ChatMessage{{
Role: "user",
Role: schemas.RoleUser,
Content: "What's the capital of the United Kingdom?",
}}}

response, err := client.Chat(context.Background(), &chatParams)

require.NoError(t, err)
require.NotNil(t, response)
require.Equal(t, "assistant", response.ModelResponse.Message.Role)
require.Equal(t, schemas.RoleAssistant, response.ModelResponse.Message.Role)
require.Equal(t, "London", response.ModelResponse.Message.Content)
}
2 changes: 1 addition & 1 deletion pkg/providers/openai/chat_stream.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ func (s *ChatStream) Recv() (*schemas.ChatStreamChunk, error) {
"generated_at": completionChunk.Created,
},
Message: schemas.ChatMessage{
Role: "assistant", // doesn't present in all chunks
Role: schemas.RoleAssistant, // doesn't present in all chunks
Content: responseChunk.Delta.Content,
},
},
Expand Down
4 changes: 2 additions & 2 deletions pkg/providers/openai/chat_stream_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ func TestOpenAIClient_ChatStreamRequest(t *testing.T) {
require.NoError(t, err)

chatParams := schemas.ChatParams{Messages: []schemas.ChatMessage{{
Role: "user",
Role: schemas.RoleUser,
Content: "What's the capital of the United Kingdom?",
}}}

Expand Down Expand Up @@ -140,7 +140,7 @@ func TestOpenAIClient_ChatStreamRequestInterrupted(t *testing.T) {
require.NoError(t, err)

chatParams := schemas.ChatParams{Messages: []schemas.ChatMessage{{
Role: "user",
Role: schemas.RoleUser,
Content: "What's the capital of the United Kingdom?",
}}}

Expand Down
2 changes: 1 addition & 1 deletion pkg/providers/openai/chat_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func TestOpenAIClient_ChatRequest(t *testing.T) {
require.NoError(t, err)

chatParams := schemas.ChatParams{Messages: []schemas.ChatMessage{{
Role: "user",
Role: schemas.RoleUser,
Content: "What's the capital of the United Kingdom?",
}}}

Expand Down