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

Update chat memory example and separate ChatMemoryStore from ChatMemory usage #107

Conversation

andreas-eberle
Copy link
Contributor

This PR updates the ChatMemory example. The idea is to separate providing the ChatMemory creation in the ChatMemoryProviderBean and the ChatMemoryStore creation via a ChatMemoryStoreProducer.

@andreas-eberle andreas-eberle requested a review from a team as a code owner December 7, 2023 15:39
Copy link
Collaborator

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

I'm not totally sure. I don't see any cleanup mechanism, which is likely going to OOM at some point.

@Override
public ChatMemory get(Object memoryId) {
return MessageWindowChatMemory.builder()
.id(memoryId)
Copy link
Collaborator

Choose a reason for hiding this comment

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

without eviction or4 cleanup, this will lead to OOM.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I need to see exactly how this is meant to work.

Copy link
Contributor Author

@andreas-eberle andreas-eberle Dec 7, 2023

Choose a reason for hiding this comment

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

Yes, there is currently no eviction mechanism. However, for a simple example, that would be hard to do. The current idea of always evicting when the request ends makes a chat with API calls impossible.
I added a note to the documentation clarifying this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The documentation should mention or have a more complex example with eviction.
I'm still unsure about the request scope part. I think, we would need to introduce a "conversation" concept soon.

@andreas-eberle andreas-eberle force-pushed the feature/update-chat-memory-example branch from c7c4821 to 6be3de2 Compare December 7, 2023 15:52
@geoand
Copy link
Collaborator

geoand commented Dec 8, 2023

I am looking into a little more. There is a chance that things are simpler than we think

@geoand
Copy link
Collaborator

geoand commented Dec 8, 2023

I took the suggestion to use ChatMemoryProvider and incorporated it into a wider scoped change in #112.

Thanks for bringing up this idea!

@geoand geoand closed this Dec 8, 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.

3 participants