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

Allow the use of the {response_schema} in the prompt #699

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

andreadimaio
Copy link
Contributor

@andreadimaio andreadimaio commented Jun 23, 2024

As mentioned in #679, the idea of this PR is to increase the flexibility of prompt creation with the use of the {response_schema} placeholder.

This new placeholder allows the developer to choose where to place the schema in the prompt.

@RegisterAiService
public interface LLMExtractor {


    @UserMessage("""
    <|begin_of_text|><|start_header_id|>system<|end_header_id|>

     # IDENTITY and PURPOSE

     You are a entity extractor. You take input and output a JSON.
     Your goal is to:

    * Extract the entities from the text: ["firstname", "lastname", "email", "city"];
    * Respond only with the entities extracted from the text;
    * Capitalize the first letter of the firstname, lastname, and city entities;
    * {response_schema};

    <|eot_id|>
    <|start_header_id|>user<|end_header_id|>

     # INPUT

     INPUT: {text}

    <|eot_id|><|start_header_id|>assistant<|end_header_id|>
     """)
    public User user(String text);
}

If there is no {response_schema} placeholder in the @SystemMessage or @UserMessage, the current default behavior is executed (the schema is added at the end of the prompt).

If necessary, the developer can disable the schema generation using the quarkus.langchain4j.response-schema property to false.

The new placeholder works with all possible AIService interactions except @StructuredPrompt.

@andreadimaio andreadimaio requested a review from a team as a code owner June 23, 2024 11:15
@@ -113,6 +113,14 @@ TriagedReview triage(String review);

In this instance, Quarkus automatically creates an instance of `TriagedReview` from the LLM's JSON response.

To enhance the flexibility of prompt creation, the `{response_schema}` placeholder can be used within the prompt. This placeholder is dynamically replaced with the defined schema of the method's return object.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
To enhance the flexibility of prompt creation, the `{response_schema}` placeholder can be used within the prompt. This placeholder is dynamically replaced with the defined schema of the method's return object.
To enhance the flexibility of prompt creation, the `+{response_schema}+` placeholder can be used within the prompt. This placeholder is dynamically replaced with the defined schema of the method's return object.

This is to avoid Antora trying to resolve this as an Antora attribute named response_schema - I can see this in the build log:

[WARNING] [io.quarkiverse.antora.deployment.NativeImageBuildRunner] /antora/docs/modules/ROOT/pages/ai-services.adoc:0: skipping reference to missing attribute: response_schema

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that one solution to avoid the warning is to use \{response_schema\}.


[IMPORTANT]
====
By default, if the placeholder `{response_schema}` is not present in `@SystemMessage` or `@UserMessage`, it will be added to the end of the prompt.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
By default, if the placeholder `{response_schema}` is not present in `@SystemMessage` or `@UserMessage`, it will be added to the end of the prompt.
By default, if the placeholder `+{response_schema}+` is not present in `@SystemMessage` or `@UserMessage`, it will be added to the end of the prompt.

dtto as in https://github.com/quarkiverse/quarkus-langchain4j/pull/699/files#r1650503191

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that one solution to avoid the warning is to use \{response_schema\}.

@@ -36,6 +36,12 @@ public interface LangChain4jBuildConfig {
*/
DevServicesConfig devservices();

/**
* Whether the response schema can be used in the prompt
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This description isn't very clear to me - should it say

the {response_schema} placeholder

instead of just

response schema

?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe repeat here what you said in #679 (comment) :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I can think of way to improve this comment.

@jmartisk
Copy link
Collaborator

Btw are we sure this isn't something that would better fit into the upstream langchain4j project? For example, what if that project decides to introduce the same feature at some point in the future, how would we handle the duplication...

@andreadimaio
Copy link
Contributor Author

Btw are we sure this isn't something that would better fit into the upstream langchain4j project? For example, what if that project decides to introduce the same feature at some point in the future, how would we handle the duplication...

Yes, maybe this is something that can be useful to have in the langchain4j repo. I sped up the implementation in quarkus because otherwise I was blocked from moving forward with creating prompts in projects.

@andreadimaio
Copy link
Contributor Author

Of course I can use a workaround like returning a String and using the ObjectMapper to have the output bean.
But this PR should avoid all that. If you think it is better to implement this feature directly in langchain4j we can close this PR :)

@jmartisk
Copy link
Collaborator

Yeah, I would say that we should generally attempt to implement things in langchain4j wherever appropriate... to avoid the potential problems in the future where it decides to do a similar feature, and we have to reconcile the duplication, etc. That is, unless the maintainer is against having this - what do you think about this feature @langchain4j ?

@jmartisk
Copy link
Collaborator

Even if it's implemented upstream, it should still be possible to have a Quarkus-side configuration property wired to it (I'm not against that)

@langchain4j
Copy link
Collaborator

I am not fully sure about this exact solution (need more time to check), but it definitely makes sense to make it configurable in LC4j. BTW, there are a couple of related PRs: langchain4j/langchain4j#1176 langchain4j/langchain4j#1126

@andreadimaio
Copy link
Contributor Author

I am not fully sure about this exact solution (need more time to check), but it definitely makes sense to make it configurable in LC4j. BTW, there are a couple of related PRs: langchain4j/langchain4j#1176 langchain4j/langchain4j#1126

The two PRs don't seem to address what I have in mind. In my case, I need to decide where to place the schema of the output object in the prompt. Today, the default behavior is to add the schema to the end of the UserMessage, and this is not always correct.

The linked PRs talk about how to customize the parser of the schema, which is something very interesting in any case.

@andreadimaio
Copy link
Contributor Author

@jmartisk, does it make sense at this point to make the changes you requested?

@jmartisk
Copy link
Collaborator

I guess some parts of this PR will still be relevant even if we do it in upstream (we will probably want to have the configuration property), but I'd perhaps suggest doing the upstream part first, and then, after/along with upgrading the langchain4j dependency later, appropriately update this PR with what will still be relevant?!

@geoand
Copy link
Collaborator

geoand commented Jun 25, 2024

I personally think this makes a lot of sense. If we can have the same in upstream LangChain4j, if not, we can live with a little drift

@andreadimaio
Copy link
Contributor Author

Not wanting to leave things half done, I made the changes @jmartisk requested. I don't know how you want to handle this PR, if you want to merge it, close it or wait for something in the langchain4j repo. From my point of view, there is nothing else to do with this PR at the moment :)

@langchain4j
Copy link
Collaborator

@andreadimaio out of curiosity, which LLM provider do you use that you need to manually format the prompt with <|begin_of_text|> and other special tokens?

@andreadimaio
Copy link
Contributor Author

out of curiosity, which LLM provider do you use that you need to manually format the prompt with <|begin_of_text|> and other special tokens?

I'm using llama-3-8b-instruct in watsonx. In this case there is no chat API to do the templating automatically. This means that putting the object schema at the end of the prompt is an error (in this particular case). And in any case, even if there was a chat API, I think it's always better to have control over where the schema is placed.

@geoand
Copy link
Collaborator

geoand commented Jun 26, 2024

I like it!

@jmartisk WDYT?

@jmartisk
Copy link
Collaborator

IMO if it's going to langchain4j itself, I'd refrain from merging this.

But maybe we will need our separate implementation anyway because our implementation of AI services is too diverged (@geoand has more knowledge of this), so I'll let you decide

@geoand
Copy link
Collaborator

geoand commented Jun 26, 2024

our implementation of AI services is too diverged

This will likely continue into the future, as there other practical alternative of incorporating build time processing

@jmartisk
Copy link
Collaborator

Ok then let's merge it

@jmartisk jmartisk merged commit 804d60e into quarkiverse:main Jun 26, 2024
12 checks passed
@andreadimaio andreadimaio deleted the response_schema_placeholder branch June 26, 2024 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants