Simplify chat memory advisor hierarchy and remove deprecated API #3121
Conversation
…d API Remove deprecated method in ChatMemory and refactor chat memory advisor classes - Refactored AbstractChatMemoryAdvisor to inherit from BaseAdvisor, leveraging its default implementations for adviseCall and adviseStream methods - Updated MessageChatMemoryAdvisor, PromptChatMemoryAdvisor, and VectorStoreChatMemoryAdvisor to directly implement their own before/after methods - Removed deprecated ChatMemory.get(String conversationId, int lastN) method in favor of using MessageWindowChatMemory - Fixed a bug in PromptChatMemoryAdvisor where it was only storing the last user message from a prompt with multiple messages - Enhanced logging in memory advisors to aid in debugging - Added comprehensive tests for advisor implementations: - Unit tests for MessageChatMemoryAdvisor and PromptChatMemoryAdvisor to check builder behavior - Integration tests for VectorStoreChatMemoryAdvisor with semantic memory retrieval Signed-off-by: Mark Pollack <[email protected]>
| * @since 1.0.0 | ||
| */ | ||
| public abstract class AbstractChatMemoryAdvisor<T> implements CallAdvisor, StreamAdvisor { | ||
| public abstract class AbstractChatMemoryAdvisor<T> implements BaseAdvisor { |
There was a problem hiding this comment.
Any chance we could remove AbstractChatMemoryAdvisor completely and have both MessageChatMemoryAdvisor and PromptChatMemoryAdvisor implement BaseAdvisor directly?
Due to all the protected constructors, methods, and builders, I think we would actually reduce the amount of code by implementing things directly in each of the advisors. We could have just the builders on each advisor class.
Furthermore, the developer experience would improve a lot and would be consistent with other advisors.
Instead of the following:
var result = chatClient.prompt()
.user(question)
.advisors(a -> a.param(AbstractChatMemoryAdvisor.CHAT_MEMORY_CONVERSATION_ID_KEY, conversationId))
.call()
.content();I could do:
var result = chatClient.prompt()
.user(question)
.advisors(a -> a.param(MessageChatMemoryAdvisor.CONVERSATION_ID, conversationId))
.call()
.content();Which is consistent with other advisors, such as for QuestionAnswerAdvisor, where params are defined on the advisor itself:
var result = this.chatClient.prompt()
.user("Please answer my question XYZ")
.advisors(a -> a.param(QuestionAnswerAdvisor.FILTER_EXPRESSION, "type == 'Spring'"))
.call()
.content();| public ChatClientRequest before(ChatClientRequest request, AdvisorChain advisorChain) { | ||
| String conversationId = doGetConversationId(request.context()); | ||
| // Add the new user messages from the current prompt to memory | ||
| List<UserMessage> newUserMessages = request.prompt().getUserMessages(); |
There was a problem hiding this comment.
As we discussed, I'm not convinced about this logic because there can be more than user messages in the input list.
If we want to handle it after RC1 and before the GA version, I would consider keeping the logic we had before here, with just the last UserMessage being included in the memory.
What do you think?
There was a problem hiding this comment.
For example, something I do a lot, is passing as input messages a list of UserMessage/AssistantMessage pairs as few shots. With the current logic, I would put all the UserMessages in memory, but not the AssistantMessages.
There was a problem hiding this comment.
I've reverted back to the original.
This commit fixes a test failure in the streaming chat memory functionality by: 1. Implementing a dedicated adviseStream method in PromptChatMemoryAdvisor that properly handles streaming responses using MessageAggregator
…r interface This commit refactors the chat memory advisor architecture to improve design and flexibility: - Remove AbstractChatMemoryAdvisor class and replace with BaseChatMemoryAdvisor interface - Move to the api package to better separate interface from implementation - Remove the abstract builder pattern entirely - Implement standalone Builder classes in each implementation: - PromptChatMemoryAdvisor - MessageChatMemoryAdvisor - VectorStoreChatMemoryAdvisor - Make constructors private in implementation classes to enforce builder usage - Simplify scheduler handling with direct configuration in builder
| /** | ||
| * The key to retrieve the chat memory conversation id from the context. | ||
| */ | ||
| String CHAT_MEMORY_CONVERSATION_ID_KEY = "chat_memory_conversation_id"; |
There was a problem hiding this comment.
Consider simplifying the naming:
| String CHAT_MEMORY_CONVERSATION_ID_KEY = "chat_memory_conversation_id"; | |
| String CONVERSATION_ID = "chat_memory_conversation_id"; |
| // 4. Add the new user message to the conversation memory. | ||
| UserMessage userMessage = processedChatClientRequest.prompt().getUserMessage(); | ||
| this.getChatMemoryStore().add(conversationId, userMessage); | ||
| // 5. Add all user messages from the current prompt to memory (after system |
There was a problem hiding this comment.
I guess we want to keep the previous behaviour here, for now:
UserMessage userMessage = processedChatClientRequest.prompt().getUserMessage();
this.chatMemory.add(conversationId, userMessage);
logger.debug("[PromptChatMemoryAdvisor.before] Added USER message to memory for conversationId={}: {}", conversationId, userMessage.getText());There was a problem hiding this comment.
ok, disabled the tests for multiple user messages
|
|
||
| private ChatMemory chatMemory; | ||
|
|
||
| protected Builder(ChatMemory chatMemory) { |
There was a problem hiding this comment.
private instead of protected?
| protected Builder(ChatMemory chatMemory) { | |
| private Builder(ChatMemory chatMemory) { |
| * @param systemTextAdvise the system text advice | ||
| * @return the builder | ||
| */ | ||
| public Builder systemTextAdvise(String systemTextAdvise) { |
There was a problem hiding this comment.
I guess we can remove this method. It was there for backward compatibility. Keeping "systemPromptTemplate" should be enough.
| * @param protectFromBlocking whether to protect from blocking | ||
| * @return the builder | ||
| */ | ||
| public Builder protectFromBlocking(boolean protectFromBlocking) { |
There was a problem hiding this comment.
I would also remove this in favour of scheduler().
There was a problem hiding this comment.
We have this in QuestionAnswerAdvisor as well so leaving for consistency unless we want to remove from there (and others). Seems like a nice helper.
| * @param context the context | ||
| * @return the conversation id | ||
| */ | ||
| protected String doGetConversationId(Map<String, Object> context) { |
There was a problem hiding this comment.
This can be removed in favour of getConversationId() provided by BaseChatMemoryAdvisor.
| } | ||
|
|
||
| private void after(ChatClientResponse chatClientResponse) { | ||
| protected String doGetConversationId(Map<String, Object> context) { |
There was a problem hiding this comment.
This can be removed in favour of getConversationId() provided by BaseChatMemoryAdvisor.
| ? context.get(ChatMemory.CHAT_MEMORY_CONVERSATION_ID_KEY).toString() : this.defaultConversationId; | ||
| } | ||
|
|
||
| private ChatClientRequest applyMessagesToRequest(ChatClientRequest request, List<Message> memoryMessages) { |
There was a problem hiding this comment.
This method is not used anymore, so it can be removed
|
|
||
| private ChatMemory chatMemory; | ||
|
|
||
| protected Builder(ChatMemory chatMemory) { |
There was a problem hiding this comment.
private instead of protected
| protected Builder(ChatMemory chatMemory) { | |
| private Builder(ChatMemory chatMemory) { |
| * @param protectFromBlocking whether to protect from blocking | ||
| * @return the builder | ||
| */ | ||
| public Builder protectFromBlocking(boolean protectFromBlocking) { |
There was a problem hiding this comment.
It can be removed in favour of scheduler()
There was a problem hiding this comment.
see previous comment, it is in other advisors outside chat memory ones.
There was a problem hiding this comment.
It's only in QuestionAnswerAdvisor for backward compatibility. My suggestion here was based on the fact we are not ensuring backward compatibility, so we could have removed it directly. But that's ok.
|
|
||
| private static final Logger logger = LoggerFactory.getLogger(VectorStoreChatMemoryAdvisor.class); | ||
|
|
||
| public static final String CHAT_MEMORY_RETRIEVE_SIZE_KEY = "chat_memory_response_size"; |
There was a problem hiding this comment.
We could consider renaming this to something more explicit about what it's used for
| public static final String CHAT_MEMORY_RETRIEVE_SIZE_KEY = "chat_memory_response_size"; | |
| public static final String TOP_K = "chat_memory_vector_store_top_k"; |
There was a problem hiding this comment.
yea, also maybe move it into the VectorStore interface so it is easier to discover. Let's consider tomorrow.
| : this.defaultChatMemoryRetrieveSize; | ||
| } | ||
|
|
||
| protected String doGetConversationId(Map<String, Object> context) { |
There was a problem hiding this comment.
It can be removed and replaced by calling getConversationId() from BaseChatMemoryAdvisor.
| } | ||
|
|
||
| private void after(ChatClientResponse chatClientResponse) { | ||
| protected int doGetChatMemoryRetrieveSize(Map<String, Object> context) { |
There was a problem hiding this comment.
this could maybe be inlined where it's used?
or else, it should be private and maybe renamed to something like "getChatMemoryTopK()" or something like that
There was a problem hiding this comment.
renamed. Seems easier to parse in the code where it is being used vs. inlined.
| * @param systemTextAdvise the system text advice | ||
| * @return this builder | ||
| */ | ||
| public Builder systemTextAdvise(String systemTextAdvise) { |
There was a problem hiding this comment.
Given the amount of changes, I would remove this since it's been superseded by systemPromptTemplate(). This was kept for backward compatibility, but things will break not matter what.
| * @param chatMemoryRetrieveSize the chat memory retrieve size | ||
| * @return this builder | ||
| */ | ||
| public Builder chatMemoryRetrieveSize(int chatMemoryRetrieveSize) { |
There was a problem hiding this comment.
maybe rename to topK() or something like that?
There was a problem hiding this comment.
sure. I dhaalso renamed the field to be public static final int DEFAULT_TOP_K = 100;
| * @param protectFromBlocking whether to protect from blocking | ||
| * @return the builder | ||
| */ | ||
| public Builder protectFromBlocking(boolean protectFromBlocking) { |
There was a problem hiding this comment.
I would remove this in favour of scheduler()
There was a problem hiding this comment.
same comment as before.
| /** | ||
| * The default chat memory retrieve size to use when no retrieve size is provided. | ||
| */ | ||
| public static final int DEFAULT_CHAT_MEMORY_RESPONSE_SIZE = 100; |
There was a problem hiding this comment.
this should probably be renamed to something like DEFAULT_TOP_K ?
Also, isn't 100 a bit too much? Should it be something lower? Like 20? (the value we use for messages)
|
merged in 848a3fd |
2 participants
Removed deprecated ChatMemory.get(String conversationId, int lastN) method in favor of using MessageWindowChatMemory
Refactored AbstractChatMemoryAdvisor to inherit from BaseAdvisor, leveraging its default implementations for adviseCall and adviseStream methods
Updated MessageChatMemoryAdvisor, PromptChatMemoryAdvisor, and VectorStoreChatMemoryAdvisor to directly implement their own before/after methods
Fixed a bug in PromptChatMemoryAdvisor where it was only storing the last user message from a prompt with multiple messages
Enhanced logging in memory advisors to aid in debugging
Added comprehensive tests for advisor implementations: