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.
There was a problem hiding this comment.
If I remove these getters, how can I access these values here on line 47?
There was a problem hiding this comment.
Please don't ask how I missed this. Sorry for the noise.
| | `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).
There was a problem hiding this comment.
you mean that I should implement TTL?
There was a problem hiding this comment.
No, it was just about having the same (or comparable table) for Cassandra and Neo4j. Because now Neo4j has this property table but Cassandra not. Only a documentation thing.
|
@enricorampazzo Thanks for the PR contributing chat memory implementation! @meistermeier Thanks for the review! The changes look good. Rebased the PR, refactored the auto configurations based on the latest auto-configuration updates, fixed some cosmetic issues, merged as 1d4bde8 |
Thank you @ilayaperumalg Sorry to bother you, but would it be possible to change my git username in the commit you merged to "enricorampazzo"? unfortunately it was wrong on my end, and I would like to be mentioned as a Spring AI contributor, if it is not too much work on your end, thanks :) |
|
@enricorampazzo I am so sorry that the GH username was incorrect. I wish I could help here by amending the GH commit but unfortunately, we can not force push the changes into "main" as it will impact other forks go out of sync. Thank you for your contributions. I had also ensured your name was updated as author in the files related to the PR when I originally merged the PR and I will make sure to mention your username when we publish the M7 release as the list of Spring AI contributors. |
|
@ilayaperumalg Ok, thanks :) |
This is the same as this PR, but with a signed-off commit