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.

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

Added the azure openai proxy resource endpoint #56

Merged
merged 30 commits into from
Sep 26, 2023

Conversation

edwardcqian
Copy link
Contributor

@edwardcqian edwardcqian commented Sep 20, 2023

Implements #55

Added proxy implementation for Azure OpenAI

User can opt to use Azure OpenAI backend updating the following logics:

  • Turn on the Use Azure OpenAI toggle
  • Setting the correct URL and corresponding api-key
  • Adding the model mapping (each OpenAI model should be mapped to a Azure deployment)

No changes required on the frontend.

@CLAassistant
Copy link

CLAassistant commented Sep 20, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@sd2k sd2k left a comment

Choose a reason for hiding this comment

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

We'll need to add some options to the config page to handle this too.

I'm not sure whether we should support multiple providers being configured at the same time... right now I don't see why not though!

OrganizationID string `json:"organizationId"`
URL string `json:"url"`
OrganizationID string `json:"organizationId"`
UseAzure bool `json:"azureOpenAI"`
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be nicer to use an open type here rather than a bool. Something like

type openAIType string

const (
	openAITypeOpenAI openAIType "openai"
	openAITypeAzure  openAIType "azure"
)

above, then

Suggested change
UseAzure bool `json:"azureOpenAI"`
Type openAIType `json:"type"`

in the settings.

(in case in future people want support for other providers which look and act like OpenAI)

@sd2k sd2k linked an issue Sep 22, 2023 that may be closed by this pull request
Copy link
Contributor

@sd2k sd2k left a comment

Choose a reason for hiding this comment

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

This will need rebasing on main where there are some important changes in path handling in stream.go - basically when creating the path we should no longer use the request's path, instead we should just hardcode /chat/completions.

pkg/plugin/stream.go Outdated Show resolved Hide resolved
pkg/plugin/stream.go Outdated Show resolved Hide resolved
pkg/plugin/resources.go Outdated Show resolved Hide resolved
pkg/plugin/resources.go Outdated Show resolved Hide resolved
pkg/plugin/resources.go Outdated Show resolved Hide resolved
pkg/plugin/stream.go Outdated Show resolved Hide resolved
pkg/plugin/stream.go Outdated Show resolved Hide resolved
pkg/plugin/stream.go Outdated Show resolved Hide resolved
pkg/plugin/resources.go Outdated Show resolved Hide resolved
pkg/plugin/resources.go Outdated Show resolved Hide resolved
@edwardcqian
Copy link
Contributor Author

@sd2k Attempted to try handling the proxy errors, but couldn't really find a good working method. currently just collecting the errors and logging.

@sd2k
Copy link
Contributor

sd2k commented Sep 25, 2023

Yeah that is a bit annoying, guess there's not much more we can do? I wonder if anyone else has encountered this while using Go's HTTP proxy...

Looks like there's 1 unhandled error still left btw 🙏

@edwardcqian
Copy link
Contributor Author

Yeah that is a bit annoying, guess there's not much more we can do? I wonder if anyone else has encountered this while using Go's HTTP proxy...

Looks like there's 1 unhandled error still left btw 🙏

Found the last unhandled error

pkg/plugin/stream.go Outdated Show resolved Hide resolved
@sd2k sd2k force-pushed the azure-openai-proxy-resource branch from 164959e to 966a493 Compare September 26, 2023 09:32
@sd2k
Copy link
Contributor

sd2k commented Sep 26, 2023

@edwardcqian I found a way to nicely return early if we can't parse the URL, unmarshal the body, find a matching deployment, etc. Added a few tests too. I think this is good to go now, but could you test it out when you get a chance?

…ully

By wrapping the ReverseProxy structs with our own structs implementing
http.Handler we can return early and send a nicer HTTP response if the
user has misconfigured their app (e.g. by omitting a model->deployment
entry or adding a bad URL).

This also adds a few tests for common cases to make sure we handle such
cases correctly.
@sd2k sd2k force-pushed the azure-openai-proxy-resource branch from 966a493 to 241120a Compare September 26, 2023 09:34
sd2k
sd2k previously approved these changes Sep 26, 2023
@sd2k sd2k dismissed their stale review September 26, 2023 13:14

need to think about the useAzure flag

Copy link
Contributor

@sd2k sd2k left a comment

Choose a reason for hiding this comment

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

Let's get this merged, I'll take a look at switching the useAzure boolean to an open type in a separate PR.

pkg/plugin/stream.go Outdated Show resolved Hide resolved
@edwardcqian edwardcqian merged commit 8e2ebe4 into main Sep 26, 2023
1 check passed
@edwardcqian edwardcqian deleted the azure-openai-proxy-resource branch September 26, 2023 15:14
SandersAaronD pushed a commit that referenced this pull request Oct 27, 2023
Build: Introduce ESM build and Treeshaking
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Azure OpenAI
3 participants