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

Add a Redis based chat memory store extension #429

Merged
merged 2 commits into from
Apr 29, 2024

Conversation

sebastienblanc
Copy link
Contributor

Replace the default InMemoryChatMemoryStore by using Redis instead.
I'm sure if I'm doing the best thing around the Codec since there is one in quarkus-langchain4j-core but I have no idea on how to use it.

@sebastienblanc sebastienblanc requested a review from a team as a code owner March 29, 2024 13:02
@geoand geoand self-requested a review March 29, 2024 13:42
@geoand
Copy link
Collaborator

geoand commented Mar 29, 2024

Very nice! I'll review this carefully next week.

@geoand
Copy link
Collaborator

geoand commented Mar 29, 2024

I think actually that the code could be a lot simpler, but I'll have to look closely and try stuff out next week.

@geoand
Copy link
Collaborator

geoand commented Apr 1, 2024

I will push some changes to the PR shortly.

@geoand
Copy link
Collaborator

geoand commented Apr 1, 2024

I pushed an update that now makes this PR complete.

However I have some comments about naming I would like @jmartisk to comment on as well

@sebastienblanc
Copy link
Contributor Author

@geoand yeah naming is hard 😅 and thx for the updates on the PR !

@geoand
Copy link
Collaborator

geoand commented Apr 2, 2024

🙏🏼

@geoand geoand changed the title added a Redis based chat memory store extension Add a Redis based chat memory store extension Apr 2, 2024
@jmartisk
Copy link
Collaborator

jmartisk commented Apr 2, 2024

Looks nice!
I'm a little worried that now we will have two modules, quarkus-langchain4j-redis and quarkus-langchain4j-chatmemorystore-redis, it won't be very clear what the former is for.
The solution going forward would be to either merge the two extensions (but then it might get too big), or to adopt a new naming strategy, where each extension contains in its name what it actually provides, so quarkus-langchain4j-embedding-store-redis and quarkus-langchain4j-chat-memory-store-redis, for example.
We should decide on a reasonable strategy before it gets too late I think

@geoand
Copy link
Collaborator

geoand commented Apr 2, 2024

going forward would be to either merge the two extensions

I don't think we should unless we really see in the real world that these two different concerns actually almost always met with the same Datastore.

We should decide on a reasonable strategy before it gets too late I think

+1

@geoand
Copy link
Collaborator

geoand commented Apr 2, 2024

@cescoffier do you have any good suggestions on the naming of the Redis modules?

@cescoffier
Copy link
Collaborator

It depends the point of view. As developer/maintainer: quarkus-langchain4j-memory-store-redis is better because it groups all memory stores together.

From the user POV, I believe quarkus-langchain4j-redis-memory-store is easier to read, and once you select redis, the auto-completion will give you all the possibilities.

For the memory store, I would drop the 'chat', which is superfluous.

@geoand
Copy link
Collaborator

geoand commented Apr 2, 2024

Good points!

@geoand
Copy link
Collaborator

geoand commented Apr 2, 2024

I am in favor of Clement's proposals, WDYT @jmartisk @sebastienblanc ?

@sebastienblanc
Copy link
Contributor Author

I agree with @cescoffier and for sure let's not merge it with the redis datastore because those are really 2 different things (like me I use pgvector for the datastore but Redis for the memory store)

@jmartisk
Copy link
Collaborator

jmartisk commented Apr 2, 2024

So, let's sum it up:

  • For AI model providers (chat model, embedding model, scoring model, image model, moderation model), we will clump all models provided from a provider into one extension and name it, say, quarkus-langchain4j-PROVIDER
  • For embedding store providers, the extension will be named quarkus-langchain4j-embedding-store-PROVIDER
  • For memory store providers, the extension will be named quarkus-langchain4j-memory-store-PROVIDER

Is this it?

From the user POV, I believe quarkus-langchain4j-redis-memory-store is easier to read, and once you select redis, the auto-completion will give you all the possibilities.

I think I'd prefer the "developer" PoV, typing memory-store will also get you auto-completion for all memory stores - and there will be lots of them, while typing redis will only give you two possibilities - memory store and embedding store.

@sebastienblanc
Copy link
Contributor Author

sebastienblanc commented Apr 3, 2024

I think I'd prefer the "developer" PoV, typing memory-store will also get you auto-completion for all memory stores - and there will be lots of them, while typing redis will only give you two possibilities - memory store and embedding store.

💯

@geoand
Copy link
Collaborator

geoand commented Apr 3, 2024

So, let's sum it up:

  • For AI model providers (chat model, embedding model, scoring model, image model, moderation model), we will clump all models provided from a provider into one extension and name it, say, quarkus-langchain4j-PROVIDER
  • For embedding store providers, the extension will be named quarkus-langchain4j-embedding-store-PROVIDER
  • For memory store providers, the extension will be named quarkus-langchain4j-memory-store-PROVIDER

Is this it?

From the user POV, I believe quarkus-langchain4j-redis-memory-store is easier to read, and once you select redis, the auto-completion will give you all the possibilities.

I think I'd prefer the "developer" PoV, typing memory-store will also get you auto-completion for all memory stores - and there will be lots of them, while typing redis will only give you two possibilities - memory store and embedding store.

@cescoffier do you agree with this proposal?

@sebastienblanc
Copy link
Contributor Author

Should we decide on one of the naming conventions ?

@geoand
Copy link
Collaborator

geoand commented Apr 9, 2024

cc @cescoffier

@sebastienblanc
Copy link
Contributor Author

sebastienblanc commented Apr 16, 2024 via email

@geoand
Copy link
Collaborator

geoand commented Apr 17, 2024

Clement is on PTO, so let's wait until Monday before moving forward

@geoand
Copy link
Collaborator

geoand commented Apr 24, 2024

@cescoffier any preferences on the naming or should we move forward with this?

@cescoffier
Copy link
Collaborator

Yes, let's use the "Developer" approach - it's inlined with what we do in Quarkus.

@geoand
Copy link
Collaborator

geoand commented Apr 24, 2024

@sebastienblanc would you like to update the PR?

@sebastienblanc
Copy link
Contributor Author

@geoand I've been messing up with my branches 😅 but yeah let me find some time today to try to fix it and do renaming.

@geoand
Copy link
Collaborator

geoand commented Apr 29, 2024

🙏🏼

extension renaming

Co-Authored-By: Georgios Andrianakis <geoand@gmail.com>
@sebastienblanc
Copy link
Contributor Author

Renamed, Rebased and squashed commits ! Let me know if it is all good

Copy link
Collaborator

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Thanks!

@geoand geoand merged commit 30abd72 into quarkiverse:main Apr 29, 2024
12 checks passed
@adriens
Copy link

adriens commented May 9, 2024

🥳 🍾

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.

None yet

5 participants