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

Azure OpenAI API #3

Closed
didalgolab opened this issue Sep 3, 2023 · 21 comments · Fixed by #42
Closed

Azure OpenAI API #3

didalgolab opened this issue Sep 3, 2023 · 21 comments · Fixed by #42
Labels
enhancement New feature or request

Comments

@didalgolab
Copy link
Contributor

Nice project! Do you plan to support the Azure OpenAI API? https://learn.microsoft.com/en-us/azure/ai-services/openai/reference

@sashirestela
Copy link
Owner

Thanks @didalgolab. It's not in my near future plans, maybe in the next major release, however I would like to know why you think I should support it? I mean, wouldn't it be kind of redundant to support Azure OpenAI if they are calling OpenAI services behind the scenes?

@didalgolab
Copy link
Contributor Author

One reason to use Azure OpenAI is that currently it's one of the few methods available for accessing gpt-4-32k model. Other considerations that might be important for some users or companies include: usage policies, GDPR compliance, co-location with other Azure services.

@the-gigi
Copy link
Contributor

@didalgolab did you try it with Azure OpenAI? It may just work. I tried it with Anyscale and it works like a charm including function calling.

@didalgolab
Copy link
Contributor Author

@the-gigi Yes, and unfortunately, it's not compatible. While the Anyscale API has been built with compatibility in mind, Azure OpenAI hasn't. It seems almost as if Microsoft did this on purpose:

  • On Azure, there is a mandatory query parameter "api-version" which must be added to each request. This can't be passed in the baseUrl to this library because it renders the URL invalid further down the road.
  • The "model" property is not allowed in the request.
  • Finally, there is a different header for authentication.

But anyway, it's a low priority for me now.

@the-gigi
Copy link
Contributor

the-gigi commented Feb 7, 2024

OK. I actually need to access Azure OpenAI now, so I'll look into making simple-openai work with the Azure incompatibilities. The api-key header instead of the standard uthorization: Bearer <token> is indeed an annoyance, but it's simple to add both. the model is probably easy to drop. The api-version query param seems the one that will require more work.

@the-gigi
Copy link
Contributor

the-gigi commented Feb 7, 2024

@sashirestela I looked into it and I think with some modifications simple-openai can handle Azure OpenAI too. I'm willing to work on a PR with your guidance.

Here are the sticking points:

  1. Endpoint URL.

Azure OpenAI has a dedicated URL per deployment. A deployment already has a specific model configured. The format of the URL is:

https://YOUR_RESOURCE_NAME.openai.azure.com/openai/deployments/YOUR_DEPLOYMENT_NAME/chat/completions?api-version=2023-05-15

So, from simple-openai point of view the base URL will be https://YOUR_RESOURCE_NAME.openai.azure.com/openai/deployments/YOUR_DEPLOYMENT_NAME (note, no /v1) and then it needs in the end to add the query params.

This can be accomplished by checking if the base url contains ? and then strip these and in the end before sending the actual HTTP request append the query params back.

simple-openai uses cleverclient, which provides an abstraction over URLs with annotations. so, I'm not sure how to accomplish it exactly. I see that for GET requests there is a @query annotation. maybe it work for POST requests too.

The other related issue is that simple-openai automatically adds the /v1 before each endpoint. This can be handled simply by removing it and having the endpoint be just /chat/completions (for chat, similar to other endpoints)

instead of :

@Resource("/v1/chat/completions")
    interface ChatCompletions {

just:

@Resource("/chat/completions")
    interface ChatCompletions {

Two additional minor issues as @didalgolab pointed out:

  • the authorization header is just api-key: <API Key>
  • model is not used (as mentioned the endpoint you're hitting is pre-configured for a specific model).

I believe supporting Azure OpenAI can be done without making simple-openai aware of the differences between providers. But, if going for multi-provider support it may be better to introduce the concept of an optional provider. The default will be OpenAI (which works out of the box for Anyscale too). For other providers like Azure OpenAI that currently are incompatible with some assumptions, simple-openai can internally adjust its behavior (e.g. drop the /v1 only for Azure OpenAI and add the api-key: <API Key> header only for Azure Open AI).

Let me know what you think.

@sashirestela
Copy link
Owner

sashirestela commented Feb 8, 2024

@the-gigi @didalgolab Do you know if the api-version url parameter can be different by endpoint or is it unique for all the API?
For example, in the case of OpenAI, the /v1 prefix is the version and it could vary by endpoint.

@the-gigi
Copy link
Contributor

the-gigi commented Feb 8, 2024

@sashirestela yes. they can even be different for the same endpoint. Each endpoint corresponds to a Deployment, which has its own model and has its own subdomain and supports different API versions with the query parameter.

See https://learn.microsoft.com/en-us/azure/ai-services/openai/reference#rest-api-versioning

@sashirestela
Copy link
Owner

sashirestela commented Feb 8, 2024

@the-gigi

In that case, one option could be adding a Query parameter to all the http methods and doing one of the following:

  1. Create default methods by each http method to set to null that api-version, so no impacting OpenAI users. The code is going to be pretty messy.
  2. Don't create default methods and the OpenAI users will need to add a null value for the api-version parameter. I don't think this is a good option.

Let me try to think of other options, but if you say that the parameter could change in every call, it is a very challenging requirement.

@the-gigi
Copy link
Contributor

the-gigi commented Feb 8, 2024

@sashir my thinking is that simple-openai should support the latest version by default for each API method. so, users normally don't have to pass it. But, if they want a different API version then there will be an optional apiVersion in their request and it will replace the default when specified. All this applies only if its targeting Azure OPenAI of course.

@sashirestela
Copy link
Owner

sashirestela commented Feb 8, 2024

@the-gigi

The scenario is the same, because Java doesn't support optional arguments for methods neither default values for arguments, so we will need to overload all the http methods with new default methods in order to support that requirement of variable api-version for Azure. The code of the OpenAI.java interface is going to grow a lot just for that new parameter.

I think that trying to use the same interface is very hard. Perhaps another option could be to create duplicated versions, I know that it is not ideal for your goal, but Microsoft is imposing a non standard way to handle this:

  • Standard: (Existing)
    • Interface: OpenAI.java
    • Class: SimpleOpenAI
  • Microsoft: (New)
    • Interface: AzureOpenAI.java
    • Class: SimpleAzureOpenAI.java

EDIT:
With this option of duplicated files we could solve the topics of:

  • api-version query param
  • /v1 only for Standard OpenAI

A remaining issue is how to handle the mandatory field model in the domain request objects. Maybe we will need to duplicate those Java classes also. What do you think?

@the-gigi
Copy link
Contributor

the-gigi commented Feb 8, 2024

My idea is not to pass the query parameters as part of the method, but as part of the request object. so, for example ChatRequest will have one new nullable field

public class ChatRequest {

    @NonNull private String model;
    @NonNull private List<ChatMsg> messages;
    private ChatRespFmt responseFormat;
   ...
   private String apiVersion; // new optional field for Azure OpenAPI version support.

If the field is not present then the default apiVersion for each Azure OpenAI API endpoint is appended to the URL.

So, the method signature remains (except for the /v1 that will be added to OpenAI only, or the relevant version if not /v1):
Instead of

@Resource("/v1/chat/completions")
    interface ChatCompletions {

We will have

@Resource("/chat/completions")
    interface ChatCompletions { 

It will require some dynamic setting of the base URL before calling celverclient.

The @resource annotation specifies just part of the path part of the URL. Different API methods and providers may have different prefixes (e.g. /v1 in case of OpenAI for /chat/completions) and different query params (api-version in case of Azure OpenAI)

Regarding the model. it shouldn't be required if we go for the multi-provider approach because not all providers require it. We can still implement per-provider logic and verify that for OpenAI it is provided.

@sashirestela
Copy link
Owner

My idea is not to pass the query parameters as part of the method, but as part of the request object. so, for example ChatRequest will have one new nullable field

If you pass it as part of the request object, it'll be on the payload body of the request, not in the url as a query parameter, so have you verified that Azure is going to recognize it in that way?

Instead of

@resource("/v1/chat/completions")
interface ChatCompletions {

We will have

@resource("/chat/completions")
interface ChatCompletions {

It will require some dynamic setting of the base URL before calling celverclient.

Remember that you need to do this by method (/v1 could be different by method). If you change baseUrl you will change it for all the methods and in a multithread scenario this could be an issue.

Regarding the model. it shouldn't be required if we go for the multi-provider approach because not all providers require it. We can still implement per-provider logic and verify that for OpenAI it is provided.

If we are going to support per-provider logic, please, take in account the Open-Close principle: open for extension and closed for modifications.

@the-gigi
Copy link
Contributor

the-gigi commented Feb 8, 2024

Yes. There is a lot of stuff that happens automatically with the annotations and serialization, so I need to verify my idea works. I'll follow up after I tried to implement my approach. It will require some provider-specific logic for sure. I hope it doesn't require duplicating a lot of classes and interfaces with different signatures per provider. Instead, I aim for a general-purpose implementation with some per-provider map of method -> prefix + query params. But, until I actually implement it, it is speculation only.

@didalgolab
Copy link
Contributor Author

In my opinion api-version should be provided just in baseUrl (and correctly handled by the cleverclient while making requests). I think there is little to no reason for a user to use different api-versions for different endpoints. More so, because single baseUrl in Azure represents just a one specific deployment that works with just a single api method (e.g. it works for chat completion and nothing else; or embeddings and nothing else - if that makes sense for you).

The user will have to create separate clients with different baseUrls for different endpoints anyway. I also agree with @sashirestela to add separate interface AzureOpenAI but preferably extending OpenAI so that both have some common part which could be used in the user's code exchangeably.

@the-gigi
Copy link
Contributor

the-gigi commented Feb 9, 2024

Technically the base URL is the part of the URL before the the path. This is how it is treated also by simple-openai/cleverclient. cleverclient constructs the full URL by appending the Path (e.g. /v1/chat/completions) to the base Url. It also checks if there are query params defined in the method signature and adds those. What we need is to provide the queryParams dynamically to clever client, so it can construct the URL as usual and then in the end add the provided query params (if any).

This is where it happens:
https://github.com/sashirestela/cleverclient/blob/main/src/main/java/io/github/sashirestela/cleverclient/http/URLBuilder.java#L29

@sashirestela the current design of cleverclient hard-codes the query params in the interface annotations as in:

@Resource("/posts")
public interface PostService {

  @GET
  List<Post> readPosts(@Query("_page") Integer page, @Query("_limit") Integer limit);

If you prefer to keep it strict (all query params must be specified at the interface level) then I think supporting Azure OpenAI will require a split.

But, if we add dynamic query params to cleverclient then simple-openai can add api-version query param based on the provider and we can remain with one version of the interfaces.

@the-gigi
Copy link
Contributor

the-gigi commented Feb 9, 2024

@sashirestela See sashirestela/cleverclient#39. This PR will allow simple-openai to inject the Azure OpenAI api-version query parameter into cleverclient without touching the interfaces.

@sashirestela
Copy link
Owner

@the-gigi Many things to comment:

  • Finally, you are discarding to set the api-version parameter per http method and you are going with the option to set a unique value for all the endpoints at the moment that you instance the SimpleOpenAI class. That's fine, we have an agreement here.

  • Cleverclient was developed to be an independent library to ease Java's HttpClient usage, it's not tied to simple-openai exclusively , so we need to think in general approaches when we want to introduce new functionalities.

  • I saw your PR to cleverclient. Please, don't misunderstand my next comment: it could resolve the specific requirement to adapt us to the Azure behavior regarding api-version, however we need a more general approach here, imho.

  • For some time now, before this Azure issue, I was thinking to update cleverclient to support request post processing to change the url, for example to add query params (eg: api-version) or to remove parts of the url (eg: /v1) or whatever change in the url that we could need to do. For this, I was planning to use the Interceptor concept to pass a function as a parameter, so the logic to change the url will be on the cleverclient's client side (simple-openai for our case). I was thinking of adding an Interceptor at cleverclient instantiation level and in the near future, to be more flexible, other Interceptors at endpoints creation level (eg: you could have different api-version per endpoint).

  • I think the time has come to develop that Interceptor functionality. So, I'll be working on that change. I'll keep you informed.

@sashirestela
Copy link
Owner

sashirestela commented Feb 9, 2024

@the-gigi

I've just developed the new feature UrlInterceptor. You can see an example of usage in the main branch (it is not deployed to Maven Central yet):

https://github.com/sashirestela/cleverclient/blob/main/src/test/java/io/github/sashirestela/cleverclient/CleverClientTest.java#L79-L105

In that example, I'm creating a functional Function with a logic to add a query param to url and remove '/vN' from url. Then, it is passed to the Cleverclient builder and it'll be executed automatically every time that we call a http method and it'll modify the url before hitting the server.

On the other hand, in previous comments you said:

Regarding the model. it shouldn't be required if we go for the multi-provider approach because not all providers require it. We can still implement per-provider logic and verify that for OpenAI it is provided.

So, where in the code do you think that we could introduce a per-provider logic to validate if the model in the request objects are required or not?

EDIT:
Have you verified if we can call Azure services by sending the model field with any value (eg: "any") and it does not fail?

@the-gigi
Copy link
Contributor

the-gigi commented Feb 10, 2024

@sashirestela that looks great. I didn't verify if it's OK to send a redundant model. I wonder if cleverclient should provide just a url interceptor or a complete request interceptor. This way consumers can intercept and modify any part of the request, including the body, which can be used to remove the model if necessary.

@sashirestela sashirestela linked a pull request Feb 13, 2024 that will close this issue
@sashirestela
Copy link
Owner

@sashirestela that looks great. I didn't verify if it's OK to send a redundant model. I wonder if cleverclient should provide just a url interceptor or a complete request interceptor. This way consumers can intercept and modify any part of the request, including the body, which can be used to remove the model if necessary.

Guys, the cleverclient library was extended to handle the full request (url, body, headers). So, simple-openai can now support to use Azure OpenAI, and you just need to provide a requestInterceptor function where you can make all the changes to adapt the standard request to the particularities of the Microsoft library.

See an example of a requestInterceptor here: https://github.com/sashirestela/cleverclient/blob/main/src/test/java/io/github/sashirestela/cleverclient/CleverClientTest.java#L71-L139

@sashirestela sashirestela added the enhancement New feature or request label Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants