Skip to content
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

memory/postgre #186

Closed
wants to merge 4 commits into from
Closed

memory/postgre #186

wants to merge 4 commits into from

Conversation

Struki84
Copy link
Contributor

@Struki84 Struki84 commented Jul 11, 2023

Initial commit for the postgre buffer, this is still a WIP as I'm not sure if this is the proper way to include it into the repo, since its a different kind of implementation when compared to the python repo...

The python repo has integrations for various DB's implemented in ChatMessageHistroy -> memory in python repo

AFAIK we here use the WithMemory() option when adding memory to agent, so the PostgreBuffer implements Memory interface, and has internal logic for storing and fetching chat history from the DB. Seems to me copying the python approach doesn't apply here.

Unlike in the python repo, I didn't want to write raw queries, and I used gorm, as a result the database structure can't be modified or the table name changed, when building your postgree memory.

@tmc I have a few tweaks left todo, but if this is ok I'll make the official PR. CC @FluffyKebab

PR Checklist

  • Read the Contributing documentation.
  • Read the Code of conduct documentation.
  • Name your Pull Request title clearly, concisely, and prefixed with the name of the primarily affected package you changed according to Good commit messages (such as memory: add interfaces for X, Y or util: add whizzbang helpers).
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. Fixes #123).
  • Describes the source of new concepts.
  • References existing implementations as appropriate.
  • Contains test coverage for new functions.
  • Passes all golangci-lint checks.

… sure if this is the proper way to include it into the repo, since its a very diffrent kind if implmentation when compared to the python repo...
@FluffyKebab
Copy link
Collaborator

As you have said, in the python version this is a ChatMessageHistory instead of Memory. There is currently no history interface implemented, but I see that #107 is working on this (@tmc can #107 be merged?). I think this should implemented a history interface instead of the memory interface such that this can be used in multiple forms of memory.

Should we consider some way to do this generically for multiple forms of SQL? Maybe a struct with fields for queries for inserting and getting messages and a db connection. Then there would be functions for creating this struct for multiple versions of SQL. Thoughts?

@Struki84
Copy link
Contributor Author

Struki84 commented Jul 13, 2023

@FluffyKebab I'm having a bit trouble understanding how Memory and ChatMessageHistory are implemented in the python repo, where they operate both in tandem and in parallel.. 🤷‍♂️ I guess I'll wait to ses what happens to #107

In the meantime here's an idea of a DBadapter and the persistent memory buffer that would allow to have a lot of flexibility when implementing the underlying db storage: https://gist.github.com/Struki84/6cbd1d9796ae4eaeb919a336143c6a81

go.mod Outdated
@@ -82,4 +84,6 @@ require (
google.golang.org/api v0.122.0
google.golang.org/grpc v1.55.0
google.golang.org/protobuf v1.30.0
gorm.io/driver/postgres v1.5.2
gorm.io/gorm v1.25.2
Copy link
Owner

Choose a reason for hiding this comment

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

we have alot of deps already, let's make this optional behind an interface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Take a look ;)

@FluffyKebab
Copy link
Collaborator

@Struki84 I think the chat history is how chat messages are stored, while the different forms of memory in most cases use a chat history. How the type of memory uses the chat history differs.

@mvrilo
Copy link
Contributor

mvrilo commented Jul 14, 2023

hi @FluffyKebab @Struki84, I created the #107 back there.
my understanding is the same as @FluffyKebab's

the idea behind my PR was to introduce a ChatMessageHistory interface and use it in the memory.Buffer (and other memory implementations) making it more flexible to use with any history backend.

here's the python implementation for sql as an example: https://github.com/hwchase17/langchain/blob/master/langchain/memory/chat_message_histories/sql.py

the history dependency is directly in the Buffer struct here but in the python implementation this is actually in the ChatBaseMemory class which is implemented by the buffer memories:

https://github.com/hwchase17/langchain/blob/master/langchain/memory/buffer.py#L10
https://github.com/hwchase17/langchain/blob/master/langchain/memory/chat_memory.py#L11-L12

@Struki84
Copy link
Contributor Author

Struki84 commented Jul 14, 2023

@FluffyKebab That part I understand, but I did notice, both chains and agents have the interface to accept a memory buffer or a chat message history in the py repo. Which seems to me as a weird design choice.

I was under impression that the memory class is a "manager" of sorts, where various types of ChatMessageHistory objects serve as a "Model" for memory. So I wouldn't expect to to be able to pass the "Model" directly into a chain or agent, without the memory wrapper. IMO we should keep our interface as is, and use ChatMessageHistory only wrapped in a Memory interface...

I am a bit new to Go, so might be talking a bit gibberish...in any way @mvrilo I see it the same, but since the PR was still open I did my PR to test the idea..., as soon you guys sort the merge, I can do my part and modify it accordingly ;)

@Struki84
Copy link
Contributor Author

Solved with #209. Canceling this PR due redundancy.

@Struki84 Struki84 closed this Jul 28, 2023
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.

4 participants