feat: add azure openai support #214

ttys3 · 2023-03-31T09:57:03Z

feat: add azure openai support

~~refactor config:~~
~~1. add WithXXX options, allow construct a config like openai.NewConfig(...Option)~~
~~2. make the config field naming more like the openai python pkg's, this will make us follow py's changes more easy~~
~~3. add new NewConfig method, which will check the config just like the py' pkg (the error message almost the same)~~

resolve #142

codecov · 2023-03-31T10:40:05Z

Codecov Report

Merging #214 (59461ec) into master (bee0656) will increase coverage by 0.92%.
The diff coverage is 92.30%.

❗ Current head 59461ec differs from pull request most recent head 222aa40. Consider uploading reports for the commit 222aa40 to get more accurate results

@@            Coverage Diff             @@
##           master     #214      +/-   ##
==========================================
+ Coverage   72.51%   73.44%   +0.92%     
==========================================
  Files          21       21              
  Lines         593      625      +32     
==========================================
+ Hits          430      459      +29     
- Misses        124      125       +1     
- Partials       39       41       +2

Impacted Files	Coverage Δ
api.go	`73.41% <86.36%> (+2.44%)`	⬆️
config.go	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

tao12345666333

LGTM, there is one authorization that needs to be revised. Also the /chat/completions API does not exist for the current stable API version api-version

api.go

ttys3 · 2023-03-31T11:50:56Z

LGTM, there is one authorization that needs to be revised. Also the /chat/completions API does not exist for the current stable API version api-version

authorization for non stream request just fixed.

about the /chat/completions API, we are using the preview version. I did not aware that -_-

what can we do about the stable version? add a check and just return an not supported error ?

tao12345666333 · 2023-03-31T11:55:40Z

stable API version 2022-12-01
https://github.com/Azure/azure-rest-api-specs/blob/main/specification/cognitiveservices/data-plane/AzureOpenAI/inference/stable/2022-12-01/inference.json

This API includes a prompt field, if we want to support it, we can just deal with this field.

Or we can just declare we only support 2023-03-15-preview api version

ttys3 · 2023-03-31T12:07:15Z

currently, I follow the openai python package's logic just use the api type to detect the auth method like:

yes, this is simple and just works.

if c.config.APIType == APITypeAzure {
// use api-key method
} else {
// user bear token
}

I think maybe it is better to add a AuthMethod option to the config ?

like

type AuthMethod interface {
	SetupAuth(req *http.Request, apiKey string)
}

then impl two common :

type GenericApiKeyAuth struct {
	KeyName string
}

func (a *GenericApiKeyAuth) SetupAuth(req *http.Request, apiKey string) {
	req.Header.Set(a.KeyName, apiKey)
}

type BearTokenAuth struct{}

func (a *BearTokenAuth) SetupAuth(req *http.Request, apiKey string) {
	req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", apiKey))
}

and we can have the default:

var AzureAPIKeyAuth = &GenericApiKeyAuth{KeyName: "api-key"}

var OpenAITokenAuth = &BearTokenAuth{}

ttys3 · 2023-03-31T12:31:56Z

declare we only support 2023-03-15-preview api version

maybe it 's simple just declare we only support 2023-03-15-preview api version ?

after all, /completions is usable ? ( I did not test it for 2022-12-01)

and, even 2023-03-15-preview does not support /audio api

sashabaranov · 2023-03-31T12:37:09Z

Thank you so much for the PR!

sashabaranov · 2023-03-31T12:49:16Z

A couple of general comments:

add WithXXX options, allow construct a config like openai.NewConfig(...Option)

We are actively avoiding this style of configuration as discussed in #79.

make the config field naming more like the openai python pkg's, this will make us follow py's changes more easy

While making this package close to python's would be a good thing, it is less of a priority than not breaking API for existing clients. I don't think renaming Config fields and NewConfig with a returned error is justified given the API non-breakage goal.

ttys3 · 2023-03-31T13:09:58Z

A couple of general comments:

add WithXXX options, allow construct a config like openai.NewConfig(...Option)

We are actively avoiding this style of configuration as discussed in #79.

make the config field naming more like the openai python pkg's, this will make us follow py's changes more easy

While making this package close to python's would be a good thing, it is less of a priority than not breaking API for existing clients. I don't think renaming Config fields and NewConfig with a returned error is justified given the API non-breakage goal.

it's just a kind of common practice, many project use this style. I can accept that, if you don't like it.
if we do not check the error in the Config constructor, then it seems inappropriate to check for configuration errors elsewhere. we may need just ignore the error checking， Leave the blame to the server and have the user check for errors returned by the server ？

ttys3 · 2023-03-31T13:28:28Z

just add error check for CreateChatCompletionStream,

seems there's other thing need fix:

=== RUN   TestCompletionsStreamWrongModel
    stream_test.go:29: CreateCompletion should return ErrCompletionUnsupportedModel, but returned: Post "http://localhost/v1/completions": dial tcp [::1]:80: connect: connection refused
--- FAIL: TestCompletionsStreamWrongModel (0.00s)

sashabaranov · 2023-04-01T16:22:50Z

it's just a kind of common practice, many project use this style. I can accept that, if you don't like it.

Exactly, there are multiple common ways to pass the configuration, this library prefers using a single config struct.

if we do not check the error in the Config constructor, then it seems inappropriate to check for configuration errors elsewhere. we may need just ignore the error checking， Leave the blame to the server and have the user check for errors returned by the server ？

This PR checks for three errors:

Empty API key — I think it's fine not to check this during the configuration
Whether API type is supported – just type checking should be enough here if we use iota-enum for APIType
Empty APIVersion — it might make sense to have a DefaultAzureConfig(...) that ensures all the nitty-gritty details are specified

if cfg.APIKey == "" {
	return ClientConfig{}, fmt.Errorf("api key is required")
}

if _, ok := supportedAPIType[cfg.APIType]; !ok {
	return ClientConfig{}, fmt.Errorf("unsupported API type %s", cfg.APIType)
}

if cfg.APIType == APITypeAzure || cfg.APIType == APITypeAzureAD {
	if cfg.APIVersion == "" {
		return ClientConfig{}, fmt.Errorf("an API version is required for the Azure API type")
	}
}

ttys3 · 2023-04-03T03:22:54Z

OK.
I updated commits. and it is a mininal patch now. it is non-breaking.

try to fix the CreateChatCompletionStream not detect http error code issue, but which cause ErrorAccumulatorWriteErrors tests fail. (I think it's better deal this with other PR. so reverted that commit)

PTAL

… config style

…not pass

sashabaranov

Thank you for re-working this PR, it is much more manageable now! I've added a couple of comments — I wonder what your thoughts are

config.go

sashabaranov · 2023-04-03T10:49:54Z

config.go


-	HTTPClient *http.Client
+	APIType    APIType
+	APIVersion string // required for APITypeAzure or APITypeAzureAD


Maybe we can also pack Azure-related fields into a sub-structure inside the config? That way, it would be more straightforward during configuration:

config.APIVersion = ... config.Engine = ...

vs

config.Azure.APIVersion = ... config.Azure.Engine = ...

If we do this to Azure, do we have to make a special openai config for openai?

https://github.com/openai/openai-python/blob/main/openai/__init__.py#L39

the py default the version to 2023-03-15-preview, we do this the same ?

api_version = ( "2023-03-15-preview" if api_type in ("azure", "azure_ad", "azuread") else None )

done via 84ea881

since this func called DefaultXXX, I think is OK to do this.

sashabaranov · 2023-04-03T13:08:57Z

Excellent, ready to merge once codecov issues are fixed 🙌🏻

…uto trim suffix /

config.go

sashabaranov

Great work, thank you!

ttys3 mentioned this pull request Mar 31, 2023

feature request: Add support for Azure OpenAI Service #142

Closed

tao12345666333 reviewed Mar 31, 2023

View reviewed changes

api.go Outdated Show resolved Hide resolved

ttys3 changed the title ~~feat: add azure openai support, refactor config~~ feat: add azure openai support Apr 3, 2023

ttys3 requested a review from tao12345666333 April 3, 2023 03:27

spatecon approved these changes Apr 3, 2023

View reviewed changes

ttys3 added 13 commits April 3, 2023 17:13

feat: add azure openai support

35ca745

chore: refine config

4ef2708

chore: make config options like the python one

350e961

chore: adjust config struct field order

61287b2

test: fix tests

9f06a7b

style: make the linter happy

41a5ae5

fix: support Azure API Key authentication in sendRequest

8cdf9a8

chore: check error in CreateChatCompletionStream

f5157ea

chore: pass tests

a8a6b7e

chore: try pass tests again

ca1315e

chore: change ClientConfig back due to this lib does not like WithXxx…

5da05cf

… config style

chore: revert fix to CreateChatCompletionStream() due to cause tests …

94438d4

…not pass

chore: at least add some comment about the required fields

37eecb8

sashabaranov reviewed Apr 3, 2023

View reviewed changes

config.go Show resolved Hide resolved

config.go Outdated Show resolved Hide resolved

config.go Show resolved Hide resolved

sashabaranov reviewed Apr 3, 2023

View reviewed changes

chore: re order ClientConfig fields

60cafde

ttys3 force-pushed the azure-openai-support branch from 43c417d to 60cafde Compare April 3, 2023 12:05

ttys3 added 3 commits April 3, 2023 20:08

chore: add DefaultAzure()

006ac90

chore: set default api_version the same as py one "2023-03-15-preview"

84ea881

style: fixup typo

becc1bb

ttys3 added 6 commits April 4, 2023 01:30

test: add api_internal_test.go

f5c822d

style: make lint happy

ee4bd5d

chore: add constant AzureAPIKeyHeader

416dc26

chore: use AzureAPIKeyHeader for api-key header, fix azure base url a…

27be572

…uto trim suffix /

test: add TestAzureFullURL, TestRequestAuthHeader and TestOpenAIFullURL

ebef9a9

test: simplify TestRequestAuthHeader

862ce64

ttys3 requested a review from sashabaranov April 3, 2023 18:12

ttys3 added 2 commits April 4, 2023 02:18

test: refine TestOpenAIFullURL

115595b

chore: refine comments

59461ec

sashabaranov reviewed Apr 3, 2023

View reviewed changes

config.go Outdated Show resolved Hide resolved

feat: DefaultAzureConfig

222aa40

ttys3 requested a review from sashabaranov April 3, 2023 18:30

sashabaranov approved these changes Apr 4, 2023

View reviewed changes

sashabaranov merged commit 8677fb4 into sashabaranov:master Apr 4, 2023

ttys3 deleted the azure-openai-support branch April 6, 2023 17:44

jnytnai0613 mentioned this pull request Apr 17, 2023

Allow for support for Azure OpenAI Services k8sgpt-ai/k8sgpt#117

Closed

sashabaranov mentioned this pull request Apr 24, 2023

improvement client constructor and add option pattern #278

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add azure openai support #214

feat: add azure openai support #214

ttys3 commented Mar 31, 2023 •

edited

Loading

codecov bot commented Mar 31, 2023 •

edited

Loading

tao12345666333 left a comment

ttys3 commented Mar 31, 2023

tao12345666333 commented Mar 31, 2023

ttys3 commented Mar 31, 2023 •

edited

Loading

ttys3 commented Mar 31, 2023 •

edited

Loading

sashabaranov commented Mar 31, 2023

sashabaranov commented Mar 31, 2023 •

edited

Loading

ttys3 commented Mar 31, 2023 •

edited

Loading

ttys3 commented Mar 31, 2023

sashabaranov commented Apr 1, 2023

ttys3 commented Apr 3, 2023 •

edited

Loading

sashabaranov left a comment

sashabaranov Apr 3, 2023

ttys3 Apr 3, 2023 •

edited

Loading

ttys3 Apr 3, 2023

ttys3 Apr 3, 2023

ttys3 Apr 3, 2023

sashabaranov commented Apr 3, 2023

sashabaranov left a comment

feat: add azure openai support #214

feat: add azure openai support #214

Conversation

ttys3 commented Mar 31, 2023 • edited Loading

codecov bot commented Mar 31, 2023 • edited Loading

Codecov Report

tao12345666333 left a comment

Choose a reason for hiding this comment

ttys3 commented Mar 31, 2023

tao12345666333 commented Mar 31, 2023

ttys3 commented Mar 31, 2023 • edited Loading

ttys3 commented Mar 31, 2023 • edited Loading

sashabaranov commented Mar 31, 2023

sashabaranov commented Mar 31, 2023 • edited Loading

ttys3 commented Mar 31, 2023 • edited Loading

ttys3 commented Mar 31, 2023

sashabaranov commented Apr 1, 2023

ttys3 commented Apr 3, 2023 • edited Loading

sashabaranov left a comment

Choose a reason for hiding this comment

sashabaranov Apr 3, 2023

Choose a reason for hiding this comment

ttys3 Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

ttys3 Apr 3, 2023

Choose a reason for hiding this comment

ttys3 Apr 3, 2023

Choose a reason for hiding this comment

ttys3 Apr 3, 2023

Choose a reason for hiding this comment

sashabaranov commented Apr 3, 2023

sashabaranov left a comment

Choose a reason for hiding this comment

ttys3 commented Mar 31, 2023 •

edited

Loading

codecov bot commented Mar 31, 2023 •

edited

Loading

ttys3 commented Mar 31, 2023 •

edited

Loading

ttys3 commented Mar 31, 2023 •

edited

Loading

sashabaranov commented Mar 31, 2023 •

edited

Loading

ttys3 commented Mar 31, 2023 •

edited

Loading

ttys3 commented Apr 3, 2023 •

edited

Loading

ttys3 Apr 3, 2023 •

edited

Loading