Refactor to add a base class and dedicated classes for Azure and Anyscale #47

the-gigi · 2024-02-14T03:34:31Z

See discussion here:
#45

Also added demos for each of the new classes

tested on all providers OpenAI, Azure OpenAI and Anyscale.

Azure OpenAI supports blocking chat completion + functions (fails on streaming)
Anyscale supports streaming, blocking + functions

…c logic

codecov-commenter · 2024-02-14T03:35:28Z

Codecov Report

Attention: 29 lines in your changes are missing coverage. Please review.

Comparison is base (4c78f0d) 87.94% compared to head (bac3158) 85.91%.

Files	Patch %	Lines
...o/github/sashirestela/openai/BaseSimpleOpenAI.java	56.00%	10 Missing and 1 partial ⚠️
.../github/sashirestela/openai/SimpleOpenAIAzure.java	63.33%	7 Missing and 4 partials ⚠️
...thub/sashirestela/openai/SimpleOpenAIAnyscale.java	66.66%	4 Missing ⚠️
...thub/sashirestela/openai/BaseSimpleOpenAIArgs.java	80.00%	0 Missing and 1 partial ⚠️
...ain/java/io/github/sashirestela/openai/OpenAI.java	90.90%	0 Missing and 1 partial ⚠️
...va/io/github/sashirestela/openai/SimpleOpenAI.java	90.90%	0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff              @@
##               main      #47      +/-   ##
============================================
- Coverage     87.94%   85.91%   -2.03%     
- Complexity      602      609       +7     
============================================
  Files           105      109       +4     
  Lines           937     1001      +64     
  Branches         27       33       +6     
============================================
+ Hits            824      860      +36     
- Misses           68       89      +21     
- Partials         45       52       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sashirestela · 2024-02-14T14:58:27Z

@the-gigi You have changed 95 files for this PR! Please, discard unnecessary changes and try to focus on the main goal of the Issue.

the-gigi · 2024-02-14T15:51:41Z

I did optimize imports in the IDE to find all unused imports. Apparently it also sorted imports in other files. Are you opposed to sorted imports? This will be a one time thing.

sashirestela · 2024-02-14T16:31:28Z

The order is following a lint rule. I need to check how to share it, to be on the same page.
For now, please, remove unused imports without reordering them.

sashirestela · 2024-02-14T17:40:47Z

See discussion here: #45

Also added demos for each of the new classes

tested on all providers OpenAI, Azure OpenAI and Anyscale.

Azure OpenAI supports blocking chat completion + functions (fails on streaming) Anyscale supports streaming, blocking + functions

@the-gigi Have you verified if those providers support the vision feature? As you should know, the Chat Completion service also provides the capacity to recognize images and you can ask the model about image details. If you haven't, please verify and comment the results.

Check details here: https://platform.openai.com/docs/guides/vision

the-gigi · 2024-02-15T01:50:54Z

@sashirestela here is the next iteration of the PR for your review.

With the BaseSimpleOpenAI being just an interface as you requested there is a lot of duplication in all 3 implementations (lots of fields and similar logic in constructors). All this duplication can be pushed down to the base class as I did in the previous iteration. This is cleaner IMO, but in the end it's your decision.

I fixed an issue that surfaces only with Azure OpenAI in the ChatRequest class - If tools are provided, but tools choice is null it fails on Azure OpenAI. I changed it so, if tools are provided, but no tools choice it set tools choice to AUTO.

If you have special formatting or linting rules you may consider sharing them as a pre-commit git hook. Or even better have github actions that to perform them automatically on PRs. This can save a lot of unnecessary work.

The chat completion with streaming doesn't work in AzureOpenAI (Not calling it in the AzureChatServiceDemo).
It just returns empty response. No error. blocking + function calling works fine.

I didn't test with images. I'll test and let you know the status.

sashirestela · 2024-02-15T23:23:47Z

@the-gigi

First, thanks for reducing the number of modified files on your PR. I'll take the task to improve the linting rules.

Regarding the main change, I understand your concerns and we could do a slight variation in order to reduce the repeated code:

Let's turn BaseSimpleOpenAI in an abstract class, but with some variations:

abstract class BaseSimpleOpenAI {
    protected static final END_OF_STREAM = "[DONE]";
    
    @Getter
    protected String apiKey;
    @Getter
    protected String baseUrl;
    @Getter
    protected HttpClient httpClient;
    @Getter
    @Setter
    protected CleverClient cleverClient;
    
    protected Map<String, String> headers;
    protected UnaryOperator<HttpRequestData> requestInterceptor;
    
    protected OpenAI.Audios audioService;
    protected OpenAI.ChatCompletions chatCompletionService;
    protected OpenAI.Completions completionService;
   // etc.
    
    @SuperBuilder
    public BaseSimpleOpenaAI(String apiKey, String baseUrl, HttpClient httpClient) {
        this.apiKey = apiKey;
        this.baseUrl = baseUrl;
        this.httpClient = Optional.ofNullable(httpClient).orElse(HttpClient.newHttpClient());
        this.headers = new HashMap<String, String>();
        this.requestInterceptor = null;
    }
    
    protected void postContructor() {
        customSetup();
        this.cleverClient = CleverClient.builder()
                .httpClient(this.httpClient)
                .baseUrl(this.baseUrl)
                .headers(headers)
                .endOfStream(END_OF_STREAM)
                .requestInterceptor(requestInterceptor)
                .build();
    }
    
    abstract protected void customSetup();
    
    // Common service calls the cleverClient.create() method. Don't include this comment.
    public OpenAI.ChatCompletions chatCompletions() {
        if (chatCompletionService == null) {
            chatCompletionService = cleverClient.create(OpenAI.ChatCompletions.class);
        }
        return chatCompletionService;
    }

    // No common service throws an exception. Don't include this comment.
    public OpenAI.Audios audios() {
        throws new UnimplementedException();
    }
    
    // No common service throws an exception. Don't include this comment.
    public OpenAI.Completions completions() {
        throws new UnimplementedException();
    }
    
    // etc.
}

Now, SimpleOpenAI can add the specific parameters and overwrite the No-common services:

public class SimpleOpenAI extends BaseSimpleOpenaAI {

    @Getter
    private String organizationId;

    @SuperBuilder
    public SimpleOpenaAI(@NonNull String apiKey, String baseUrl, HttpClient httpClient, String organizatonId) {
        super(apiKey, baseUrl, httpClient);
        this.organizationId = organizationId;
        postConstructor();
    }
    
    protected void customSetup() {
        baseUrl = Optional.ofNullable(baseUrl).orElse(OPENAI_BASE_URL);
        headers.put(AUTHORIZATION_HEADER, BEARER_AUTHORIZATION + apiKey);
        if (organizationId != null) {
            headers.put(ORGANIZATION_HEADER, organizationId);
        }
    }
    
    // Don't need to implement common services (ie: chatCompletions), but no-common, do.

    public OpenAI.Audios audios() {
        if (audioService == null) {
            audioService = cleverClient.create(OpenAI.Audios.class);
        }
        return audioService;
    }

    public OpenAI.Completions completions() {
        if (completionService == null) {
            completionService = cleverClient.create(OpenAI.Completions.class);
        }
        return completionService;
    }
    
    // etc.
    
}

Now, SimpleOpenAIAzure can add the specific parameters and doesn't need overwrite any services:

public class SimpleOpenAIAzure extends BaseSimpleOpenaAI {

    @Getter
    private String apiVersion;

    @SuperBuilder
    public SimpleOpenaAIAzure(@NonNull String apiKey, @NonNull String baseUrl, HttpClient httpClient, @NonNull String apiVersion) {
        super(apiKey, baseUrl, httpClient);
        this.apiVersion = apiVersion;
        postConstructor();
    }
    
    protected void customSetup() {
        headers.put(APIKEY_HEADER, apiKey);
        requestInterceptor = request -> {
            // modify the url to add the property 'this.apiVersion' as query param
            // modify the url to remove /v1
            // modify the body to remove the model property
            return request;
        }
    }
    
    // Don't need to implement any service.
}

sashirestela · 2024-02-15T23:50:24Z

@the-gigi

Regarding your change in the ChatRequest class, I'm afraid that is not the right place to do it. We are using constructors as an exceptional place to do special validation logic, in this case, for properties that could be one of several types. I'm thinking of replacing that special validation with annotations, so we won't need to declare explicit constructors.

The right place for your change is on the OpenAI interface on the default methods. Specifically, you should do your change here for blocking chats and here for streaming chats.

Take in account that we are working with immutable objects, so, you need to use the Lombok's @With annotation for the field that you are going to change, in this case toolChoice and you should chain it with the existing one for the field stream.

sashirestela · 2024-02-16T00:31:14Z

@the-gigi

I see a small improvement in the url for SimpleOpenAIAzure:

public class SimpleOpenAIAzure extends BaseSimpleOpenaAI {

    @Getter
    private String apiVersion;

    @SuperBuilder
    public SimpleOpenaAIAzure(@NonNull String apiKey, String baseUrl, HttpClient httpClient, @NonNull String resourceName, @NonNull String deploymentId, @NonNull String apiVersion) {
        super(apiKey, "https://"+resourceName+Optional.ofNullable(baseUrl).orElse(AZURE_BASE_URL)+deploymentId, httpClient);
        this.apiVersion = apiVersion;
        postConstructor();
    }
    
    ...
}

the-gigi · 2024-02-16T10:32:10Z

@sashirestela this all sounds good.

I'm not clear about the value that postConstructor() and customSetup() methods provide. in your sample code BaseSimpleOpenAI creates the cleverclient in the postConstructor, but it doesn't call postConstructor() at the end of the BaseSimpleOpenaAI constructor. This means that sub-classes are now responsible for calling postConstructor() in order for the base class to have a cleverclient.

Also, postConstructor is protected, which means sub-classes can override it and do anything they want instead of creating a cleverclient.

@SuperBuilder
    public BaseSimpleOpenaAI(String apiKey, String baseUrl, HttpClient httpClient) {
        this.apiKey = apiKey;
        this.baseUrl = baseUrl;
        this.httpClient = Optional.ofNullable(httpClient).orElse(HttpClient.newHttpClient());
        this.headers = new HashMap<String, String>();
        this.requestInterceptor = null;
    }

  protected void postContructor() {
        customSetup();
        this.cleverClient = CleverClient.builder()
                .httpClient(this.httpClient)
                .baseUrl(this.baseUrl)
                .headers(headers)
                .endOfStream(END_OF_STREAM)
                .requestInterceptor(requestInterceptor)
                .build();
    }

is it possible you meant to call postConstructor() here and make it private while the protected customSetup()is where sub-classes do their customization? something like:

@SuperBuilder
    public BaseSimpleOpenaAI(String apiKey, String baseUrl, HttpClient httpClient) {
        this.apiKey = apiKey;
        this.baseUrl = baseUrl;
        this.httpClient = Optional.ofNullable(httpClient).orElse(HttpClient.newHttpClient());
        this.headers = new HashMap<String, String>();
        this.requestInterceptor = null;
        this.postConstructor();
    }

  private void postContructor() {
        customSetup();
        this.cleverClient = CleverClient.builder()
                .httpClient(this.httpClient)
                .baseUrl(this.baseUrl)
                .headers(this.headers)
                .endOfStream(END_OF_STREAM)
                .requestInterceptor(requestInterceptor)
                .build();
    }

However in this case, the postConstructor logic can just be collapsed into the constructor as in:

@SuperBuilder
    public BaseSimpleOpenaAI(String apiKey, String baseUrl, HttpClient httpClient) {
        this.apiKey = apiKey;
        this.baseUrl = baseUrl;
        this.httpClient = Optional.ofNullable(httpClient).orElse(HttpClient.newHttpClient());
        this.headers = new HashMap<String, String>();
        this.requestInterceptor = null;
        // call protected method potentially overridden by sub-class
        customSetup();
        this.cleverClient = CleverClient.builder()
                .httpClient(this.httpClient)
                .baseUrl(this.baseUrl)
                .headers(headers)
                .endOfStream(END_OF_STREAM)
                .requestInterceptor(requestInterceptor)
                .build();
    }

This workflow allows the sub-class to modify the fields of the base class used to build the clever client. However, this has some problems.

First, when looking at the constructor it's not clear what parameters are needed. it divides the parameters into 3 groups: mandatory constructor arguments.(apiKey and baseUrl), optional constructor arguments (httpClient) and "hidden" optional arguments that may be set in customSetup() like the headers and the request interceptor. Another problem is that different parameters can be set in 3 different places now.

For example, the httpClient can be passed as constructor argument or as null and then a default HttpClient is created. But, it can also be overridden in customSetup() later. Similarly, baseUrl can be passed as constructor argument, be set later in customSetup() and also be modified later in requestInterceptor. This can be very confusing when trying to troubleshoot some misbehaving sub-class.

What do you think about this alternative?

BaseSimpleOpenAi has a constructor that takes a single argument BaseSimpleOpenAiArgs.
It is now responsible for two things only:

creating the default httpClient if it wasn't provided
create the cleverclient

It is very clean now nad has a single filed for cleverClient (with a setter because of the tests)
and another one for the chat completion service.

public class BaseSimpleOpenAI {

    private static final String END_OF_STREAM = "[DONE]";

    @Setter
    protected CleverClient cleverClient;

    protected OpenAI.ChatCompletions chatCompletionService;

    BaseSimpleOpenAI(@NonNull BaseSimpleOpenAiArgs args) {
        var httpClient =
            Optional.ofNullable(args.getHttpClient()).orElse(HttpClient.newHttpClient());
        this.cleverClient = CleverClient.builder()
            .httpClient(httpClient)
            .baseUrl(args.getBaseUrl())
            .headers(args.getHeaders())
            .endOfStream(END_OF_STREAM)
            .requestInterceptor(args.getRequestInterceptor())
            .build();
    }

It also has per your suggestion the implementation of chat services and throw the
not implemented exception for all the other endpoints:

    /**
     * Throw not implemented
     */
    public OpenAI.Audios audios() {
        throw new SimpleUncheckedException("Not implemented");
    }

    /**
     * Generates an implementation of the ChatCompletions interface to handle
     * requests.
     *
     * @return An instance of the interface. It is created only once.
     */
    public OpenAI.ChatCompletions chatCompletions() {
        if (this.chatCompletionService == null) {
            this.chatCompletionService = this.cleverClient.create(OpenAI.ChatCompletions.class);
        }
        return this.chatCompletionService;

    }

// the rest of endpoint also throw "Not implemented"

BaseSimpleOpenAiArgs has all the arguments needed to build a clever client and a builder

@Getter
@Builder
public class BaseSimpleOpenAiArgs {
    @NonNull
    private final String baseUrl;
    private final Map<String, String> headers;
    private final HttpClient httpClient;
    private final UnaryOperator<HttpRequestData> requestInterceptor;
}

Note that it doesn't have an api key because the different providers will already
embed the API key in one of the headers.

Finally, the different providers are very simple too. Their constructor is just one line
calling the super() with BaseSimpleOpenAiArgs that they create in a static method called
prepareBaseSimpleOpenAiArgs().

Here SimpleOpenAI

    private static BaseSimpleOpenAiArgs prepareBaseSimpleOpenAiArgs(
        String apiKey, String organizationId, String baseUrl, HttpClient httpClient) {

        var headers = new HashMap<String, String>();
        headers.put(AUTHORIZATION_HEADER, BEARER_AUTHORIZATION + apiKey);
        if (organizationId != null) {
            headers.put(ORGANIZATION_HEADER, organizationId);
        }

        return BaseSimpleOpenAiArgs.builder()
            .baseUrl(Optional.ofNullable(baseUrl).orElse(OPENAI_BASE_URL))
            .headers(headers)
            .httpClient(httpClient)
            .build();
    }

    @Builder
    public SimpleOpenAI(@NonNull String apiKey, String organizationId, String baseUrl, HttpClient httpClient) {
        super(prepareBaseSimpleOpenAiArgs(apiKey, organizationId, baseUrl, httpClient));
    }

There is no need even for @SuperBuilder because the base class doesn't have a builder.
It just accepts in its constructor the BaseSimpleOpenAIArgs, which has a @builder.

SimpleOpenAI also implements all the endpoints except chatServices that the base already implements:

    /**
     * Generates an implementation of the Audios interface to handle requests.
     *
     * @return An instance of the interface. It is created only once.
     */
    public OpenAI.Audios audios() {
        if (audioService == null) {
            audioService = cleverClient.create(OpenAI.Audios.class);
        }
        return audioService;
    }
    
    // and the rest of the endpoints

With this design we can also get rid in OpenSimpleAI from all the @Getter(AccessLevel.NONE)
annotation on all the fields as well as the overall @Getter on the class itself. There is no
need to get any field ever because it doesn't keep any fields everything goes to the base class.

SimpleOpenAIAnyscale is the simplest. Here is literally the entire class:

public class SimpleOpenAIAnyscale extends BaseSimpleOpenAI {

    private static final String AUTHORIZATION_HEADER = "Authorization";
    private static final String BEARER_AUTHORIZATION = "Bearer ";
    
    /**
     * Constructor used to generate a builder.
     *
     * @param apiKey         Identifier to be used for authentication. Mandatory.
     * @param baseUrl        Host's url
     * @param httpClient     A {@link java.net.http.HttpClient HttpClient} object.
     *                       One is created by default if not provided. Optional.
     */
    private static BaseSimpleOpenAiArgs prepareBaseSimpleOpenAiArgs(
        String apiKey, String baseUrl, HttpClient httpClient) {

        var headers = new HashMap<String, String>();
        headers.put(AUTHORIZATION_HEADER, BEARER_AUTHORIZATION + apiKey);

        return BaseSimpleOpenAiArgs.builder()
            .baseUrl(baseUrl)
            .headers(headers)
            .httpClient(httpClient)
            .build();
    }

    @Builder
    public SimpleOpenAIAnyscale(
        @NonNull String apiKey,
        @NonNull String baseUrl,
        HttpClient httpClient) {
        super(prepareBaseSimpleOpenAiArgs(apiKey, baseUrl, httpClient));
    }
}

SimpleOpenAiAzure is similar except it sets a request interceptor too in its prepare method:

public class SimpleOpenAIAzure extends BaseSimpleOpenAI {

    private static BaseSimpleOpenAiArgs prepareBaseSimpleOpenAiArgs(
        String apiKey, String baseUrl, String apiVersion, HttpClient httpClient) {

        var headers = Map.of("api-Key", apiKey);

        // Inline the UnaryOperator<HttpRequestData> as a lambda directly.
        var requestInterceptor = (UnaryOperator<HttpRequestData>) request -> {
            var url = request.getUrl();
            var contentType = request.getContentType();
            var body = request.getBody();

            // add a query parameter to url
            url += (url.contains("?") ? "&" : "?") + "api-version=" + apiVersion;
            // remove '/vN' or '/vN.M' from url
            url = url.replaceFirst("(\\/v\\d+\\.*\\d*)", "");
            request.setUrl(url);

            if (contentType != null) {
                if (contentType.equals(ContentType.APPLICATION_JSON)) {
                    var bodyJson = (String) request.getBody();
                    // remove a field from body (as Json)
                    bodyJson = bodyJson.replaceFirst(",?\"model\":\"[^\"]*\",?", "");
                    bodyJson = bodyJson.replaceFirst("\"\"", "\",\"");
                    body = bodyJson;
                }
                if (contentType.equals(ContentType.MULTIPART_FORMDATA)) {
                    var bodyMap = (Map<String, Object>) request.getBody();
                    // remove a field from body (as Map)
                    bodyMap.remove("model");
                    body = bodyMap;
                }
                request.setBody(body);
            }

            return request;
        };

        return BaseSimpleOpenAiArgs.builder()
            .baseUrl(baseUrl)
            .headers(headers)
            .httpClient(httpClient)
            .requestInterceptor(requestInterceptor)
            .build();
    }

    /**
     * Constructor used to generate a builder.
     *
     * @param apiKey         Identifier to be used for authentication. Mandatory.
     * @param baseUrl        The URL of the Azure OpenAI deployment.   Mandatory.
     * @param apiVersion     Azure OpeAI API version. See:
     *                       <a href="https://learn.microsoft.com/en-us/azure/ai-services/openai/reference#rest-api-versioning">Azure OpenAI API versioning</a>
     * @param httpClient     A {@link HttpClient HttpClient} object.
     *                       One is created by default if not provided. Optional.
     */
    @Builder
    public SimpleOpenAIAzure(
        @NonNull String apiKey,
        @NonNull String baseUrl,
        @NonNull String apiVersion,
        HttpClient httpClient) {
        super(prepareBaseSimpleOpenAiArgs(apiKey, baseUrl, apiVersion, httpClient));
    }
}

the-gigi · 2024-02-16T10:38:35Z

src/test/java/io/github/sashirestela/openai/SimpleOpenAITest.java

-                .build();
-        assertTrue(openAI.getCleverClient().getHeaders().containsValue(openAI.getOrganizationId()));
-    }
+//    @Test


these tests check for set properties. they use getters for fields that don't exist anymore. it also seems a little awkward to have a getter only for tests. I think these tests don't provide much value. but, if you think it is useful we can restore them and add the getters again.

Take in account that we point to have a high test coverage of the code. You could remove it, but you should provide alternative unit tests.

On the other hand, please, provide unit tests for the changes and new classes introduced in this PR.

All the existing tests pass except for these tests that check for fields that don't exist anymore. the organizationId is embedded into a header in the new design.

with your permission I'll delete the commented out tests.

I'll for the new classes I'll add tests for the prepare functions.

I think what can really increase the testability and the confidence that simple-openai works as expected and changes are safe is to add end-to-end tests. pretty much what the demos are doing, but against a dedicated HTTP test server, not the real providers of course. Each provider will have its own mock service that conforms to its convention and can check the headers and base url passed in and the request schema. Obviously , this is a bug change so not for today. WDYT?

sashirestela · 2024-02-16T15:38:53Z

@the-gigi

Your proposal is not bad. Let's move in that direction. I'm going to review the code details.

sashirestela

@the-gigi Please, provide changes or answers.

sashirestela · 2024-02-16T15:50:42Z

src/main/java/io/github/sashirestela/openai/BaseSimpleOpenAI.java

+ */
+
+
+public class BaseSimpleOpenAI {


Shouldn't this be an abstract class?

I don't think so. It can be instantiated on its own. For example, if someone wants to access another OpenAI provider that is not one of the officially supported: OpenAI, Azure OpenAI or Anyscale. They can directly proper create BaseSimpleOpenAIArgs and instantiate a BaseSimpleOpenAI object.

Please, turn into an abstract class. Any other provider must extend this abstract class

sashirestela · 2024-02-16T15:52:17Z

src/main/java/io/github/sashirestela/openai/BaseSimpleOpenAiArgs.java

+
+@Getter
+@Builder
+public class BaseSimpleOpenAiArgs {


To be consistent, change Ai to AI in the class name

sashirestela · 2024-02-16T16:07:30Z

src/main/java/io/github/sashirestela/openai/SimpleOpenAI.java

    private OpenAI.Moderations moderationService;

-    @Getter(AccessLevel.NONE)
    private OpenAI.Assistants assistantService;

    @Getter(AccessLevel.NONE)


Do you keep this Getter for any special reason?

no. I just missed it somehow.

sashirestela · 2024-02-16T16:10:44Z

src/main/java/io/github/sashirestela/openai/SimpleOpenAIAnyscale.java

+    }
+
+    @Builder
+    public SimpleOpenAIAnyscale(


I think the parameters could be in the same line of the constructor name.

I don't see the change here

sashirestela · 2024-02-16T16:13:48Z

src/main/java/io/github/sashirestela/openai/SimpleOpenAIAnyscale.java

+    @Builder
+    public SimpleOpenAIAnyscale(
+        @NonNull String apiKey,
+        @NonNull String baseUrl,


Does Anyscale need a different url every time? Isn't that the same as OpenAI that has a standard base url? If that is the case, you could create a constant ANYSCALE_BASE_URL.

They plan to introduce a private endpoints service (more enterprise ready). I'm not sure what the base URL situation is going to be. but, I can make it optional and use the current base URL by default if not provided.

sashirestela · 2024-02-16T17:13:52Z

src/demo/java/io/github/sashirestela/openai/demo/AzureChatServiceDemo.java

+import java.util.Base64;
+import java.util.List;
+
+public class AzureChatServiceDemo extends AbstractDemo {


To be consistent with the other demos, rename it to ChatAzureServiceDemo

I don't see the renaming.

sashirestela · 2024-02-16T17:15:23Z

src/demo/java/io/github/sashirestela/openai/demo/AnyscaleChatServiceDemo.java

+    private ChatRequest chatRequest;
+
+
+    @SuppressWarnings("unchecked")


This is not necessary anymore here.

sashirestela · 2024-02-16T17:16:12Z

src/demo/java/io/github/sashirestela/openai/demo/AzureChatServiceDemo.java

+public class AzureChatServiceDemo extends AbstractDemo {
+    private ChatRequest chatRequest;
+
+    @SuppressWarnings("unchecked")


This is not necessary anymore here.

sashirestela · 2024-02-16T17:17:32Z

src/main/java/io/github/sashirestela/openai/OpenAI.java

 import java.io.InputStream;
 import java.util.EnumSet;
 import java.util.List;
+import java.util.Optional;


Remove this unused import

src/main/java/io/github/sashirestela/openai/SimpleOpenAIAzure.java

…quest interception

sashirestela

I think you have bypassed many observations on my first revision. Please, review carefully

sashirestela · 2024-02-16T22:30:00Z

src/main/java/io/github/sashirestela/openai/BaseSimpleOpenAI.java

+ */
+
+
+public class BaseSimpleOpenAI {


Please, turn into an abstract class. Any other provider must extend this abstract class

sashirestela · 2024-02-16T22:31:43Z

src/demo/java/io/github/sashirestela/openai/demo/AnyscaleChatServiceDemo.java

+import io.github.sashirestela.openai.function.FunctionExecutor;
+import java.util.ArrayList;
+
+public class AnyscaleChatServiceDemo extends AbstractDemo {


I don't see the renaming

sashirestela · 2024-02-16T22:32:21Z

src/demo/java/io/github/sashirestela/openai/demo/AzureChatServiceDemo.java

+import java.util.Base64;
+import java.util.List;
+
+public class AzureChatServiceDemo extends AbstractDemo {


I don't see the renaming.

sashirestela · 2024-02-16T22:34:15Z

src/main/java/io/github/sashirestela/openai/SimpleOpenAI.java

 import lombok.AccessLevel;
 import lombok.Builder;
 import lombok.Getter;
 import lombok.NonNull;
-import lombok.Setter;

 /**
 * The factory that generates implementations of the {@link OpenAI OpenAI}


Please update the comment according to my previous observation.

sashirestela · 2024-02-16T22:34:42Z

src/main/java/io/github/sashirestela/openai/SimpleOpenAIAnyscale.java

+import lombok.NonNull;
+
+/**
+ * The factory that generates implementations of the {@link OpenAI OpenAI}


Please update the comment according to my previous observation.

sashirestela · 2024-02-16T22:38:58Z

src/main/java/io/github/sashirestela/openai/domain/chat/ChatRequest.java

@@ -13,6 +15,7 @@
 import io.github.sashirestela.openai.domain.chat.tool.ChatTool;
 import io.github.sashirestela.openai.domain.chat.tool.ChatToolChoice;
 import io.github.sashirestela.openai.domain.chat.tool.ChatToolChoiceType;
+import java.util.Optional;


I don't see the change here.

sashirestela · 2024-02-16T22:39:14Z

src/main/java/io/github/sashirestela/openai/domain/chat/ChatRequest.java

@@ -29,7 +32,7 @@ public class ChatRequest {
    private ChatRespFmt responseFormat;
    private Integer seed;
    private List<ChatTool> tools;
-    private Object toolChoice;
+    @With private Object toolChoice;


Any unit test for this?

sashirestela · 2024-02-16T22:40:39Z

src/main/java/io/github/sashirestela/openai/SimpleOpenAIAnyscale.java

+                                                                   HttpClient httpClient) {
+
+        if (isNullOrEmpty(baseUrl)) {
+            baseUrl = DEFAULT_BASE_URL;


Change this for Optional

sashirestela · 2024-02-16T22:41:59Z

src/test/java/io/github/sashirestela/openai/SimpleOpenAIAnyscaleTest.java

+
+class SimpleOpenAIAnyscaleTest {
+    @Test
+    void shouldPrepareBaseOpenSimpleAIArgsCorrectly() {


You could check that the default url is taken when you no pass any one and the same for the httpClient

sashirestela · 2024-02-16T22:45:11Z

src/test/java/io/github/sashirestela/openai/SimpleOpenAITest.java

-                .organizationId("orgId")
-                .build();
-        assertTrue(openAI.getCleverClient().getHeaders().containsValue(openAI.getOrganizationId()));
+    void shouldPrepareBaseOpenSimpleAIArgsCorrectly() {


Check when baseUrl ans httpClient are not passed

sashirestela

I am approving this, but there are some observations that I need to address on a different PR

sashirestela · 2024-02-17T18:50:52Z

src/main/java/io/github/sashirestela/openai/domain/chat/ChatRequest.java

@@ -48,12 +46,6 @@ public ChatRequest(@NonNull String model, @NonNull @Singular List<ChatMsg> messa
            Integer seed, @Singular List<ChatTool> tools, Object toolChoice, Double temperature, Double topP, Integer n,
            Boolean stream, Object stop, Integer maxTokens, Double presencePenalty, Double frequencyPenalty,
            Map<String, Integer> logitBias, String user, Boolean logprobs, Integer topLogprobs) {
-        if (toolChoice != null &&


@the-gigi Why have you removed this type validation from here?

sashirestela · 2024-02-17T18:52:50Z

src/main/java/io/github/sashirestela/openai/OpenAI.java

+        if (!isNullOrEmpty(chatRequest.getTools())) {
+            if (toolChoice == null) {
+                toolChoice = ChatToolChoiceType.AUTO;
+            } else if (!(toolChoice instanceof ChatToolChoice) &&


@the-gigi Why have you brought this type validation code? This should keep on the ChatRequest class

the-gigi added 4 commits February 13, 2024 14:57

refactor SimpleOpenAI into a base class + a class with OpenAI specifi…

e1ca88f

…c logic

added dedicated classes for Anyscale and Azure OpenAI + demos

8a7e9a6

remove unused constructor

b731d6e

optimize imports

4df6fd4

the-gigi added 3 commits February 14, 2024 15:45

revert optimize imports

1d215f8

reverted the refactoring of core functionality into a base class

c524404

throw 'not implemented' instead of returning null

8359999

the-gigi added 6 commits February 16, 2024 01:16

refactor to base class instead of interface

d897f0a

parameterize apiVersion + chat completion with image works

ee44c38

make base url NotNull

848d1b5

get rid of redundant Getter annotationations

ae1afa9

fix constructor docmentation

5bc64a6

moved tool choice fix to OpenAI

f4c2e8b

the-gigi commented Feb 16, 2024

View reviewed changes

sashirestela requested changes Feb 16, 2024

View reviewed changes

the-gigi added 2 commits February 16, 2024 13:18

addressed review comments

497d89b

added tests for new classes, BaseSimpleOPenAIArgs construction and re…

2ce5a9b

…quest interception

sashirestela requested changes Feb 16, 2024

View reviewed changes

address more review comments

0d91616

the-gigi added 5 commits February 16, 2024 17:14

refactored updateRequest() function

39203bf

updated tool choice verification

9ecae33

fix constructor parameters

fb1a45c

fixed class comment for SimpleOpenAI

b553ab5

fixed more constructor arguments

bac3158

sashirestela linked an issue Feb 17, 2024 that may be closed by this pull request

Support multiple OpenAI providers directly #45

Closed

sashirestela changed the title ~~Refactor core functionality of SimpleOpenAI to a base class and add dedicated classes for Azure and Anyscale~~ Refactor to add a base class and dedicated classes for Azure and Anyscale Feb 17, 2024

sashirestela approved these changes Feb 17, 2024

View reviewed changes

sashirestela merged commit 1daf208 into sashirestela:main Feb 17, 2024
1 check passed

		private ChatRequest chatRequest;


		@SuppressWarnings("unchecked")

Refactor to add a base class and dedicated classes for Azure and Anyscale #47

Refactor to add a base class and dedicated classes for Azure and Anyscale #47

Conversation

the-gigi commented Feb 14, 2024 • edited Loading

codecov-commenter commented Feb 14, 2024 • edited Loading

Codecov Report

sashirestela commented Feb 14, 2024

the-gigi commented Feb 14, 2024

sashirestela commented Feb 14, 2024

sashirestela commented Feb 14, 2024 • edited Loading

the-gigi commented Feb 15, 2024 • edited Loading

sashirestela commented Feb 15, 2024 • edited Loading

sashirestela commented Feb 15, 2024

sashirestela commented Feb 16, 2024 • edited Loading

the-gigi commented Feb 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sashirestela commented Feb 16, 2024

sashirestela left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sashirestela left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sashirestela left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

the-gigi commented Feb 14, 2024 •

edited

Loading

codecov-commenter commented Feb 14, 2024 •

edited

Loading

sashirestela commented Feb 14, 2024 •

edited

Loading

the-gigi commented Feb 15, 2024 •

edited

Loading

sashirestela commented Feb 15, 2024 •

edited

Loading

sashirestela commented Feb 16, 2024 •

edited

Loading