This change aims to conceal the implementation and facilitate handling option objects through the interface.

And enforcing immutability to ensure greater safety in multi-threaded environments.

  • Add Builder and Nested Private Implemented Class.

  • Add builder() to Create New Object from existing.

  • Add PortableFunctionCallingOptions to extract ChatOptions, FunctionsOptions

Open AI

  • To separate the responsibilities of objects, implement a dedicated object for Properties binding.

  • Hide the implementation and only allow access through the interface to increase extensibility.

Thanks

  • 408

Comment From: markpollack

Thank you, this is an area of the api I would like to review.

Comment From: markpollack

There are some good things—no one is arguing against immutability—but the PR does too much, and there are some choices that I don't understand or disagree with. Let's see how we can split this up into more fine-grained issues.

I'll take your bullet points in the PR comment one by one.

  1. Add Builder and Nested Private Implemented Class

The convention that I've found in Spring is not to have a top-level class with the suffix Builder that is accessed directly. Instead, there is a private builder class inside the class being built.

As an example, this PR has a top-level:

public class OpenAiAudioTranscriptionOptionsBuilder {

But the current code has:

public static Builder builder() {
   return new Builder();
}

public static class Builder {
   protected OpenAiAudioTranscriptionOptions options;
}

I do not want to break this stylistic convention, as we should adhere to the Spring ecosystem conventions.

I do see that sometimes Spring projects create a builder interface and then have a DefaultBuilder implementation. For example, UriHandlerFilter in Spring Framework does this. I think that's a good idea,

Also, we haven’t been consistent: for example, ChatOptionsBuilder is a top-level class outside the ChatOptions compilation unit. Adding tests for the options builder class is a good idea.

Created issue #1592 to discuss.

  1. Add builder() to Create New Object from Existing

I see that our current options builder already takes an existing options object, so this is already handled.

  1. Add PortableFunctionCallingOptions to Extract ChatOptions, FunctionsOptions

There is already a class for this, it doesn’t follow the pattern mentioned above (i.e., the builder class is outside the class it builds).

  1. To Separate the Responsibilities of Objects, Implement a Dedicated Object for Properties Binding

We are currently looking to remove the Boot dependency from spring-ai-core, which mostly boils down to removing @NestedConfigurationProperty. We're not sure about the cleanest approach yet, so we are experimenting to avoid adding too much boilerplate. This PR doesn't address that concern and also I don't see the advantage over the implementation you provided.

The separation will happen, and any Boot-specific implementation to achieve this will occur in the auto-configuration classes where the @ConfigurationProperties are located.

Created #1591 for this topic.

I appreciate the contributions, but i will close this issue and the points raised can be tackled through the new issues.