sashirestela / simple-openai

A Java library to use the OpenAI Api in the simplest possible way.
MIT License
182 stars 17 forks source link

Support for using custom Json ObjectMapper #205

Closed the-gigi closed 3 days ago

the-gigi commented 3 weeks ago

Here is the issue. This specific instance happened during tool calls.

Oct 30 18:01:37.882
i-0c37647db12b51de5
invisible
Caused by: com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Java 8 date/time type `java.time.LocalDate` not supported by default: add Module "com.fasterxml.jackson.datatype:jackson-datatype-jsr310" to enable handling

Oct 30 18:01:37.882
i-0c37647db12b51de5
invisible
... 51 more

Oct 30 18:01:37.882
i-0c37647db12b51de5
invisible
at io.github.sashirestela.openai.common.function.FunctionExecutor.execute(FunctionExecutor.java:81)

The FunctionExecutor calls Cleverclient's JsonUtil to deserialize JSON objects.

        try {
            var function = mapFunctions.get(functionName);
            var object = JsonUtil.jsonToObject(
                    functionCall.getArguments().isBlank() ? "{}" : functionCall.getArguments(),
                    function.getFunctionalClass());
            return (T) object.execute();
        } catch (RuntimeException e) {
            throw new SimpleUncheckedException("Cannot execute the function {0}.", functionName, e);
        }

If the functional class contains types that are not registered by the vanilla object mapper / object reader in Cleverclient's JsonUtil we have a problem.

public class JsonUtil {

    private static final ObjectMapper objectMapperStrict = new ObjectMapper();

    private static final ObjectReader objectReaderIgnoringUnknown = objectMapperStrict.reader()
            .without(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);
    .
    .
    .
    public static <T> T jsonToObject(String json, Class<T> clazz) {
        try {
            return objectReaderIgnoringUnknown.readValue(json, clazz);
        } catch (IOException e) {
            throw new CleverClientException("Cannot convert the Json {0} to class {1}.", json, clazz.getName(), e);
        }
    }

Since the ObjectReader is generated from a vanilla ObjectMapper that doesn't register the com.fasterxml.jackson.datatype.jsr310.JavaTimeModule it can't handle dates. That's fine. Cleverclient shouldn't register any possible module. But, it should be possible to pass an external ObjectMapper/ObjectReader to all the methods in JsonUtil.

I opened an issue for Cleverclient: https://github.com/sashirestela/cleverclient/issues/77

Once, it is possible to provide a custom object mapper/object reader to JsonUtil, then, the FunctionExecutor should be able to pass a custom ObjectReader to Cleverclient's JsonUtil.jsonToObject() method

Probably overload the execute() and executeAll() methods like so:

public <T> T execute(FunctionCall functionCall, ObjectReader objectReader) {
   ...
           try {
            var function = mapFunctions.get(functionName);
            var object = JsonUtil.jsonToObject(
                    functionCall.getArguments().isBlank() ? "{}" : functionCall.getArguments(),
                    function.getFunctionalClass(),
                    objectReader);
            return (T) object.execute();
        } catch (RuntimeException e) {
            throw new SimpleUncheckedException("Cannot execute the function {0}.", functionName, e);
        }
       ...
}
kdubb commented 2 weeks ago

Setting an ObjectMapper on the SimpleOpenAI.Builder would be helpful, preferred actually. When using SimpleOpenAI from Kotlin you generally want "everything" to include the Kotlin module.

the-gigi commented 2 weeks ago

Setting an ObjectMapper on the SimpleOpenAI.Builder would be helpful, preferred actually. When using SimpleOpenAI from Kotlin you generally want "everything" to include the Kotlin module.

The issue is that for different calls to jsonToObject() you may want a different ObjectReader. If it is set at the SimpleOpenAI level then whenever you need a different ObjectReader you'll have to create a new instance of SimpleOpenAI (which means passing around the API key + other initialization data).

kdubb commented 2 weeks ago

My use of preferred was in reference to our situation, needing to register a single mapper to make "anything" work. I wasn't discounting your suggestion. It seems easy enough to provide both!

the-gigi commented 2 weeks ago

Agreed. Both options can be useful for different workflows.

sashirestela commented 5 days ago

@the-gigi Due to my limited time availability, I prefer the option proposed by @kdubb, which involves less change. I think it could cover your requirements, but I would like to know if you agree, so I can start working on this change.

the-gigi commented 4 days ago

@sashirestela Yes. We can make it work for our requirements. We can aggregate the custom data types we need across all use cases into one big unified ObjectMapper that can serialize everything we need.

Thanks for adding this important capability.

the-gigi commented 4 days ago

@sashirestela I'm also happy to see the usage of simple-openai growing and more and more requests are coming in. Let me know if I can help with a PR or anything else.