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.

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

#42 init router config #48

Merged
merged 13 commits into from
Jan 1, 2024
Merged

#42 init router config #48

merged 13 commits into from
Jan 1, 2024

Conversation

roma-glushko
Copy link
Member

@roma-glushko roma-glushko commented Dec 31, 2023

Inited the routing configuration & restructured the provider codebase to prepare it for the routing logic

Changes:

  • Cleaned up and restructured provider/openai codebase. Separated unified schemas to the api package.
  • Added and exposed the language chat API with the unified request/response schemas. Updated the bruno collection with this request
  • Added an example of client tests
  • Connected Glide API with the underlying model provider (OpenAI client is hardcoded for now)
  • Implemented default value setting for nested nillable config items
  • Implemented provider setting validation on the model item level

Follow-ups

Testing

Glide Chat API is connected to the underlying client:

Screenshot 2024-01-01 at 15 05 44 Screenshot 2024-01-01 at 15 11 40

@roma-glushko roma-glushko self-assigned this Dec 31, 2023
@roma-glushko roma-glushko linked an issue Dec 31, 2023 that may be closed by this pull request
Copy link

codecov bot commented Dec 31, 2023

Codecov Report

Attention: 32 lines in your changes are missing coverage. Please review.

Comparison is base (c62579d) 62.92% compared to head (9b05734) 67.52%.

Files Patch % Lines
pkg/providers/openai/chat.go 76.19% 13 Missing and 2 partials ⚠️
pkg/providers/openai/config.go 60.00% 12 Missing ⚠️
pkg/providers/openai/client.go 79.16% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop      #48      +/-   ##
===========================================
+ Coverage    62.92%   67.52%   +4.60%     
===========================================
  Files            5        8       +3     
  Lines           89      234     +145     
===========================================
+ Hits            56      158     +102     
- Misses          30       65      +35     
- Partials         3       11       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@roma-glushko roma-glushko marked this pull request as draft December 31, 2023 16:07
Copy link
Contributor

@mkrueger12 mkrueger12 left a comment

Choose a reason for hiding this comment

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

The changes so far look quite good. Nice to see all this come together.


resp, err := c.createChatHTTP(r, u)
// TODO: this is suspicious we do zero remapping of OpenAI response and send it back as is.
Copy link
Contributor

Choose a reason for hiding this comment

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

No, I need to update per our discussion of unified response. Will be working on mapping this week.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's good to know then I'm leaving this spot without changes

}

// UnifiedChatResponse defines Glide's Chat Response Schema unified across all language models
type UnifiedChatResponse struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be updated to match unified response in the GEP?

Copy link
Member Author

Choose a reason for hiding this comment

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

The previous UnifiedChatRequest looked like this:

type UnifiedAPIData struct {
	// Model          string                 `json:"model"`
	// APIKey         string                 `json:"api_key"`
	// Params         map[string]interface{} `json:"params"`
	Message        map[string]string      `json:"message"`
	MessageHistory []map[string]string    `json:"messageHistory"`
}

Since that schema is going to be used in Glide API to validate incoming requests, I though it would be better to have it more specific then just map[string]string. So I looked through other context and come up with this definition.

I was not 100% sure it's good for us, so I wanted to double check with you if that make sense.

c.Telemetry.Logger.Info("running createChatHttp")
func (c *Client) createChatRequestSchema(request *schemas.UnifiedChatRequest) *ChatRequest {
// TODO: consider using objectpool to optimize memory allocation
chatRequest := c.chatRequestTemplate // hoping to get a copy of the template
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you have in mind for the template? Is it different from the ChatRequest type above?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have noticed that most of the chat request object values are well-defined in advance on the client initialization ('cause they are passed via configs and nothing changes them afterwards as of now). So I have filled a instance of chat request on the client initialization to just copy it further and change fields that vary like messages.

In any case, it's better to benchmark this code to see if this brings any value (Go supports benchmarking as a special type of tests out of the box which is nit).

@roma-glushko roma-glushko marked this pull request as ready for review January 1, 2024 12:19
@roma-glushko roma-glushko merged commit ae3cf1b into develop Jan 1, 2024
6 checks passed
@roma-glushko roma-glushko deleted the 42-init-router-config branch January 1, 2024 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Init Model Pools
2 participants