Skip to content
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

54 unified response #55

Merged
merged 4 commits into from
Jan 2, 2024
Merged

54 unified response #55

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
75008d0
#54: Refactor UnifiedChatResponse struct and add ProviderResponse and…
mkrueger12 Jan 1, 2024
eaf61ee
#54: Unified response created and tested - passing
mkrueger12 Jan 1, 2024
956c404
#54: lint
mkrueger12 Jan 1, 2024
e879a8d
#54: lint
mkrueger12 Jan 2, 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
26 changes: 20 additions & 6 deletions pkg/api/schemas/language.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,26 @@ type UnifiedChatRequest struct {

// UnifiedChatResponse defines Glide's Chat Response Schema unified across all language models
type UnifiedChatResponse struct {
ID string `json:"id,omitempty"`
Created float64 `json:"created,omitempty"`
Choices []*ChatChoice `json:"choices,omitempty"`
Model string `json:"model,omitempty"`
Object string `json:"object,omitempty"` // TODO: what does this mean "Object"?
Usage Usage `json:"usage,omitempty"`
ID string `json:"id,omitempty"`
Created float64 `json:"created,omitempty"`
Provider string `json:"provider,omitempty"`
Router string `json:"router,omitempty"`
Model string `json:"model,omitempty"`
Cached bool `json:"cached,omitempty"`
ProviderResponse ProviderResponse `json:"provider_response,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Do you think ProviderResponse -> ModelResponse would be a good idea?

}

// ProviderResponse contains data from the chosen provider
type ProviderResponse struct {
ResponseId map[string]string `json:"response_id,omitempty"`
Message ChatMessage `json:"message"`
TokenCount TokenCount `json:"token_count"`
}

type TokenCount struct {
PromptTokens float64 `json:"prompt_tokens"`
ResponseTokens float64 `json:"response_tokens"`
TotalTokens float64 `json:"total_tokens"`
}

// ChatMessage is a message in a chat request.
Expand Down
55 changes: 51 additions & 4 deletions pkg/providers/openai/chat.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"fmt"
"io"
"net/http"
"time"

"glide/pkg/providers/errs"

Expand Down Expand Up @@ -78,14 +79,12 @@ func (c *Client) Chat(ctx context.Context, request *schemas.UnifiedChatRequest)
// Create a new chat request
chatRequest := c.createChatRequestSchema(request)

// TODO: this is suspicious we do zero remapping of OpenAI response and send it back as is.
// Does it really work well across providers?
chatResponse, err := c.doChatRequest(ctx, chatRequest)
if err != nil {
return nil, err
}

if len(chatResponse.Choices) == 0 {
if len(chatResponse.ProviderResponse.Message.Content) == 0 {
return nil, ErrEmptyResponse
}

Expand Down Expand Up @@ -147,8 +146,56 @@ func (c *Client) doChatRequest(ctx context.Context, payload *ChatRequest) (*sche
return nil, errs.ErrProviderUnavailable
}

// Read the response body into a byte slice
bodyBytes, err := io.ReadAll(resp.Body)
if err != nil {
c.telemetry.Logger.Error("failed to read openai chat response", zap.Error(err))
return nil, err
}

// Parse the response JSON
var responseJSON map[string]interface{}

err = json.Unmarshal(bodyBytes, &responseJSON)
if err != nil {
c.telemetry.Logger.Error("failed to parse openai chat response", zap.Error(err))
return nil, err
}

// Parse response
var response schemas.UnifiedChatResponse

return &response, json.NewDecoder(resp.Body).Decode(&response)
var responsePayload schemas.ProviderResponse

var tokenCount schemas.TokenCount

message := responseJSON["choices"].([]interface{})[0].(map[string]interface{})["message"].(map[string]interface{})
Copy link
Member

Choose a reason for hiding this comment

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

@mkrueger12 looking at this code I feel like it's worth defining OpenAI Chat Response schema, map the response into it and then remap into the unified chat response schema

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 I can do that. What advantage does it provide?

Copy link
Member

Choose a reason for hiding this comment

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

@mkrueger12 That would make the implementation more readable at very least and it would give an understanding what the actual provider response might look like, what information it contains in a declarative way 👀

In this specific case, expression like responseJSON["choices"].([]interface{})[0].(map[string]interface{}) means that you are sure that the response has the choices key where you also are sure there is at least one item e.g. [0]. But is this always a case? Because if not I think this code will fail with an exception trying to access that info but in fact getting nil.

If you had a validation via the defined OpenAI chat response, you would ensure that the response has all needed fields which would simply the code here.

Something like that.

messageStruct := schemas.ChatMessage{
Role: message["role"].(string),
Content: message["content"].(string),
}

tokenCount = schemas.TokenCount{
PromptTokens: responseJSON["usage"].(map[string]interface{})["prompt_tokens"].(float64),
ResponseTokens: responseJSON["usage"].(map[string]interface{})["completion_tokens"].(float64),
TotalTokens: responseJSON["usage"].(map[string]interface{})["total_tokens"].(float64),
}

responsePayload = schemas.ProviderResponse{
ResponseId: map[string]string{"system_fingerprint": responseJSON["system_fingerprint"].(string)},
Message: messageStruct,
TokenCount: tokenCount,
}

response = schemas.UnifiedChatResponse{
ID: responseJSON["id"].(string),
Created: float64(time.Now().Unix()),
Copy link
Member

Choose a reason for hiding this comment

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

Is it in UTC timezone?

Provider: "openai",
Router: "chat",
Model: responseJSON["model"].(string),
Cached: false,
ProviderResponse: responsePayload,
}

return &response, nil
}
3 changes: 2 additions & 1 deletion pkg/providers/openai/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@ import (

"glide/pkg/api/schemas"

"github.com/stretchr/testify/require"
"glide/pkg/telemetry"

"github.com/stretchr/testify/require"
)

func TestOpenAIClient_ChatRequest(t *testing.T) {
Expand Down
Loading