Builders are unnecessarily complex in some cases

[Rocky0102]

Are those all kinds of builder necessary?

[Rocky0102]

Since ChatCompletionUserMessageParam used, why role is not set automatically, it's very annoying.
`

public static void main(String[] args) {
    OpenAIClient client = OpenAIOkHttpClient.fromEnv();
    ChatCompletionCreateParams completionCreateParams = ChatCompletionCreateParams.builder()
            .model(ChatModel.GPT_3_5_TURBO)
            .maxTokens(1024)
            .addMessage(ChatCompletionMessageParam.ofChatCompletionUserMessageParam(
                    ChatCompletionUserMessageParam.builder()
                            .role(ChatCompletionUserMessageParam.Role.USER)
                            .content(ChatCompletionUserMessageParam.Content.ofTextContent(
                                    "Tell me a story about building the best SDK!"
                            ))
                            .build()
            ))
            .build();

`

[TomerAberbach]

Since ChatCompletionUserMessageParam used, why role is not set automatically, it's very annoying.

Yes, I agree! We are actually planning to update the SDK to automatically set this value for you.

Are those all kinds of builder necessary?

In Java, builders are the common approach to make construction of complex deeply nested objects readable and flexible. Otherwise you'd have to pass the parameters to a constructor in the right order, which is easy to get wrong.

Curious if you encountered any difficulties other than the role one you mentioned? We'd be eager to address those as well :)

[sahil28032005]

still good approach

[TomerAberbach]

still good approach

Are you referring to the builders or something else?

[sahil28032005]

ya i am talking regarding builder

[stargazer33]

In Java, builders are the common approach ...

Well, using builders make sense in SOME situations where you have 6-7 or more constructor arguments

Otherwise you'd have to pass the parameters to a constructor in the right order

I am capable to pass 3-4-5 parameters to constructor in right order, this is not a problem (and having Javadoc helps)
By the way, you can have multiple constructors with different number of parameters, kind of "syntax sugar"

In my code I have to do this (unfortunately):

messageParam = ofChatCompletionUserMessageParam(
    ChatCompletionUserMessageParam
    .builder()
    .role(ChatCompletionUserMessageParam.Role.USER)
    .content( ChatCompletionUserMessageParam.Content.ofTextContent(msg.msg) )
    .build() );

...

messageParam = ofChatCompletionAssistantMessageParam(
    ChatCompletionAssistantMessageParam
    .builder()
    .role(ChatCompletionAssistantMessageParam.Role.ASSISTANT)
    .content( ChatCompletionAssistantMessageParam.Content.ofTextContent( msg.msg ) )
    .build() );

This code above seriously over-engineered (because it use over-engineered API):
I have to use word "Assistant" 3 times (the same applies to "User"):

ChatCompletionAssistantMessageParam, 
ChatCompletionAssistantMessageParam.Role.ASSISTANT
ChatCompletionAssistantMessageParam.Content.ofTextContent

The question is WHY?
Remember, all we want to build is just this simple JSON (only ONE use of word "assistant"):

            {
                "role": "user",
                "content": "..."
            },
            {
                "role": "assistant",
                "content": "... "
            }

[TomerAberbach]

In Java, builders are the common approach ...

Well, using builders make sense in SOME situations where you have 6-7 or more constructor arguments

Otherwise you'd have to pass the parameters to a constructor in the right order

I am capable to pass 3-4-5 parameters to constructor in right order, this is not a problem (and having Javadoc helps) By the way, you can have multiple constructors with different number of parameters, kind of "syntax sugar"

I hear you, but there are other benefits:

• Builders scale better as the API grows. Even if we have relatively few parameters now, we may add more parameters in the future, which will make a constructor not ideal. At that point swapping to a builder would be a breaking change, and we care deeply about avoiding breaking changes. Starting with a builder avoids the problem.

• Builders let you build up the object "over time" more easily (i.e. without many local variables, can pass the builder around across functions as a first-class value, etc.)

• Understood that you could figure out how to pass parameters in the right order, but:

  • It would be easy to mess up, especially if many of the parameters are primitives. Everyone inevitably makes mistakes when writing code, and we want to decrease the likelihood of that happening! i.e. make it easy to fall into the Pit of Success.

  • The code would not be readable. For example, if we wrote new ChatCompletionCreateParams(ChatModel.GPT_3_5_TURBO, 500, ...), it's not clear what the 500 means without looking at the Javadoc. Of course you can read the Javadoc, but ideally the code is self-documenting and easy to skim.

• Making a constructor for every possible ordering doesn't solve all of these problems, and scales ~exponentially with the number of parameters.

In my code I have to do this (unfortunately):

messageParam = ofChatCompletionUserMessageParam(
    ChatCompletionUserMessageParam
    .builder()
    .role(ChatCompletionUserMessageParam.Role.USER)
    .content( ChatCompletionUserMessageParam.Content.ofTextContent(msg.msg) )
    .build() );

...

messageParam = ofChatCompletionAssistantMessageParam(
    ChatCompletionAssistantMessageParam
    .builder()
    .role(ChatCompletionAssistantMessageParam.Role.ASSISTANT)
    .content( ChatCompletionAssistantMessageParam.Content.ofTextContent( msg.msg ) )
    .build() );

This code above seriously over-engineered (because it use over-engineered API): I have to use word "Assistant" 3 times (the same applies to "User"):

ChatCompletionAssistantMessageParam, 
ChatCompletionAssistantMessageParam.Role.ASSISTANT
ChatCompletionAssistantMessageParam.Content.ofTextContent

The question is WHY? Remember, all we want to build is just this simple JSON (only ONE use of word "assistant"):

            {
                "role": "user",
                "content": "..."
            },
            {
                "role": "assistant",
                "content": "... "
            }

I agree this code is not ideal!

As I mentioned above, we're working on improving this part. Here are some of the upcoming improvements that we're planning to make before the beta release (SDK is currently alpha):

• Setting single value enums for you (e.g. no need to call role(...) since it's implied by ChatCompletionAssistantMessageParam)
• Shortening some union factory function names (e.g. ofChatCompletionAssistantMessageParam is pretty verbose)
• Letting you omit union factory functions in some cases with overloads (e.g. allow passing the message directly to addMessage without calling ofChatCompletionAssistantMessageParam, allow passing a string directly to content, etc.)
• Perhaps some other things to make the API more delightful. If you feel the above improvements wouldn't address all your concerns, then definitely let me know. We want the API to be great :)

To answer "why" it's currently like this: we are generating this code from the OpenAI OpenAPI spec using our Stainless code generator and we're still iterating! I can assure you we'll work out all the issues before the GA release of the SDK.

[codefromthecrypt]

I didn't see this and opened up a redundant one. I think we should probably distance from polarizing thoughts (builder good vs bad) and more into making a simpler api, like exist in other languages in openai and like exist in other SDKs in java

this was the issue I raised about this vs python #81

Here's an example of spring-ai which is also a lot easier to read due to a lot less symbols and methods in the foreground, despite it using a builder pattern

ChatResponse response = chatModel.call(
    new Prompt(
        "Generate the names of 5 famous pirates.",
        OpenAiChatOptions.builder()
            .model("gpt-4-o")
            .temperature(0.4)
        .build()
    ));

If we decide to use an approach that looks like auto-generation, what's likely to happen is folks will release their own libraries that wrap this one to avoid complicated code. That's fine, but there are drawbacks especially in a new project notably version lock-ups, etc. I really hope we can lean into the UX here and solve it here.

[TomerAberbach]

Did you read through the future improvements I mentioned above that should simplify the API? Any thoughts about that?

[codefromthecrypt]

Enums(or constants), shorter factory names, and overloads sound like progress.

A 'diff' block quote (triple backtick) of the before after using the README example might help get buy in from others as it will show the end game in a way that doesn't require a mental refactoring tool ;)

[TomerAberbach]

If we decide to use an approach that looks like auto-generation, what's likely to happen is folks will release their own libraries that wrap this one to avoid complicated code. That's fine, but there are drawbacks especially in a new project notably version lock-ups, etc. I really hope we can lean into the UX here and solve it here.

I also want to clarify that I strongly believe we can generate a great SDK that doesn't require wrapping. The Python SDK is fully generated by Stainless too :)

We care a great deal about good UX. It's just that this SDK is still in alpha, so we're iterating.

Enums(or constants), shorter factory names, and overloads sound like progress.

A 'diff' block quote (triple backtick) of the before after using the README example might help get buy in from others as it will show the end game in a way that doesn't require a mental refactoring tool ;)

Good point :)

My initial goal is something like this:

import com.openai.models.ChatCompletion;
import com.openai.models.ChatCompletionCreateParams;
import com.openai.models.ChatCompletionUserMessageParam;
import com.openai.models.ChatModel;

ChatCompletionCreateParams params = ChatCompletionCreateParams.builder()
    .addMessage(
        ChatCompletionUserMessageParam.builder()
            .content("Say this is a test")
            .build())
    .model(ChatModel.O1)
    .build();
ChatCompletion chatCompletion = client.chat().completions().create(params);

You could call addMessage as many times as you want. You can omit the role, since it's implied, and you can omit wrapping in the union type (addMessage will wrap internally).

And after that, I'm thinking of maybe also adding overloads for user and assistant messages (e.g. addUserMessage and addAssistantMessage).

import com.openai.models.ChatCompletion;
import com.openai.models.ChatCompletionCreateParams;
import com.openai.models.ChatModel;

ChatCompletionCreateParams params = ChatCompletionCreateParams.builder()
    .addUserMessage("Say this is a test")
    .model(ChatModel.O1)
    .build();
ChatCompletion chatCompletion = client.chat().completions().create(params);

[codefromthecrypt]

lovely. I might suggest that in the optimized case you could consider addMessage to default to user, or have a param of the role. regardless, looks like a fantastic outcome.

[codefromthecrypt]

ps motivation is important. I work at Elastic and we are working on opentelemetry instrumentation for this SDK. I want to put an "easy example" together and then have it traced (technically also metrics and logs) by default. So, I ran into this as I wondered if it was better to do a showcase with something as simple as we can make it vs the now state.

[TomerAberbach]

As of v0.13.0 this entirely implemented with the exception of addUserMessage

[codefromthecrypt]

curious.. was this change intentional?

-                                  val.asResponseFormatText().type().toString());
+                                  val.asResponseFormatText()._type().toString());

[TomerAberbach]

Yup, the convention we have is that raw JsonValue fields are exposed with underscore prefix, since it's unlikely you'll need to access them.

For example, in this case you don't really need to ever access the _type() method because it's implied by which class you're dealing with. It's a leftover artifact from deserializing the server response. We still expose it in case you wanna look at the value for whatever reason though.

Let me know if you have any thoughts or feedback about that!

[codefromthecrypt]

@TomerAberbach thanks. that part was in internal instrumentation, so ok if stable.

For user-side, what is the replacement constant for this, assuming you want to say set it to assistant?

        .role(ChatCompletionUserMessageParam.Role.USER)

[codefromthecrypt]

also these. Not sure the new constants to use here.

       ChatCompletionTool.builder().type(ChatCompletionTool.Type.FUNCTION)

...

ResponseFormatText.builder().type(ResponseFormatText.Type.TEXT)

[TomerAberbach]

The replacement is you don't have to specify it at all!

The fact that the role is user is implied by the fact that you are using ChatCompletionUserMessageParam (the name of the class has User in it). Same concept for ChatCompletionDeveloperMessageParam (the implicit role is developer), etc. The role field is now more of an internal implementation detail of how the API needs to be called, but you don't have to worry about it anymore when using the SDK.

It is also no longer necessary to set type for ChatCompletionTool and ResponseFormatText for similar reasons. The type of ChatCompletionTool is always function and the type of ResponseFormatText is always text, so they are set for you automatically.

So for any constant that you've noticed has disappeared in v0.13.0, you can assume that we now automatically set it for you.

[codefromthecrypt]

Thanks for the help, I suspect this will build just need to wait on it to finish ;) elastic/elastic-otel-java#514

[TomerAberbach]

Nice!

Do let me know if you have any feedback or thoughts on the changes. e.g. if you find it intuitive, if you find something confusing, etc.

[codefromthecrypt]

Do let me know if you have any feedback or thoughts on the changes. e.g. if you find it intuitive, if you find something confusing, etc.

So far, I really like the changes and would close out this issue. The drift isn't a big deal and worth it.

[codefromthecrypt]

@TomerAberbach we have a test for explicit response format, basically that the json ends up correct even if default. Is there a constant form of this, or is builder().build() the way?

        ChatCompletionCreateParams.builder()
            .responseFormat(ResponseFormatText.builder().build())

[codefromthecrypt]

ps blog went out which has the old syntax until our agent is rebuilt to support latest. Thanks for all the help and also releasing this SDK. https://www.elastic.co/observability-labs/blog/elastic-opentelemetry-openai

[TomerAberbach]

Right now .responseFormat(ResponseFormatText.builder().build()) is the only way in this case.

We can't make responseFormat have a constant default because it can have multiple values. responseFormat can be either a ResponseFormatText, ResponseFormatJsonObject, or ResponseFormatJsonSchema.

However, maybe there's an opportunity here to optimize for these cases where you'd have to write SomeClass.builder().builder()... I could imagine ChatCompletionCreateParams.builder().responseFormatText() being a nice shorthand. I'll think on it a bit

ps blog went out which has the old syntax until our agent is rebuilt to support latest. Thanks for all the help and also releasing this SDK. elastic.co/observability-labs/blog/elastic-opentelemetry-openai

Nice! I saw your PR was merged. Anything left before you can update the blog post?

[TomerAberbach]

With the release of v0.18.0, builders should be lovely and simple to use.

See the readme and the examples for usage patterns.

If you encounter further issues, then please file a new issue for that specific problem