openai.DefaultAzureConfig support configure apiVersion

[fregie]

fix #323

Azure OpenAI ChatGPT

ChatCompletion error: error, status code: 404, message: The API deployment for this resource does not exist. If you created the deployment within the last 5 minutes, please wait a moment and try again.

[codecov]

Codecov Report

Merging #414 (dabdbb0) into master (581f70b) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@ Coverage Diff @@ ## master #414 +/- ## ========================================== + Coverage 96.95% 96.99% +0.03%  ========================================== Files 17 17 Lines 690 698 +8 ========================================== + Hits 669 677 +8  Misses 15 15 Partials 6 6 

Impacted Files	Coverage Δ
config.go	91.66% <100.00%> (+2.38%)	⬆️

[vvatanabe]

@fregie That's a kind comment. 👍

[vvatanabe]

@fregie The functional options approach you're proposing is really slick! You might already know this, but you can also change the APIVersion with the existing code below. Do you still think it's better to expand the DefaultAzureConfig function with functional options?

cfg := openai.DefaultAzureConfig("apiKey", "baseURL") cfg.APIVersion = "APIVersion"

The responsibility of the DefaultAzureConfig function is really just to return a config with default parameters. Ideally, I'd like to keep things simple and stick to the single responsibility principle.

[fregie]

I suggest to add the APIVersion para to DefaultAzureConfig because I don't think it should be a default configure.
It seems like the APIVersion is not a fixed value, the default value of this repo 2023-05-15 maybe not work in some deployment of azure openai.
I think the default configure should work at least.
This is just my thought,and I don't think this is a big deal,Just go ahead and do it your way.

[vvatanabe]

It seems like the APIVersion is not a fixed value, the default value of this repo 2023-05-15 maybe not work in some deployment of azure openai.

@fregie I see, I understand. So, the APIVersion is not optional but required. In that case, I thought the following signature would be appropriate.

func DefaultAzureConfig(apiKey, baseURL, apiVersion string) ClientConfig 

@sashabaranov
Since it involves breaking changes, I would like to incorporate this change into the plan for v2.0.0 (major update). I would like to bundle and release other improvements involving breaking changes in v2.0.0 as well, for example, the issue with the default value of Temperature.

[sashabaranov]

Folks, I just wanted to add my two cents here:

• Historically, we followed the pattern that @vvatanabe listed above: create cfg struct and set its fields. We consistently kept this pattern and never introduced a setters-based approach. See Better configuration #79
• The optional list of setters (e.g., WithAzureAPIVersion) is powerful and expressive. See, for example, refactoring f1b6696 that @vvatanabe made recently
• I feel like the consistency is important — we either need to keep the current struct-based pattern, or introduce With* setters for all configs, so it would be possible to do both:

config := openai.DefaultConfig("auth token") config.BaseURL = "my URL" config.OrgID = "my org id" config.EmptyMessagesLimit = 10000

and

config := openai.DefaultConfig("auth token", openai.WithBaseURL("my URL"), openai.WithOrgID("my org id"), openai.WithEmptyMessagesLimit(10000), )

My take is that we probably don't need With* setters because:

• It would introduce extra code that is not 100% needed
• It's easier to read a doc for a single ClientConfig struct than trying to search through a multitude of With-setters
• In cases like Azure apiVersion, we can provide a good example in README indicating that you need to care about this apiVersion parameter
• If the default apiVersion becomes a no-go — I think it's best to require this parameter in DefaultAzureConfig and deprecate the current 2-parameter method