Allow specifying the organizationId in the configuration

[edeandrea]

See https://platform.openai.com/docs/api-reference/organization-optional - allow the organizationId to be specified in the configuration.

Fixes #131

[edeandrea]

Just an implementation note. In OpenAiRestApi I didn't add an @ClientHeaderParam annotation because then I would have had to modify every method signature to add a @NotBody String organizationId parameter to it. I thought instead adding a ResteasyReactiveClientRequestFilter that conditionally added the header would be a better/cleaner approach.

[geoand]

Thanks for this.

I'll have to think about this, I'm not really a fan of the filter for this case.

This might be an opportunity actually to improve the REST Client...
But like I said, I have to think about it 🙂

[edeandrea]

Yeah I'd much prefer to have just the annotation as well, but I thought having to add an extra method parameter to every method, when a lot of the time the value is null wasn't the best option either. This way, the filter is only ever added to the chain if the value is even specified, which is better IMO.

[geoand]

The thing is that I don't want to have two different approaches for the parameters if it can be avoided.

I'm thinking about some improvements to the @BeanParam support of the REST Client (I think we support it, but I could be mistaken) that could apply in this case

[edeandrea]

I thought about making the filter more generic as well, something like a ClientHeaderFilter where we could configure it with different headers, rather than a specific filter for each set of headers we might want. But for now I kept it simple.

Some of the other edits (like transforming annonymous class instances to lambdas) we done by IntelliJ automatically. I can turn that off and revert those changes if you'd prefer.

[geoand]

Yeah, no lambdas please 🙂.

But in any case, I need to look into what I mentioned above before moving this ahead

[edeandrea]

Yeah, no lambdas please

Ok I'll revert that. Can I ask why?

I feel like

return builder::build;

is way cleaner than

return new Supplier<>() {
            @Override
            public Object get() {
                return builder.build();
            }
        };

and

public void cleanUp(ShutdownContext shutdown) {
     shutdown.addShutdownTask(QuarkusOpenAiClient::clearCache);
}

is way cleaner than

    public void cleanUp(ShutdownContext shutdown) {
        shutdown.addShutdownTask(new Runnable() {
            @Override
            public void run() {
                QuarkusOpenAiClient.clearCache();
            }
        });
    }

[geoand]

Ok I'll revert that. Can I ask why?

Certainly!
The reason is that early on during Quarkus' development, we found that when a larger number of lambdas where bootstrapped during application initialization, startup time and RSS took a small hit.

There is no doubt that the lambdas and method references look cleaner in this case, it's just a case of us trying to eliminate lambdas in runtime code as much as possible.

[geoand]

Assuming

• Use @BeanParams in QuarkusRestApiResource #198

works fine (it works just fine in whatever test I ran), this should be rebased onto it and it will become simpler

[edeandrea]

I'll take a look at that and rebase onto this. Had somewhat of a pet medical emergency today so I might not get to it until tomorrow.

[geoand]

Hopefully your pet will be fine!

[edeandrea]

Thanks!

So I started rebasing this and saw what you did with the ApiMetadata metadata class and @BeanParam. Question - this organizationId header is optional, meaning that it shouldn't always be set/sent, and should never be sent in the Azure OpenAI case.

If I add another @HeaderParam value in the class, will that value be sent if the value is null?

[geoand]

A header (or whatever) will not be sent if the value is null

[edeandrea]

A header (or whatever) will not be sent if the value is null

Thats what I needed to know. I'll work on this new integration now.

Just a thought - instead of adding a new method parameter to public static ApiMetadata of(String apiKey, String apiVersion) what if I instead created a builder for the ApiMetaData? That way in the future whenever we want to add new things we don't have to touch everywhere in the codebase where we use that specific method?

[geoand]

Yeah, we can certainly have a builder, but we need to be careful of how the key translates to the two headers

[edeandrea]

Yeah, we can certainly have a builder, but we need to be careful of how the key translates to the two headers

I'll put something together and you can review.

[geoand]

👌

[edeandrea]

Just pushed changes. Let me know what you think.

[geoand]

I added a minor comment.

Also, it would be nice to have the commits squashed

[geoand]

I'd prefer we don't use star imports

[edeandrea]

I ran mvn impsort:sort from the project root and thats what it produced. If I commit without running that then the build fails saying I need to run it...

[geoand]

Interesting

[edeandrea]

Also, it would be nice to have the commits squashed

They are. Maybe you were looking at it just as I was squashing.

[geoand]

Thanks a lot!

[edeandrea]

Thanks a lot!

You're welcome! I like the builder approach coupled with the metadata @BeanParam. I think its nice and flexible.

I'll still agree to disagree that

if (someOptional.isPresent()) {
  someOptional.get();
}

is an anti-pattern.

Optional<String> apiKeyOpt = runtimeConfig.apiKey();
if (apiKeyOpt.isEmpty()) {
    throw new ConfigValidationException(createApiKeyConfigProblems());
}
ChatModelConfig chatModelConfig = runtimeConfig.chatModel();
var builder = OpenAiChatModel.builder()
        .apiKey(apiKeyOpt.get());

would be much better as

var apiKey = runtimeConfig.apiKey()
    .orElseThrow(() -> new ConfigValidationException(createApiKeyConfigProblems()));
ChatModelConfig chatModelConfig = runtimeConfig.chatModel();
var builder = OpenAiChatModel.builder()
        .apiKey(apiKey);

[geoand]

I don't think I said it's an antipattern.
I definitely did say that for reasons of runtime footprint we avoid lambdas wherever possible

[edeandrea]

I don't think I said it's an antipattern.

No, I don't think you did. I'm saying it is :)

[edeandrea]

Not sure why the native java 21 build is failing. Its timing out when creating the binary. I've seen that happen in other apps for java 21. Takes longer to build the image.

[geoand]

That's a bug in GraalVM that we need to address.

Not related to this PR