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

Make chat memory available to the system message template #887

Merged
merged 3 commits into from
Sep 18, 2024
Merged

Conversation

geoand
Copy link
Collaborator

@geoand geoand commented Sep 11, 2024

@andreadimaio
Copy link
Collaborator

@geoand , the toString() is too much. We need to reformat the messages to the "default" format.

User: <text>
Assistant: <text>

I will change this behaviour as well.

@andreadimaio
Copy link
Collaborator

I'm looking at the implementation and I think there are some things that limit the use of the {chat_memory} placeholder (at least in my specific case). For example the {chat_memory} doesn't work if I'm using a chain where one of the AIService has no memory.

Let me try to explain with an example.
I have two AI services that are called sequentially to form a chain.

@ApplicationScoped
@RegisterAiService(chatMemoryProviderSupplier = NoChatMemoryProviderSupplier.class)
public interface RephraseService {

    @SystemMessage("""
        Given the following conversation and a follow-up question, rephrase the follow-up question to be a
        standalone question, in its original language. Return the follow-up question VERBATIM if the
        question is changing the topic with respect to the conversation. It is **VERY IMPORTANT** to only
        output the rephrased standalone question; do not add notes or comments in the output.

        Chat History:
        ------
        {chat_memory}
        ------
        """)
    public String rephrase(@MemoryId String id, @UserMessage String question);
}
@ApplicationScoped
@RegisterAiService
public interface AiService {

    @SystemMessage("You are a helpful assistant")
    public String answer(@MemoryId String id, @UserMessage String question);
}
String question = rephraseService.rephrase("chatMemoryId", message);
return aiService.answer("chatMemoryId", question);

The goal of this chain is to take the conversation history and the last question asked by the user. Based on the input, it rephrases the question and passes the result to the second prompt. The result of the first prompt must not be saved, otherwise there would be duplicates in the conversation, but the NoChatMemoryProviderSupplier.class makes the {chat_memory} placeholder always empty.

Is there a simple way to get around this behaviour? Otherwise, I don't know how useful this new placeholder can be. (Maybe there's some scenario I'm missing). At this point, I'd just leave the option of using Qute, which doesn't cause any problems. WDYT?

@geoand
Copy link
Collaborator Author

geoand commented Sep 12, 2024

Maybe I am missing something, but how would the option of using chat memory yourself with Qute differ?
Are you saying that you would manually provide the List<ChatMessage> objects and those would not the same as the ones provided by the AiService's memory?

@andreadimaio
Copy link
Collaborator

The problem is in the Ai service class that is annotated with NoChatMemoryProviderSupplier.class. In this case, the chat memory is not retrieved and the placeholder is always empty. I tried to change the code to avoid this behaviour but I get a null point exception. yesterday I did some quick tests and today if I can I'll try to review the code to better understand where the exception is coming from.

@geoand
Copy link
Collaborator Author

geoand commented Sep 12, 2024

Okay, I would like to see a sample application of what you are trying to do when you have time to add it

@andreadimaio
Copy link
Collaborator

Are you saying that you would manually provide the List<ChatMessage> objects

Yes, when I pass the ChatMessage(s) manually everything works.

and those would not the same as the ones provided by the AiService's memory?

Sorry, I don't understand. The history of the messages contains always what I'm expecting.

@geoand
Copy link
Collaborator Author

geoand commented Sep 12, 2024

Okay, I guess I need to see an example in action of what you are trying to achieve :)

@andreadimaio
Copy link
Collaborator

Okay, I guess I need to see an example in action of what you are trying to achieve :)

I can write a test and commit it to the branch with the results I expect. In this case, you can run it and see the result. Or would you prefer a new application?

@geoand
Copy link
Collaborator Author

geoand commented Sep 12, 2024

That's perfectly fine

@andreadimaio
Copy link
Collaborator

I don't have the grant to commit to this PR, but you can see the test scenario at this link

@geoand
Copy link
Collaborator Author

geoand commented Sep 16, 2024

Thanks, I'll have a look soon

@geoand
Copy link
Collaborator Author

geoand commented Sep 16, 2024

Can you accept the collaboration invitation? After that, you should be able to push to my branch

@andreadimaio
Copy link
Collaborator

To run the test: mvn clean test -Dtest=ChatMemoryPlaceholderTest -Dmaven.surefire.debug -Dsurefire.failIfNoSpecifiedTests=false in the /model-providers/watsonx folder.

@geoand
Copy link
Collaborator Author

geoand commented Sep 16, 2024

🙏🏽

@geoand
Copy link
Collaborator Author

geoand commented Sep 16, 2024

So the first thing I notice is that I didn't expect {chat_history} to be used on it's own, but to iterated over as shown here.

Also, when there is no memory associated with the AiService, for whatever reason, the variable is an empty list, which seems reaonsable, no?

@andreadimaio
Copy link
Collaborator

andreadimaio commented Sep 16, 2024

So the first thing I notice is that I didn't expect {chat_history} to be used on it's own, but to iterated over as shown here.

In my case, what I have done is to format the list of messages in a "default" format, but this is something that can be changed.

Also, when there is no memory associated with the AiService, for whatever reason, the variable is an empty list, which seems reaonsable, no?

This is exactly the scenario I'm trying to solve (and that it works if I do all steps "manually"). I have two AiServices one of them doesn't need to store the result of the messages but it should have the possibility to read them!

At this point I was wondering if it makes sense to add this new placeholder anyway or just do all the steps manually and finally use Qute (like here).

@geoand
Copy link
Collaborator Author

geoand commented Sep 16, 2024

In my case, what I have done is to format the list of messages in a "default" format, but this is something that can be changed.

I think that chat_memory should contain the actual messages, and we can provide Template Extension Methods that can gives us a default "parsed" view.

This is exactly the scenario I'm trying to solve (and that it works if I do all steps "manually"). I have two AiServices one of them doesn't need to store the result of the messages but it should have the possibility to read them!

At this point I was wondering if it makes sense to add this new placeholder anyway or just do all the steps manually and finally use Qute (like #881 (comment)).

Okay I see. In that case there is no we can no what you are using as memory, and you would have to specify the items manually.
This is all the more reason to adopt the Template Extension Methods approach I mentioned above.

@andreadimaio
Copy link
Collaborator

Ok, now I understand (Qute is something new to me). At this point the Template Extension Methods is a good way to proceed.

@geoand
Copy link
Collaborator Author

geoand commented Sep 16, 2024

👍🏽

@geoand
Copy link
Collaborator Author

geoand commented Sep 17, 2024

So would you like to update the PR or should I?

@andreadimaio
Copy link
Collaborator

I can do that. I'm thinking about what kind of methods to create in the template extension.
For now what I have in mind is the extractDialogue method. In this case the placeholder can be used like this {chat_memory.extractDialogue} and the result will be:

User: <text>
Assistant: <text>

Other methods could be created to minimize typing the qute template into the prompt. Sounds good?

@geoand
Copy link
Collaborator Author

geoand commented Sep 17, 2024

Sounds good to me!

Feel free to add whatever methods you feel make sense.

@andreadimaio
Copy link
Collaborator

I have made some changes to the code to achieve what we have in mind. If there's nothing to change, the code part can be considered closed. What is missing is the documentation. I'm thinking of creating a new section under the AI Services menu to talk about this new feature.

WDYT?

@geoand
Copy link
Collaborator Author

geoand commented Sep 17, 2024

Agreed on both counts :)

@geoand geoand marked this pull request as ready for review September 18, 2024 04:17
@geoand geoand requested a review from a team as a code owner September 18, 2024 04:17
@geoand
Copy link
Collaborator Author

geoand commented Sep 18, 2024

Thanks!

I am marking the PR as ready for review.

@cescoffier mind taking a quick look?

@@ -428,6 +430,7 @@ private static Optional<SystemMessage> prepareSystemMessage(AiServiceMethodCreat
}

templateParams.put(ResponseSchemaUtil.templateParam(), createInfo.getResponseSchemaInfo().outputFormatInstructions());
templateParams.put("chat_memory", previousChatMessages);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not related to this PR, but it would be great to have a list of all the variables we handle (like current_date, response_schema...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, we should improve the docs to include all these

@geoand geoand merged commit e136212 into main Sep 18, 2024
12 checks passed
@geoand geoand deleted the #881 branch September 18, 2024 05:51
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.

Inject ChatMemory into the Prompt
3 participants