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

Jump to bottom

openai.DefaultAzureConfig support configure apiVersion #414

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

openai.DefaultAzureConfig support configure apiVersion #414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 10 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,11 @@ func main() {
// return azureModelMapping[model]
// }

// If you met the error "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."
// You can set the APIVersion to the correct version by:
// config := openai.DefaultAzureConfig("your Azure OpenAI Key", "https://your Azure OpenAI Endpoint", openai.WithAzureAPIVersion("2021-03-01-preview"))
// find the corrent version in Azure AI Studio -> Chat playgrount -> Chat session -> view Code

Comment on lines +447 to +451
Copy link
Collaborator

@vvatanabe vvatanabe Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@fregie That's a kind comment. 👍

client := openai.NewClientWithConfig(config)
resp, err := client.CreateChatCompletion(
context.Background(),
Expand Down Expand Up @@ -494,6 +499,11 @@ func main() {
// return azureModelMapping[model]
//}

// If you met the error "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."
// You can set the APIVersion to the correct version by:
// config := openai.DefaultAzureConfig("your Azure OpenAI Key", "https://your Azure OpenAI Endpoint", openai.WithAzureAPIVersion("2021-03-01-preview"))
// find the corrent version in Azure AI Studio -> Chat playgrount -> Chat session -> view Code

input := "Text to vectorize"

client := openai.NewClientWithConfig(config)
Expand Down
19 changes: 16 additions & 3 deletions config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const (

azureAPIPrefix = "openai"
azureDeploymentsPrefix = "deployments"
azureDefaultAPIVersion = "2023-05-15"
)

type APIType string
Expand Down Expand Up @@ -50,13 +51,21 @@ func DefaultConfig(authToken string) ClientConfig {
}
}

func DefaultAzureConfig(apiKey, baseURL string) ClientConfig {
return ClientConfig{
type AzureConfigOption func(*ClientConfig)

func WithAzureAPIVersion(apiVersion string) AzureConfigOption {
return func(c *ClientConfig) {
c.APIVersion = apiVersion
}
}

func DefaultAzureConfig(apiKey, baseURL string, opts ...AzureConfigOption) ClientConfig {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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

c := ClientConfig{
authToken: apiKey,
BaseURL: baseURL,
OrgID: "",
APIType: APITypeAzure,
APIVersion: "2023-05-15",
APIVersion: azureDefaultAPIVersion,
AzureModelMapperFunc: func(model string) string {
return regexp.MustCompile(`[.:]`).ReplaceAllString(model, "")
},
Expand All @@ -65,6 +74,10 @@ func DefaultAzureConfig(apiKey, baseURL string) ClientConfig {

EmptyMessagesLimit: defaultEmptyMessagesLimit,
}
for _, opt := range opts {
opt(&c)
}
return c
}

func (ClientConfig) String() string {
Expand Down
7 changes: 7 additions & 0 deletions config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,3 +60,10 @@ func TestGetAzureDeploymentByModel(t *testing.T) {
})
}
}

func TestSetApiVersion(t *testing.T) {
conf := DefaultAzureConfig("", "https://test.openai.azure.com/", WithAzureAPIVersion("2021-03-01-preview"))
if conf.APIVersion != "2021-03-01-preview" {
t.Errorf("Expected %s, got %s", "2021-03-01-preview", conf.APIVersion)
}
}