openai-kotlin icon indicating copy to clipboard operation
openai-kotlin copied to clipboard

Update to work with Google Gemini

Open sepatel opened this issue 9 months ago • 12 comments
trafficstars

Q A
Bug fix? yes
New feature? no
BC breaks? yes
Related Issue Fix #412

Describe your change

Enable the id on the ChatCompletion to be nullable. Unfortunately this does technically break backwards compatibility as it was defined as a required field but is not actually provided back by all services that are openai compatible. ~~This is also in direct contradiction of the openai documented standard which states that the id is a required field as well.~~

What problem is this fixing?

The inability for Gemini to work correctly.

sepatel avatar Feb 21 '25 16:02 sepatel

My original description saying that id is supposed to be a required field according to the docs is actually incorrect.

It is probably important to also note though that the gemini api's are actually openai compliant because the missing id field is actually not a required field.

https://platform.openai.com/docs/api-reference/chat/create https://platform.openai.com/docs/api-reference/chat/object

sepatel avatar Feb 23 '25 16:02 sepatel

Don't forget to change streaming chunk too. Google models responses fail with it also.

@Serializable
public final data class ChatCompletionChunk(
    id: String,
    created: Long,
    model: ModelId,
    choices: List<ChatChunk>,
    usage: Usage? = COMPILED_CODE,
    systemFingerprint: String? = COMPILED_CODE
)

stardomains3 avatar Mar 13 '25 04:03 stardomains3

Thank you, it hits as as well.

morki avatar Mar 17 '25 10:03 morki

To not to break the backward compatibility, probably you could use some default value, maybe even autogenerated ID. As for extra parameters, i'd even provide the raw response string and ignore unknown fields, because there are many "openai-compatible" API's which adds extra params here and there (perplexity for example). Universally and easily it would be possible to extract it using JSON node parsing. Otherwise the response object might grow more and more nullable fields. Thats my considerations

Melodeiro avatar Mar 24 '25 06:03 Melodeiro

Nice! Do you mind adding https://jina.ai/deepsearch/ as well?

Melodeiro avatar Apr 06 '25 19:04 Melodeiro

To not to break the backward compatibility, probably you could use some default value

I'm not a fan of "faking" bad data and it creates a bad baseline as well. The official documentation states it can be null, the id value has specific meaning that if a person uses it for that meaning will get messed up by the fake data as well. It is better to break the compatibility or note it for people and move forward with the correct specifications that lineup with the official published documentation as well.

Don't forget to change streaming chunk too. Google models responses fail with it also.

Thanks, I completely forgot about that but I've now pushed up that fix too.

sepatel avatar Apr 06 '25 19:04 sepatel

Good point

Melodeiro avatar Apr 06 '25 19:04 Melodeiro

Nice! Do you mind adding https://jina.ai/deepsearch/ as well?

I think that should be done as a separate PR actually as this one is focused around Gemini. Once it is merged in, I'm happy to add DeepSeek, DeepSearch, Groq, and a few other popular ones just to help make lives a bit easier for people.

sepatel avatar Apr 06 '25 19:04 sepatel

@sepatel Would you mind adding those to your repo while they aren't merged here, please?

Melodeiro avatar Apr 12 '25 22:04 Melodeiro

Hello and thank you for your contribution! 🙌

The library’s top priority is to support the official OpenAI API, since that’s what most users expect. Support for other providers is a welcome bonus, as long as it doesn’t break our core API.

In this particular case, the only classes we need to change are response models; users rarely instantiate those directly, but they do expect all fields to be present when they access them. With that in mind, I propose:

  1. Rename the constructor parameter to
    @SerialName("id") val idOrNull: String? = null
    
  2. Add a non-nullable accessor:
    val id: String
        get() = requireNotNull(idOrNull)
    

This approach handles both concerns:

  1. Deserialization allows id to be missing or null
  2. Field access enforces non-null behavior.

This is still an API-breaking change (the constructor signature has been altered), but it’s the safest way to address nullable deserialization without compromising the non-nullable API surface.

aallam avatar Apr 23 '25 09:04 aallam

Thanks @aallam ,

I've made the recommended updates, however I will point out that it is still breaking backwards compatibility because anyone who does named param settings will have to rename the constructor parameters.

Also, if code is already compiled and this library is replaced, it will cause a signature error as well forcing things to be recompiled (this part I'm less concerned with but it is a difference in API vs ABI as well). The original making of the constructor nullable was what I felt the safest way to transition forward.

Update: Got the following build failure

com.aallam.openai.api.exception.AuthenticationException: Incorrect API key provided: ''. You can find your API key at https://platform.openai.com/account/api-keys.

As I am able to export my personal openai keys and see all the tests passing, I'm unclear as to what is off or how these changes impact the build.

sepatel avatar May 04 '25 10:05 sepatel

@sepatel cc @aallam Thanks for your work. I have used this PR to create a fork of openai-kotlin and published it in the Sonatype Maven Central repository. Here is the URL: https://github.com/maxrave-dev/gemini-kotlin

maxrave-dev avatar May 25 '25 07:05 maxrave-dev