neo4j chatmemory implementation #2071
Conversation
Signed-off-by: Enrico Rampazzo <[email protected]>
Signed-off-by: Enrico Rampazzo <[email protected]>
enricorampazzo
changed the title
|
@meistermeier Could you help reviewing this PR? |
There was a problem hiding this comment.
I have no deep insights how a chat memory should work and what the expected outcome is, besides from what I can read up in code here and compared to e.g. Cassandra store. Because of this I would be happy to get also your thoughts on the overall behaviour @ilayaperumalg (and team).
Some things I am missing here:
- a dedicated integration test within the store module for the chat memory
- spring formatter (build fails)
| @@ -191,6 +193,7 @@ protected Neo4jVectorStore(Builder builder) { | |||
| this.idProperty = SchemaNames.sanitize(builder.idProperty).orElseThrow(); | |||
| this.constraintName = SchemaNames.sanitize(builder.constraintName).orElseThrow(); | |||
| this.initializeSchema = builder.initializeSchema; | |||
| this.batchingStrategy = new TokenCountBatchingStrategy(); | |||
There was a problem hiding this comment.
This does not take the batchingStrategy set in the Builder into account.
I cannot judge if this is needed at all in context of this PR.
| @@ -511,6 +516,11 @@ public Builder initializeSchema(boolean initializeSchema) { | |||
| return this; | |||
| } | |||
|
|
|||
| public Builder batchingStrategy(BatchingStrategy batchingStrategy){ | |||
There was a problem hiding this comment.
This is unnecessary because it's the same as the setter in the AbstractVectorStoreBuilder.
| Map<String, Object> queryParameters = new HashMap<>(); | ||
| queryParameters.put("conversationId", conversationId); | ||
| StringBuilder statementBuilder = new StringBuilder(); | ||
| statementBuilder.append(""" |
There was a problem hiding this comment.
Please consider using Neo4j CypherDSL here. There is already a bom declared in the pom but we only use it for proper name escaping yet.
There was no need for this before because we didn't do any string concatenation. Now I would say, that this would be a good idea to keep it maintainable.
|
|
||
| private final String mediaLabel; | ||
|
|
||
| public String getSessionLabel() { |
There was a problem hiding this comment.
Those getters are not needed.
| | `spring.ai.chat.memory.neo4j.toolCallLabel` | The label for nodes that store tool calls, for example | ||
| in Assistant Messages | `ToolCall` |
There was a problem hiding this comment.
| | `spring.ai.chat.memory.neo4j.toolCallLabel` | The label for nodes that store tool calls, for example | |
| in Assistant Messages | `ToolCall` | |
| | `spring.ai.chat.memory.neo4j.toolCallLabel` | The label for nodes that store tool calls, for example in Assistant Messages | `ToolCall` |
| @@ -374,7 +374,7 @@ Refer to the xref:_retrieval_augmented_generation[Retrieval Augmented Generation | |||
|
|
|||
| The interface `ChatMemory` represents a storage for chat conversation history. It provides methods to add messages to a conversation, retrieve messages from a conversation, and clear the conversation history. | |||
|
|
|||
| There are currently two implementations, `InMemoryChatMemory` and `CassandraChatMemory`, that provide storage for chat conversation history, in-memory and persisted with `time-to-live`, correspondingly. | |||
| There are currently three implementations, `InMemoryChatMemory`, `CassandraChatMemory` and `Neo4jChatMemory`, that provide storage for chat conversation history, in-memory, persisted with `time-to-live` in Cassandra, and persisted without `time-to-live` in Neo4j correspondingly. | |||
|
|
|||
| To create a `CassandraChatMemory` with `time-to-live`: | |||
There was a problem hiding this comment.
Maybe a more general remark, but the sections for Cassandra and Neo4j as chat memory stores(?) should look the the same regarding parameters etc. (maybe this is also true for InMemoryChatMemory and this needs to be added).
This is the same as this PR, but with a signed-off commit