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

Add Azure OpenAI support #18

Merged
merged 1 commit into from
May 16, 2023
Merged

Add Azure OpenAI support #18

merged 1 commit into from
May 16, 2023

Conversation

ugwun
Copy link
Contributor

@ugwun ugwun commented May 14, 2023

No description provided.

Copy link
Owner

@CJCrafter CJCrafter left a comment

Choose a reason for hiding this comment

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

Everything looks good, and I appreciate you adding those test classes for people to refer to.

But I am thinking about how people may implement in this in their apps. One of their clients may want to have an option in the app to switch the endpoint to their azure endpoint, or use the default openai endpoint.

In this case, it may be easier to just have a

private val endpoint: String = "https://api.openai.com/v1/"

in the constructor. And since we now have so many default variables, it may be better to have a builder class like I've been doing with ChatRequest#builder() and CompletionRequest#builder(). Let me know what you think, and I'll make those changes.

@ugwun
Copy link
Contributor Author

ugwun commented May 14, 2023

Something like this?

 class Builder {
        private var apiKey: String = ""
        private var organization: String? = null
        private var client: OkHttpClient = OkHttpClient()
        private var azureBaseUrl: String = ""
        private var apiVersion: String = "2023-03-15-preview"
        private var modelName: String = ""

        fun apiKey(apiKey: String) = apply { this.apiKey = apiKey }

        fun organization(organization: String?) = apply { this.organization = organization }

        fun client(client: OkHttpClient) = apply { this.client = client }

        fun azureBaseUrl(azureBaseUrl: String) = apply { this.azureBaseUrl = azureBaseUrl }

        fun apiVersion(apiVersion: String) = apply { this.apiVersion = apiVersion }

        fun modelName(modelName: String) = apply { this.modelName = modelName }

        fun build(): AzureOpenAI {
            return AzureOpenAI(
                apiKey = apiKey,
                organization = organization,
                client = client,
                azureBaseUrl = azureBaseUrl,
                apiVersion = apiVersion,
                modelName = modelName
            )
        }
    }

@CJCrafter
Copy link
Owner

That's the idea but instead of having a seperate AzureOpenAI, we just change the existing OpenAI class to allow custom endpoints. Then the builder class could have a method like:

fun azure(baseUrl: String, apiVersion: String, modelName: String) = apply { 
    this.endpoint = "$baseUrl/openai/deployments/$modelName/$endpoint?api-version=$apiVersion" 
}

So the build() function will always build an instance of OpenAI, even when azure is used.

@ugwun
Copy link
Contributor Author

ugwun commented May 15, 2023

I don't quite agree. We should separate concerns as stated in SOLID principles.

OpenAI API and Azure OpenAI API are two different concepts and may even diverge in the future. What we could is to create an interface with two different implementation for OpenAI API and Azure OpenAI API. This would allow the users to more easily create even their own implementations.

@CJCrafter
Copy link
Owner

Alright we'll leave it to the developers to handle swapping between OpenAI and AzureOpenAI, should they choose to.

@ugwun
Copy link
Contributor Author

ugwun commented May 16, 2023

Yes, it would be like having a Connection object for the database which could switch the backing databases on the fly. For example the developer could switch from PostreSQL to a MySQL. It would be prone to bugs.
Can we proceed with the merge than? :)

@CJCrafter
Copy link
Owner

Yessir, I'll merge now, then write documentation later.

@CJCrafter CJCrafter merged commit 68aeb73 into CJCrafter:master May 16, 2023
@CJCrafter
Copy link
Owner

1.3.1 should be live on maven central soon, I'll double check in the morning. Added a quick note in the wiki as well about Azure support. Thankyou for contributing!

@ugwun
Copy link
Contributor Author

ugwun commented May 20, 2023

That's great news, thanks! I will be probably submitting more Pull Requests, as I am currently working on a project using your OpenAI implementation... :)

@warmwind
Copy link

warmwind commented Jun 4, 2023

Thank you for supporting the Azure API. I'm having some trouble using it, as it seems that the final URL is incorrect. According to the Azure documentation, the endpoint doesn't have a 'v1' prefix, which is different from the OpenAI API.

Here are the correct endpoints for each API:

Azure API: https://{your-resource-name}.openai.azure.com/openai/deployments/{deployment-id}/chat/completions?api-version={api-version}
OpenAI API: https://api.openai.com/v1/chat/completions
In the current code, both OpenAI and AzureOpenAI use the same endpoint with the 'v1' prefix:

const val COMPLETIONS_ENDPOINT = "v1/completions"
const val CHAT_ENDPOINT = "v1/chat/completions"
const val IMAGE_CREATE_ENDPOINT = "v1/images/generations"
const val IMAGE_EDIT_ENDPOINT = "v1/images/edits"
const val IMAGE_VARIATION_ENDPOINT = "v1/images/variations"

When I ran it in my project, I got a 404 error. All of the other parameters passed to AzureOpenAI are correct, so the 'v1' prefix might be causing the issue. Could you please check this? Thank you.

@ugwun
Copy link
Contributor Author

ugwun commented Jun 4, 2023

@warmwind Hi, I will go through your remarks and let you know how we can approach this issue.

@ugwun
Copy link
Contributor Author

ugwun commented Jun 5, 2023

@warmwind You are right, I will fix it ASAP and create a Pull request

@warmwind
Copy link

warmwind commented Jun 6, 2023

@ugwun Thanks for your quick response. I am really looking forward to it. :D

@ugwun
Copy link
Contributor Author

ugwun commented Jun 7, 2023

@warmwind this is a quickfix pullrequest: #23

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.

3 participants