29 setup openai client #34
Conversation
.gitignore
Outdated
| bin | ||
| glide | ||
| tmp | ||
| coverage.txt | ||
| precommit.txt | ||
| openai_test.go |
There was a problem hiding this comment.
Should we ignore that test file? 🤔
There was a problem hiding this comment.
Yeah... I was trying too but got lazy haha
pkg/providers/openai/openaiclient.go
Outdated
| // Returns: | ||
| // - *Client: A pointer to the created client. | ||
| // - error: An error if the client creation failed. | ||
| func Client(poolName string, modelName string, payload []byte) (*ProviderClient, error) { |
There was a problem hiding this comment.
@mkrueger12 OpenAI client (and any other client clients) should not know about the model pooling we do which one level above them. So as I have set it up in the prev PR (https://github.com/modelgateway/glide/pull/30/files#diff-c2b4b6c5026c2afccc50ebcfe652abb63f0456fc1c389652adabe82a0a9d983fR1), the model pooling is kinda another unconnected set of components that are going to use provider clients, but clients should know anything about pool (why should they?).
pkg/providers/openai/chat.go
Outdated
|
|
||
| // Send the chat request | ||
|
|
||
| slog.Info("sending chat request") |
There was a problem hiding this comment.
Feel free to pull logging setup from the recent PR #30 at some point.
pkg/providers/openai/chat.go
Outdated
| // Returns: | ||
| // - *ChatResponse: a pointer to a ChatResponse | ||
| // - error: An error if the request failed. | ||
| func (c *ProviderClient) Chat() (*ChatResponse, error) { |
There was a problem hiding this comment.
If our chat method looks this way e.g. takes no args how would we feed it users request with unified chat data?
| func (c *ProviderClient) Chat() (*ChatResponse, error) { | |
| func (c *ProviderClient) Chat() (*ChatResponse, error) { |
There was a problem hiding this comment.
Client is instantiated with the unified data and the chat method access that data when called.
There was a problem hiding this comment.
@mkrueger12 We would to avoid massive struct creation, so I would have one client that serves all requests with given configs. So we allocate memory for it once and it's working without further memory allocations.
The client is simply inited with a config from the corresponding model pool it represents at service intialization and then we just call it methods with passing the unified schema for further processing.
Besides saving some time not allocating memory, this would help us to keep state like connection pooling, possibly resiliency related things.
Does it make sense? 👀
There was a problem hiding this comment.
I kind of understand. So it would look something like this:
newClient = Client(configFromPool) // what does this config look like? Is it from the config file?
resp, err = newClient.Chat(UnifiedAPIData) // this data matches discussion in GEPThere was a problem hiding this comment.
@mkrueger12 yes, that's where we should probably stop at for this PR. It's going to be wrapped into the pooling logic in the follow-up PRs.
|
|
||
| // CreateChatResponse creates chat Response. | ||
| func (c *ProviderClient) CreateChatResponse(ctx context.Context, r *ChatRequest) (*ChatResponse, error) { | ||
| _ = ctx // keep this for future use |
There was a problem hiding this comment.
Yes, this would be useful to implement request timeouts.
pkg/providers/openai/chat.go
Outdated
| return resp, err | ||
| } | ||
|
|
||
| func (c *ProviderClient) CreateChatRequest(message []byte) *ChatRequest { |
There was a problem hiding this comment.
The provider client should not be operating on raw bytes. It should be operating on the unified request schema.
Here is the flow:
User/Client -> Glide API (Hertz) -(dispatch a handler) -> Validate and Deserialize the incoming request to the unified API -(pass the unified request schema) -> Router/The Target Pool -(pass the unified schema)-> Provider Client
Then, the provider client does a request to the provider & receives a response. It maps the provider response to the unified response schema and returns all the way back to the user/client.
Do you have a different workflow in mind?
There was a problem hiding this comment.
Nope this workflow makes sense
There was a problem hiding this comment.
I've update the response schema in the GEP for your review
pkg/providers/openai/openaiclient.go
Outdated
| // Returns: | ||
| // - *Client: A pointer to the created client. | ||
| // - error: An error if the client creation failed. | ||
| func Client(poolName string, modelName string, payload []byte) (*ProviderClient, error) { |
There was a problem hiding this comment.
I feel like I have commented about this in a different places, but the unified API schemas should allow us to do validation & deserialization of incoming user request on the API handler level, so the rest of the system would work with that structure.
This way we decouple the schema receiver from the way that schema is processed leaving a way for many receivers e.g. HTTP REST, gRPC, what have you.
No description provided.