Skip to content

Chat Memory Enhancements #2890

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

Closed

Conversation

ThomasVitale
Copy link
Contributor

  • ChatMemory will become a generic interface to implement different memory management strategies. It’s been moved from the “”spring-ai-client-chat” package to “spring-ai-model” package while retaining the same package, so it’s transparent to users.
  • A MessageWindowChatMemory has been introduced to provide support for a chat memory that keeps at most N messages in the memory.
  • A ChatMemoryRepository interface has been introduced to support different storage strategies for the chat memory. It’s meant to be used as part of a ChatMemory implementation. This is different than before, where the storage-specific implementation was directly tied to the ChatMemory. This design is familiar to Spring users since it’s used already in the ecosystem. The goal was to use a programming model similar to Spring Session and Spring Data.
  • The JdbcChatMemory has been supersed by JdbcChatMemoryRepository.
  • A ChatMemory bean is auto-configured for you whenever using one of the Spring AI Model starters. By default, it uses the MessageWindowChatMemory implementation and stores the conversation history in memory. If a different repository is already configured (e.g., Cassandra, JDBC, or Neo4j), Spring AI will use that instead.
  • First-class documentation has been introduced to describe the ChatMemory API and related features.
  • All the changes introduced in this PR are backward-compatible.

@ThomasVitale ThomasVitale force-pushed the chat-memory-repository branch from e95b81e to 772be8b Compare April 25, 2025 15:59
@ThomasVitale
Copy link
Contributor Author

@leijendary we are introducing a new ChatMemoryRepository API to decouple the storage mechanism (JDBC, Cassandra...) from the memory management strategy (last N messages, last N tokens...). In this pull request, as the first mover, I have extended the JDBC chat memory support to use the new APIs. The existing ones are still there, but deprecated. Since you implemented the initial JDBC chat memory support, it would be great if you could have a look at the JDBC changes in this PR and share your feedback. Thank you!

The new documentation explains how these new APIs will work.

The upgrade notes explains how to migrate to the new APIs.


void add(String conversationId, List<Message> messages);
List<Message> findMessages(String conversationId);
Copy link
Member

Choose a reason for hiding this comment

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

findByConversationId instead? just thinking of spring data repo conventions for finders.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


void clear(String conversationId);
void deleteMessages(String conversationId);
Copy link
Member

Choose a reason for hiding this comment

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

deleteByConversationId ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


List<Message> get(String conversationId, int lastN);
void saveMessages(String conversationId, List<Message> messages);
Copy link
Member

Choose a reason for hiding this comment

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

saveAll ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

case USER -> new UserMessage(content);
case ASSISTANT -> new AssistantMessage(content);
case SYSTEM -> new SystemMessage(content);
case TOOL -> null;
Copy link
Member

Choose a reason for hiding this comment

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

is returning null ok here?

Copy link
Member

Choose a reason for hiding this comment

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

why not accomodate tool messages so that if the lower level apis are currently used to store them, they can be retrieved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense! I kept the original logic from JdbcChatMemory, but it does make sense to handle tool messages nicely. I'll do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to return an empty ToolResponseMessage, because the storage saves the content of "getText()" and that method returns always an empty string for ToolResponseMessage.

We need a separate enhancement if we want to store the list of tool call results as well.

*/
public class JdbcChatMemoryRepository implements ChatMemoryRepository {

private static final String QUERY_GET_IDS = """
Copy link
Member

Choose a reason for hiding this comment

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

as with vector store schemas, in the future we need to let users customize their schema, e.g. table and row names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. For now, I kept the same logic as we have in JdbcChatMemory, but we do need the possibility to choose a custom table name and customise queries.


private static final String QUERY_GET_IDS = """
SELECT conversation_id FROM ai_chat_memory
""";
Copy link
Member

Choose a reason for hiding this comment

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

select distinct instead otherwise get duplicates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. This comment made me realise the current SQL schema definition do not define a primary key. I fixed that as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Postponed the primary key discussion for a separate task.


boolean hasNewSystemMessage = newMessages.stream()
.filter(message -> message instanceof SystemMessage)
.anyMatch(message -> !memoryMessages.contains(message));
Copy link
Member

Choose a reason for hiding this comment

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

this is a loop within a loop.

		Set<Message> memoryMessagesSet = new HashSet<>(memoryMessages);

		boolean hasNewSystemMessage = newMessages.stream()
				.filter(SystemMessage.class::isInstance) 
				.anyMatch(message -> !memoryMessagesSet.contains(message));

makes it more efficient

Copy link
Member

Choose a reason for hiding this comment

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

Is it correct that the UserMessage object equals/hashcode doesn't take into account media? we aren't storing it, so yes, but prob it needs to be updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About the UserMessage, I guess it's not correct that we are not considering the media. Agreed that it should be updated.

For SystemMessages, that works fine since it only has text. But for other message types, we should probably override the equals/hashcode inherited from the AbstractMessage.

@ThomasVitale ThomasVitale force-pushed the chat-memory-repository branch from bd11883 to 59f8df8 Compare April 25, 2025 19:46
* ChatMemory will become a generic interface to implement different memory management strategies. It’s been moved from the “”spring-ai-client-chat” package to “spring-ai-model” package while retaining the same package, so it’s transparent to users.
* A MessageWindowChatMemory has been introduced to provide support for a chat memory that keeps at most N messages in the memory.
* A ChatMemoryRepository interface has been introduced to support different storage strategies for the chat memory. It’s meant to be used as part of a ChatMemory implementation. This is different than before, where the storage-specific implementation was directly tied to the ChatMemory. This design is familiar to Spring users since it’s used already in the ecosystem. The goal was to use a programming model similar to Spring Session and Spring Data.
* The JdbcChatMemory has been supersed by JdbcChatMemoryRepository.
* A ChatMemory bean is auto-configured for you whenever using one of the Spring AI Model starters. By default, it uses the MessageWindowChatMemory implementation and stores the conversation history in memory. If a different repository is already configured (e.g., Cassandra, JDBC, or Neo4j), Spring AI will use that instead.
* First-class documentation has been introduced to describe the ChatMemory API and related features.
* All the changes introduced in this PR are backward-compatible.

Signed-off-by: Thomas Vitale <[email protected]>
@ThomasVitale
Copy link
Contributor Author

ThomasVitale commented Apr 25, 2025

Delivered as 0024e4d

@linarkou
Copy link
Contributor

linarkou commented Apr 28, 2025

@ThomasVitale thank you for the new api, really fine!
But I think fetching all rows from the database, loading it to memory and then keeping only last N seems to be not very efficient - maybe better to keep an ability to fetch only N rows in ChatMemoryRepository?

Moreover, for inserting single message we do need to 1) select all, 2) delete all and 3) insert all, is it right? Seems also not efficient..

And one more thing. I see that current solution leaves only one system message in history. But currently PromptChatMemoryAdvisor and MessageChatMemoryAdvisor save only user and assistant messages. Does there any plans to save all messages to chatMemory, not only user/assistant?

public class JdbcChatMemory implements ChatMemory {

private static final String QUERY_ADD = """
INSERT INTO ai_chat_memory (conversation_id, content, type) VALUES (?, ?, ?)""";

private static final String QUERY_GET = """
SELECT content, type FROM ai_chat_memory WHERE conversation_id = ? ORDER BY "timestamp" DESC LIMIT ?""";
Copy link
Contributor

Choose a reason for hiding this comment

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

It wasn't correct to remove descending order here - now it will always fetch only first N rows instead of last N.
I'll fix message order in #2781.

Copy link
Contributor Author

@ThomasVitale ThomasVitale Apr 28, 2025

Choose a reason for hiding this comment

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

Thanks for the feedback! The ChatMemoryRepository as designed now keeps in storage only the messages allowed by the specific ChatMemory strategy. Right now, the only built-in strategy is MessageWindowChatMemory, which keeps at most M messages. The logic for sorting out which messages to keep/remove is handled within ChatMemory before storing them, whereas ChatMemoryRepository stores exactly what the ChatMemory defines.

When you call JdbcChatMemoryRepository.findByConversationId(), you want to return all the messages because those are the ones determined as relevant to keep in memory. That's why I removed the "DESC". And that's why the lastN() method is deprecated and will be removed (besides leading to wrong results). When you call JdbcChatMemoryRepository.saveAll(), you practically overwrite the current list of messages with the new list (which has been processed already based on a given memory management strategy, such as # of messages or # of tokens).

If the message window is set to 15 messages, I will always have at most 15 messages stored in the database for a given conversationId. Does it make sense? I'm sure we can optimize the operations, especially the saveAll() one, since it's not the best. But I guess we need to introduce identifiers for each row (right now they don't have a primary key). Possibly related to #2902. If we make the schema definition customizable, it would be possible to enable different types of implementations, such as having one row per conversationId, stored as a JSON BLOB since they are handled as a single unit anyway.

Considering also your other comment, I wonder if we need some kind of specialization of the API or perhaps new APIs. ChatMemory as designed now is tailored towards standard short-term memory, keeping only the messages identified by the specific strategy.

If we want to keep the entire history and then make decisions at query time (with standard capabilities for filtering, selecting, and so on), I think we might need a different strategy. That would probably require a separate API since it wouldn't be chat memory, but it would be chat history (two different concepts). And for that we would benefit from the existing Spring Data APIs. I'll create a separate issue to talk about that since it's strictly related to our wish to surface memory as first-class citizen in ChatClient, and when doing that we need to support actual memory (short-term) but also chat history (long-term).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About what kind of messages end up in the memory, right now the built-in advisors in ChatClient never store system messages and tool response messages. But since ChatMemory is a core API that can be used outside ChatClient (for example, directly with ChatModel as shown here), we need to support all message types.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you very much for so detailed explanation, everything is now clarified!

What about DESC order - my comment was about old JdbcChatMemory , not JdbcChatMemoryRepository. So I still think it is a mistake there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, good catch! I hadn't noticed that. Thanks for reporting it! That should still be there for the old implementation. Are you fixing it in your existing PR or should I create a separate task?

Thanks so much for all your contributions!

Copy link
Contributor

Choose a reason for hiding this comment

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

Already fixed it in my PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks so much!

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.

3 participants