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

Harrison/memory base #2122

Merged
merged 11 commits into from
Mar 29, 2023
Merged

Harrison/memory base #2122

merged 11 commits into from
Mar 29, 2023

Conversation

hwchase17
Copy link
Contributor

@3coins + @zoltan-fedor.... heres the pr + some minor changes i made. thoguhts? can try to get it into tmrws release

zoltan-fedor and others added 5 commits March 28, 2023 15:59
This is to fix #1115 

Anybody wanting to use langchain in a production application will want
to be able to:
- handle multiple chat sessions in parallel without mixing up their chat
memory
- have the ability so scale out the application, while keeping the chat
session's state (eg the chat memory) stored centrally, accessible to all
instances of the scaled out application
- potentially archive the chat memory by having them stored in a
database

This requires the library to have the ability to store the chat memory
of each chat session in a central database (like Redis or DynamoDB), so
it is retrieved from there when a chat session receives a new message
from the human and saved back in there with the response.

To support this I have created a new object called
`ChatMessageHistoryBase` to store the messages of a chat session,
`ChatMessageHistory` object got reimplemented to support existing
functionality of storing message history in the memory without the
session and two database implementations like `RedisChatMessageHistory`
and `DynamoDBChatMessageHistory` added with proper implementation with
per session storage.
Additional database or other backend (like file-system) implementations
can be added either centrally or by the users.

Credits:
- @3coins for much of the high level design
- @KBB99 for the DynamoDB implementation



To use this message store with Redis:
```
from langchain.chains.conversation.memory import ConversationBufferMemory,
from langchain.memory.chat_message_histories import RedisChatMessageHistory

message_history = RedisChatMessageHistory(
        url="redis://localhost:6379/0", ttl=10, session_id="my-test-session"
)
memory = ConversationBufferMemory(
        memory_key="baz", chat_memory=message_history, return_messages=True
)

# now you can use this memory in chains as normal

# to test without chains you can add messages directly
memory.chat_memory.add_ai_message("This is me, the AI")
memory.chat_memory.add_ai_message("This is me, the human")

# get the message history from the memory store
messages = memory.chat_memory.store.read()
```

---------

Co-authored-by: Piyush Jain <piyushjain@duck.com>
Co-authored-by: Harrison Chase <hw.chase.17@gmail.com>
Copy link
Contributor

@3coins 3coins left a comment

Choose a reason for hiding this comment

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

@hwchase17
Thanks for looking into this. 🚀

Comment on lines +11 to +13
message_history = RedisChatMessageHistory(
url="redis://localhost:6379/0", ttl=10, session_id="my-test-session"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably needs a local redis engine running to pass. Instead, should there be a mock object for unit tests?
https://github.com/cunla/fakeredis-py

Copy link
Contributor

Choose a reason for hiding this comment

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

The integration tests are using Redis directly (see), so I assumed a local instance is installed for the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah i think for integration tests we currently assume real connections are set up (not only here, but in a bunch of places)

if this was in unit tests, would be another story

@@ -54,6 +54,7 @@ aleph-alpha-client = {version="^2.15.0", optional = true}
deeplake = {version = "^3.2.9", optional = true}
pgvector = {version = "^0.1.6", optional = true}
psycopg2-binary = {version = "^2.9.5", optional = true}
boto3 = {version = "^1.26.96", optional = true}
Copy link
Contributor

@zoltan-fedor zoltan-fedor Mar 29, 2023

Choose a reason for hiding this comment

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

As you noted on the other PR, we could leave this out - as this is only required for DynamoDB and there is a case to be made that the users should install the necessary database drivers himself if he/she decides to use one.

return self.key_prefix + session_id

@property
def messages(self) -> List[BaseMessage]: # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

So my recommendation would be doing this with a Redis List data type. OR Redis JSON. But Lists are built -in and don't need any exyta modules. Instead of storing things in a json dumped blob, you can stor messages in a list.

https://redis.io/docs/data-types/lists/
https://redis-py.readthedocs.io/en/stable/commands.html#redis.commands.core.CoreCommands.lpush

Copy link
Contributor

Choose a reason for hiding this comment

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

This will also make appening to the list much easier, and done on the db side, not client side

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tylerhutcherson i took a stab at updating - lmk wht you think!

Copy link
Contributor

@zoltan-fedor zoltan-fedor Mar 29, 2023

Choose a reason for hiding this comment

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

@hwchase17, @tylerhutcherson ,
What happened to the TTL (see)?
I am fine changing to using a Redis List data type, but it seems we have also dropped the TTL option with this change.

I don't think it is a good idea not to have a TTL, as that means that these records would be kept in Redis forever with no expiration option.

Outside it probably not being a good idea to accumulate data forever in Redis, also most data privacy laws disallow that.

With lpush now you would need to set the expiration of the list separately, each time in the append operation,
see https://redis-py.readthedocs.io/en/stable/commands.html#redis.commands.cluster.RedisClusterCommands.expire

Copy link
Contributor

Choose a reason for hiding this comment

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

This is true. Though managing the size of that list could be simple, and even configurable. One mode of operation could be maintaining the list at a fixed size. That combined with overall conversation TTL should be sufficient.


def append(self, message: BaseMessage) -> None:
"""Append the message to the record in Redis"""
self.redis_client.lpush(self.key, json.dumps(_message_to_dict(message)))
Copy link
Contributor

@zoltan-fedor zoltan-fedor Mar 29, 2023

Choose a reason for hiding this comment

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

We should add back the TTL option which got removed by switching to using the Redis list object - as the lpush has no TTL option, now the TTL need to be set by a separate call, see https://redis-py.readthedocs.io/en/stable/commands.html#redis.commands.cluster.RedisClusterCommands.expire

Something like

if self.ttl:
    self.redis_client.expire(self.key, self.ttl)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zoltan-fedor good catch, added back in

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch

@hwchase17 hwchase17 merged commit b35260e into master Mar 29, 2023
@hwchase17 hwchase17 deleted the harrison/memory-base branch March 29, 2023 17:10
@property
def messages(self) -> List[BaseMessage]: # type: ignore
"""Retrieve the messages from Redis"""
_items = self.redis_client.lrange(self.key, 0, -1)
Copy link
Contributor

@zoltan-fedor zoltan-fedor Mar 29, 2023

Choose a reason for hiding this comment

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

@hwchase17, @tylerhutcherson , @3coins,
I know this just got released, but trying it out just found a bug with it.

Unfortunately the use of self.redis_client.lrange(self.key, 0, -1) returns the messages in the list in a RANDOM ORDER, not in the order how it was inserted.

I haven't yet looked into how to fix this, just wanted to let everybody know.

CORRECTION: lrange() seems to receive the messages in descending order (last added message first), so it seems we just need to sort them in the opposite order, that should be an easy fix. I will make a new PR for that.
See PR raised to fix this.

hwchase17 pushed a commit that referenced this pull request Mar 30, 2023
…rting (#2167)

The new functionality of Redis backend for chat message history
([see](#2122)) uses the Redis
list object to store messages and then uses the `lrange()` to retrieve
the list of messages
([see](https://github.com/hwchase17/langchain/blob/master/langchain/memory/chat_message_histories/redis.py#L50)).

Unfortunately this retrieves the messages as a list sorted in the
opposite order of how they were inserted - meaning the last inserted
message will be first in the retrieved list - which is not what we want.

This PR fixes that as it changes the order to match the order of
insertion.
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