Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot get the service to return anything but a String. #671

Open
FroMage opened this issue Jun 13, 2024 · 23 comments
Open

Cannot get the service to return anything but a String. #671

FroMage opened this issue Jun 13, 2024 · 23 comments
Labels
bug Something isn't working

Comments

@FroMage
Copy link

FroMage commented Jun 13, 2024

The docs at https://docs.quarkiverse.io/quarkus-langchain4j/dev/ai-services.html#_ai_method_return_type say I should be able to get a structured return type from my service, but it does not seem to support anything but a String.

package util;

import java.util.List;

import dev.langchain4j.service.SystemMessage;
import dev.langchain4j.service.UserMessage;
import io.quarkiverse.langchain4j.RegisterAiService;

@RegisterAiService( 
		retrievalAugmentor = ScheduleDocumentRetreiver.class
)
public interface ScheduleAI {

    @SystemMessage("You are a computer science conference organiser") 
    @UserMessage("""
    			I want to find the talks from the conference program that match my interests and constraints.
                Give me the list of talks that match my interests and constraints: {topics}
            """)
    List<AITalk> findTalks(String topics);
    
    public static class AITalk {
    	public String title;
    	public String id;
    }
}
package util;

import java.util.function.Supplier;

import dev.langchain4j.model.embedding.EmbeddingModel;
import dev.langchain4j.rag.DefaultRetrievalAugmentor;
import dev.langchain4j.rag.RetrievalAugmentor;
import dev.langchain4j.rag.content.retriever.EmbeddingStoreContentRetriever;
import io.quarkiverse.langchain4j.pgvector.PgVectorEmbeddingStore;
import jakarta.inject.Singleton;

@Singleton
public class ScheduleDocumentRetreiver implements Supplier<RetrievalAugmentor> {

    private final RetrievalAugmentor augmentor;

    ScheduleDocumentRetreiver(PgVectorEmbeddingStore store, EmbeddingModel model) {
        EmbeddingStoreContentRetriever contentRetriever = EmbeddingStoreContentRetriever.builder()
                .embeddingModel(model)
                .embeddingStore(store)
                .maxResults(3)
                .build();
        augmentor = DefaultRetrievalAugmentor
                .builder()
                .contentRetriever(contentRetriever)
                .build();
    }

    @Override
    public RetrievalAugmentor get() {
        return augmentor;
    }

}
@Path("/")
@Blocking
public class Application {

	@CheckedTemplate
	public static class Templates {
		public static native TemplateInstance ai(String results);
	}

    @Inject
    ScheduleAI ai;
    
    @GET
    @Path("/ai")
    public TemplateInstance ai(@RestQuery String topics){
    	StringBuilder results = new StringBuilder();
    	if(topics != null && !topics.isBlank()) {
    		results.append("Results: ");
    		for (AITalk aiTalk : ai.findTalks(topics)) { // CCE EXCEPTION HERE
        		results.append(aiTalk.id);
        		results.append(", ");
			}
    	}
    	return Templates.ai(results.toString());
    }
}

This generates the following exception:

2024-06-13 12:07:03,893 ERROR [io.qua.ver.htt.run.QuarkusErrorHandler] (executor-thread-1) HTTP Request to /ai?csrf-token=dQpPok314ThUaiceewld7g&topics=I+want+to+learn+about+hibernate%2C+and+web+applications failed, error id: a57d74f1-f0a0-4176-a120-25be5321f0d9-1: java.lang.ClassCastException: class java.lang.String cannot be cast to class util.ScheduleAI$AITalk (java.lang.String is in module java.base of loader 'bootstrap'; util.ScheduleAI$AITalk is in unnamed module of loader io.quarkus.bootstrap.classloading.QuarkusClassLoader @49cb1baf)
	at rest.Application.ai(Application.java:518)
	at rest.Application_ClientProxy.ai(Unknown Source)
	at rest.Application$quarkusrestinvoker$ai_c38fa7a6cc502cf9089d9e3dafbbfabcc91df15a.invoke(Unknown Source)
	at org.jboss.resteasy.reactive.server.handlers.InvocationHandler.handle(InvocationHandler.java:29)
	at io.quarkus.resteasy.reactive.server.runtime.QuarkusResteasyReactiveRequestContext.invokeHandler(QuarkusResteasyReactiveRequestContext.java:141)
	at org.jboss.resteasy.reactive.common.core.AbstractResteasyReactiveContext.run(AbstractResteasyReactiveContext.java:147)
	at io.quarkus.vertx.core.runtime.VertxCoreRecorder$14.runWith(VertxCoreRecorder.java:599)
	at org.jboss.threads.EnhancedQueueExecutor$Task.doRunWith(EnhancedQueueExecutor.java:2516)
	at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2495)
	at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1521)
	at org.jboss.threads.DelegatingRunnable.run(DelegatingRunnable.java:11)
	at org.jboss.threads.ThreadLocalResettingRunnable.run(ThreadLocalResettingRunnable.java:11)
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
	at java.base/java.lang.Thread.run(Thread.java:1583)

So it looks like the generated service interface has String as a method return type.

Also, @geoand told me:

First of all, try returning a domain object, instead of a string
that will result in the prompt being augmented with the proper instructions on how to return the result

But the docs say:

If the prompt defines the JSON response format precisely, you can map the response directly to an object:

So that's contradictory.

@geoand
Copy link
Collaborator

geoand commented Jun 13, 2024

If you can attach a sample application that we can use to run and debug things, that would be most helpful

@FroMage
Copy link
Author

FroMage commented Jun 13, 2024

@geoand
Copy link
Collaborator

geoand commented Jun 13, 2024

Thanks

@geoand
Copy link
Collaborator

geoand commented Jun 13, 2024

I assume I need some data as well?

@FroMage
Copy link
Author

FroMage commented Jun 13, 2024

Nope

@geoand
Copy link
Collaborator

geoand commented Jun 13, 2024

I see what the problem is.

dev.langchain4j.service.ServiceOutputParser from upstream LangChain4j does not really handle collections all that well.
Let me see if we can do better on our side

@FroMage
Copy link
Author

FroMage commented Jun 13, 2024

I tried with arrays before and got a funky error as well.

@geoand
Copy link
Collaborator

geoand commented Jun 13, 2024

Yeah, they don't work great

@geoand
Copy link
Collaborator

geoand commented Jun 13, 2024

Seems like in upstream the methods I need are private, so I'll have to open a PR to open them.

@geoand
Copy link
Collaborator

geoand commented Jun 13, 2024

At some point we probably want to write our own JsonSchema generater from a Jandex type (should be fairly easy), but I don't want to do it now since I have a talk next week.

Maybe I can bait you into doing that? :P

The upstream code that tries to do this is here.
What we need is something that takes org.jboss.jandex.Type and returns a String which is the json schema

@FroMage
Copy link
Author

FroMage commented Jun 13, 2024

I wish I had time to do something like that, but I really can't promise anything soon. I also filed #675 which I hope I might be able to work on in the future.

@geoand
Copy link
Collaborator

geoand commented Jun 13, 2024

🙏🏼

@andreadimaio
Copy link
Collaborator

andreadimaio commented Jun 29, 2024

It would be helpful if the new parser could be customized using the quarkus.langchain4j.response-schema property (today this is a boolean, but can become an object with different values).

For example, today the use of the {response_schema} placeholder is replaced by the default value:

You must respond strictly in the following JSON format: <object_schema>

With the custom parser, we could have control of the prefix, which could be enabled or disabled, with a property like

quarkus.langchain4j.response-schema.prefix={true|false}

This allows the developer to remove the prefix and add something else, for example if they are writing the prompt in a language other than English.

@geoand
Copy link
Collaborator

geoand commented Jul 1, 2024

That's a good point.

We just need to do this at some point: I don't envision it to be more than a day's work (likely less) :)

@geoand geoand added the bug Something isn't working label Jul 4, 2024
@Tarjei400
Copy link
Contributor

Tarjei400 commented Nov 3, 2024

@geoand Do I assume correctly that this is a place to use extension specific json schema generator? I so I will give it a try and try to implement it

@geoand
Copy link
Collaborator

geoand commented Nov 3, 2024

Correct!

But I think that @andreadimaio was looking at something similar...

@Tarjei400
Copy link
Contributor

I see, well I will give it a shot just in case as without mapped collections it is misses a lot of potential :D

@andreadimaio
Copy link
Collaborator

But I think that @andreadimaio was looking at something similar...

I'm looking for how to force the LLM provider to force the use of the json/json-schema mode but not the deserialization of collections.

@Tarjei400
Copy link
Contributor

@geoand I think part in QuarkusServiceOutputParser is one thing, the other is having tools argument to be lists. I found this in ToolProcessor.java is extension deployment:

        if ((type.kind() == Type.Kind.ARRAY)
                || DotNames.LIST.equals(typeName)
                || DotNames.SET.equals(typeName)) { // TODO something else?
            return removeNulls(ARRAY, description); // TODO provide type of array?
        }

Do you happen to have any ideas if its even possible to resolve generic types from Collections there? I think java is eliding generic types at runtime isnt it?

Tarjei400 added a commit to Tarjei400/quarkus-langchain4j that referenced this issue Nov 3, 2024
Tarjei400 added a commit to Tarjei400/quarkus-langchain4j that referenced this issue Nov 3, 2024
…chema, to allow collections in tool arguments
Tarjei400 added a commit to Tarjei400/quarkus-langchain4j that referenced this issue Nov 3, 2024
…chema, to allow collections in tool arguments
Tarjei400 added a commit to Tarjei400/quarkus-langchain4j that referenced this issue Nov 3, 2024
…chema, to allow collections in tool arguments
Tarjei400 added a commit to Tarjei400/quarkus-langchain4j that referenced this issue Nov 3, 2024
…chema, to allow collections in tool arguments
@Tarjei400
Copy link
Contributor

Tarjei400 commented Nov 3, 2024

@geoand I think I was wrong about that type elision, I will try to also put required and optional parameters but in general now it works, but I also need to check when strict set, as this is adding some extra field checks on schema. Then I will also adjust output parser

@geoand
Copy link
Collaborator

geoand commented Nov 4, 2024

Correct!

#1039 seems to cover that pretty well

@geoand geoand closed this as completed in b8417fb Nov 4, 2024
@Tarjei400
Copy link
Contributor

Tarjei400 commented Nov 4, 2024

@geoand Can we reopen this? I wanted to still tackle the case mentioned by @FroMage as well, As mapping results from case he mentions
Is handled via QuarkusServiceOutputParser.java, I will take care of this soon. by having extension local parser instead of using langchain4j as you suggested.

@geoand
Copy link
Collaborator

geoand commented Nov 4, 2024

Oh, I was not aware it was closed automatically

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants