-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add JdbcChatMemory #1528
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
Add JdbcChatMemory #1528
Conversation
...ngframework/ai/autoconfigure/chat/memory/pgvector/PgVectorChatMemoryAutoConfigurationIT.java
Outdated
Show resolved
Hide resolved
...ector-store/src/test/java/org/springframework/ai/chat/memory/PgVectorChatMemoryConfigIT.java
Outdated
Show resolved
Hide resolved
...ai-pgvector-store/src/test/java/org/springframework/ai/chat/memory/PgVectorChatMemoryIT.java
Outdated
Show resolved
Hide resolved
Is it will be usable with VectorStoreChayMemoryAdvisor? Or what is the point ? |
@ogbozoyan |
@leijendary which advisor you should use with that ChatMemory to process saving ? |
@ogbozoyan You can use @Bean
fun chatClient(builder: ChatClient.Builder, vectorStore: VectorStore, chatMemory: ChatMemory): ChatClient {
val memoryAdvisor = MessageChatMemoryAdvisor(chatMemory)
val documentRetriever = VectorStoreDocumentRetriever.builder()
.vectorStore(vectorStore)
.build()
val ragAdvisor = RetrievalAugmentationAdvisor.builder()
.documentRetriever(documentRetriever)
.build()
return builder
.defaultAdvisors(memoryAdvisor, ragAdvisor)
.build()
} |
in this example you passing VectorStore, may be you meant ? fun chatClient(builder: ChatClient.Builder, **pgVectorChatMemory: PgVectorChatMemory**, chatMemory: ChatMemory): ChatClient {
val memoryAdvisor = MessageChatMemoryAdvisor(**pgVectorChatMemory**)
val documentRetriever = VectorStoreDocumentRetriever.builder()
.vectorStore(vectorStore)
.build()
val ragAdvisor = RetrievalAugmentationAdvisor.builder()
.documentRetriever(documentRetriever)
.build()
return builder
.defaultAdvisors(memoryAdvisor, ragAdvisor)
.build()
} |
The code I provided is correct. I am using it right now. The |
I wonder if this should be more generic and instead providing a |
@eddumelendez I agree. I also originally designed it to be the same as the existing CREATE TABLE ai_chat_memory (
session_id character varying(36) NOT NULL,
content text NOT NULL,
type character varying(10) NOT NULL, -- USER/ASSISTANT
"timestamp" timestamp without time zone NOT NULL DEFAULT NOW()
); Do you also mean that the table should not be automatically created and instead be created manually by the developer? |
it should be created automatically by detecting the database. I meant having something like https://github.com/spring-projects/spring-session/blob/main/spring-session-jdbc/src/main/resources/org/springframework/session/jdbc/schema-postgresql.sql that spring ai can execute, so, if new database should be supported then adding a script should be enough |
maybe add check ? CREATE TABLE ai_chat_memory (
session_id character varying(36) NOT NULL,
content text NOT NULL,
type character varying(10) NOT NULL CHECK (type IN ('USER', 'ASSISTANT')), -- USER/ASSISTANT
"timestamp" timestamp without time zone NOT NULL DEFAULT NOW()
); |
ac555b9
to
af61678
Compare
@leijendary @ogbozoyan We are reviewing this PR currently. Could you rebase the changes against the latest |
@sobychacko I pushed the changes. |
@@ -0,0 +1 @@ | |||
DROP TABLE IF EXISTS ai_chat_memory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these SQL scripts necessary for the JDBC chat memory? I see they are used in the integration test, but it's unclear how a user would use it. The docs below don't mention them either. Could you clarify? At the very least, we need to inform the end user of the existence of these scripts. I see some of these scripts are used from the auto-configuration, but how would an end user that is not using auto configuration makes use of these scripts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the docs and specified that the chat memory table will be automatically created using autoconfiguration and how to disable it. I also removed the sql files that are not used.
); | ||
|
||
CREATE INDEX IF NOT EXISTS ai_chat_memory_conversation_id_timestamp_idx | ||
ON ai_chat_memory(conversation_id, `timestamp` DESC); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the docs and specified that the chat memory table will be automatically created using autoconfiguration and how to disable it. I also removed the sql files that are not used.
); | ||
|
||
CREATE INDEX IF NOT EXISTS ai_chat_memory_conversation_id_timestamp_idx | ||
ON ai_chat_memory(conversation_id, "timestamp" DESC); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the docs and specified that the chat memory table will be automatically created using autoconfiguration and how to disable it. I also removed the sql files that are not used.
@@ -0,0 +1 @@ | |||
DROP TABLE IF EXISTS ai_chat_memory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is now removed. See comment above.
@leijendary I see that you are making some updates to the PR. I was in the process of making your PR conformant with the new structure we introduced in the latest |
Signed-off-by: leijendary <jonathanleijendekker@gmail.com>
@sobychacko I took care of it and migrated the folder structure based on the latest structure in main. Feel free to update it if I missed anything. |
@leijendary Thank you for the PR. It is now merged upstream via 4be1002. |
An option to use a generic
JdbcChatMemory
with a starter for users that want to use SQL as chat memory.