[Feature] Added per-call headers capabilities to OpenAiClient

[singularity-sg]

Description

In order to better facilitate integration with OpenAI proxies, we want to be able to inject headers per request so that these headers could be used for auxiliary handling of actual OpenAI calls e.g use case being usage tracking and rate limits.

Approach

The approach is to retain much of the OpenAiClient interface and calls but extending the capabilities to send extra headers per request. This requires us to open up the abstract methods to take in a new inner class in OpenAiClient.OpenAiClientContext as a method argument. We can extend this context so that other implementations of OpenAiClient abstract class could use the same context object to add in other metadata if needed for executing OpenAi API methods.

For now, I've only added headers in the OpenAiClientContext but I could imagine this being used for other cases as well. The default signature is retained for the OpenAiClient so we can still call chatCompletion(ChatCompletionRequest) as it is and it will simply pass along a default OpenAiClientContext

cc: @langchain4j

[singularity-sg]

There was 1 test that failed due to to a strange issue where the weather unit is described as FELSIUS where I think it was trying to say CELSIUS or FAHRENHEIT . @langchain4j could you help review this PR if it makes sense?

[singularity-sg]

I ran the test on IntelliJ manually and it seemed to pass everything (including the one that failed in the mvn test)

[langchain4j]

@singularity-sg sorry, but this PR is very hard to review. Please revert all the changed that only do re-formatting

[singularity-sg]

@singularity-sg sorry, but this PR is very hard to review. Please revert all the changed that only do re-formatting

Apologies for that - I think the formatting by the IDE caused the differences due to our differing formatting rules. I've only included the required changes now

[singularity-sg]

Hi @langchain4j could you help me review this PR when you are available 🙏🏼

[langchain4j]

Hi @singularity-sg I am trying to review everything in the order of priority, I have too much PRs to review now. Thanks a lot for patience and understanding!

[singularity-sg]

Hi @singularity-sg I am trying to review everything in the order of priority, I have too much PRs to review now. Thanks a lot for patience and understanding!

No worries! I appreciate your time in this 🙏🏼

[singularity-sg]

Hi @langchain4j ,

Sorry for sounding pushy but it'll help with our integration with langchain4j framework if we could unblock this PR. Appreciate your help in this 🙏🏼

[langchain4j]

@singularity-sg do you plan to make a similar change in langchain4j?

[singularity-sg]

@singularity-sg do you plan to make a similar change in langchain4j? @langchain4j cmiimw but once I've merged this PR, wouldn't that create a new version that would be picked up by the langchain4j-open-ai module since it takes the latest version from the pom.xml . If there's anything else I need to do, I'll be happy to do so 🙏🏼

[langchain4j]

No, I mean the alignment with the current lc4j api. There is now no way to define parameters per request now. Although there is a plan to make ChatLanguageModel API more flexible (e.g. to set parameters per request), it is not there yet.

[singularity-sg]

No, I mean the alignment with the current lc4j api. There is now no way to define parameters per request now. Although there is a plan to make ChatLanguageModel API more flexible (e.g. to set parameters per request), it is not there yet.

I see what you mean. I've not made any modifications to the existing API contracts (just adding more method for flexibility). I can enhance LC4J so that it can make use of these new methods for enhancing the per request calls but merging this PR change should not break the existing contract. Once this is merged and a new version created, I can work on the PR on langchain4j-open-ai to utilize those new methods

[langchain4j]

Is there a way to make this change non-breaking? OpenAiClient class is also extended by Quarkus extension.

[singularity-sg]

Is there a way to make this change non-breaking? OpenAiClient class is also extended by Quarkus extension.

I don't think there's a good way to do that without having to change the Quarkus extension. But even if the extension was to be upgraded, the change would simply be as simple as this if there is no need for per-request calls.

Also, it appears that the repo is in maintenance mode already since there is a message on the repo that says This repository has been archived by the owner on Nov 13, 2023. It is now read-only.

@langchain4j do you think we could proceed to have this merged first and I could raise a separate PR for Quarkus Extension later

[singularity-sg]

@langchain4j It appears that the GPT_4_VISION_PREVIEW model has been deprecated and we can either switch to gpt-4-turbo or gpt-4o - both which supports vision with OpenAI. I've taken out that model and will add in those 2 perhaps?

It also looks like the OPENAI key is not set up for the repo so tests are failing to run

[langchain4j]

I don't think there's a good way to do that without having to change the Quarkus extension. But even if the extension was to be upgraded, the change would simply be as simple as this if there is no need for per-request calls.

When you add new abstract methods to the class/interface, you can always add a default implementation (e.g. throwing a "not implemented" exception) to not break things for everyone who extends this interface. They can always provide their implementation when needed, but they won't be forced to do so if they are not interested in this particular feature.

It appears that the GPT_4_VISION_PREVIEW model has been deprecated and we can either switch to gpt-4-turbo or gpt-4o - both which supports vision with OpenAI. I've taken out that model and will add in those 2 perhaps?

Please add new models and mark GPT_4_VISION_PREVIEW as @Deprecated (instead of removing it)

It also looks like the OPENAI key is not set up for the repo so tests are failing to run

I've updated the key, could you please check?

[singularity-sg]

you can always add a default implementation (e.g. throwing a "not implemented" exception) to not break things for everyone who extends this interface

Ok, I got the intention now - I've made those methods throw NotImplementedException instead of forcing subclasses to inherit the method via an abstract method. Since the OpenAiClient class is an abstract class and not an interface, I wasn't able to create a default method but I think this also arrives at the same goal of not breaking existing subclasses.

Please add new models and mark GPT_4_VISION_PREVIEW as @Deprecated (instead of removing it)

That is done - also I have marked some test cases to exclude GPT_4_VISION_PREVIEW since OpenAI now doesn't accept that as a model option.

I've updated the key, could you please check?

It looks like the tests are still failing because it can't find OPENAI_API_KEY. Is it set in the repo secrets ?

[langchain4j]

It looks like the tests are still failing because it can't find OPENAI_API_KEY. Is it set in the repo secrets ?

Yes, it is in secrets. I've run all the tests locally and they are green

[singularity-sg]

It looks like the tests are still failing because it can't find OPENAI_API_KEY. Is it set in the repo secrets ?

Yes, it is in secrets. I've run all the tests locally and they are green

The tests are all passing locally for me when I set the OPENAI_API_KEY that I have. However, the status checks for Github Actions are failing. Could you help investigate this please @langchain4j ? I don't have the admin rights thus I can't view the settings for this repo

[langchain4j]

@singularity-sg oh sorry, I forgot to merge it.

I guess tests fail because you did not set OPENAI_API_KEY secret in github fork and my secret is not accessible for your fork. Anyway, no worries, I will merge it now.

[singularity-sg]

@singularity-sg oh sorry, I forgot to merge it.

I guess tests fail because you did not set OPENAI_API_KEY secret in github fork and my secret is not accessible for your fork. Anyway, no worries, I will merge it now.

Thank you for your help once again! much appreciated 🙏🏼