change azure engine config to modelMapper #306
Conversation
Codecov Report
@@ Coverage Diff @@
## master #306 +/- ##
==========================================
+ Coverage 92.52% 92.63% +0.11%
==========================================
Files 22 22
Lines 669 679 +10
==========================================
+ Hits 619 629 +10
Misses 37 37
Partials 13 13
|
|
Wow, thank you so much for such a great work! |
client.go
Outdated
| // fullURL returns full URL for request. | ||
| // If API type is Azure, model is required to get deployment name. | ||
| // If API type is OpenAI, model will be ignored. | ||
| func (c *Client) fullURL(suffix, model string) string { |
There was a problem hiding this comment.
As a suggestion: do we want to use variadic options here instead of the required model argument? That would require fewer changes to other parts of the library. https://gobyexample.com/variadic-functions
We didn't adopt this pattern for the whole library in #278, but it seems useful in the context of the fullURL method.
What do you think?
There was a problem hiding this comment.
I agree, I would be happy to do this job.
config.go
Outdated
| @@ -50,14 +50,21 @@ func DefaultConfig(authToken string) ClientConfig { | |||
| } | |||
| } | |||
|
|
|||
| func DefaultAzureConfig(apiKey, baseURL, engine string) ClientConfig { | |||
| func DefaultAzureConfig(apiKey, baseURL string, modelMapper map[string]string) ClientConfig { | |||
There was a problem hiding this comment.
I suggest we remove modelMapper argument here as it doesn't really fit the "default" config. Let's allow modifying it afterward.
config.go
Outdated
| func DefaultAzureConfig(apiKey, baseURL string, modelMapper map[string]string) ClientConfig { | ||
| if len(modelMapper) == 0 { | ||
| modelMapper = map[string]string{ | ||
| GPT3Dot5Turbo0301: "gpt-35-turbo-0301", |
There was a problem hiding this comment.
What if modelMapper would be just a function that user can provide?
There was a problem hiding this comment.
Your suggestion is great, I tried to make those changes, the code become more concise and has better extensibility. I will submit the new code later.
|
Please review the code，thx |
|
It's not compatible, it should not release in patch version! Ref: |
"azure engine" refers to the deployment name, and each deployment name can only deploy one model.
This means that if I use multiple models, I need to configure multiple engines.
Since a client can only configure one engine, I have to maintain many clients, which is not easy.
I submitted a pull request that uses a modelMapper to map the model to the deployment name. By default, the Azure deployment name uses the model name, it can work without any engine configuration.