Bug fixes and error handling in Redis - Vectorstore

[iamadhee]

Bug fixes in Redis - Vectorstore (Added the version of redis to the error message and removed the cls argument from a classmethod)

Fixes #3893 #4896

Who can review?

@dev2049 @tylerhutcherson

[tylerhutcherson]

Hey -- What is this fixing exactly? We need this redis module check here otherwise it will fail for folks running vanilla redis

[tylerhutcherson]

Hey -- What is this fixing exactly? We need this redis module check here otherwise it will fail for folks running vanilla redis

The confusion might be that there's Redis modules... RediSearch and RedisJSON for example (part of Redis Stack). These are what we use the code above to check for existence...

And then there's Python client libraries. The standard redis python library is all we need for langchain.

[iamadhee]

I have reverted the previous code deletion and refined the error message @tylerhutcherson

[tylerhutcherson]

I have reverted the previous code deletion and refined the error message @tylerhutcherson

Sorry for the confusion here, but I think there's still a misunderstanding.

The raised error message as-it-was, is still the correct one. We're NOT asking the user to install a different pip python library or package at all i.e pip install redis redisearch....

We're asking them to use/run the right flavor of Redis found here. So I agree the error message can be improved. Possibly linking to this documentation link.

[iamadhee]

Sorry about all the chaos. I'll revert the changes done to this error message. I'll just probably add the version of redis (>=4.1.0) that has to be installed by the user, in order to be compatible with langchain here. Would that be fine?

[nikhilkandur]

Interestingly, I'm also getting the same error. I have rq for async workflow which also installs redis. Even with the redisearch installation, i'm still getting the same error.

$ pip show redis
Name: redis
Version: 3.5.3
Summary: Python client for Redis key-value store
Home-page: https://github.com/andymccurdy/redis-py
...
$ pip show rq
Name: rq
Version: 1.14.1
Summary: RQ is a simple, lightweight, library for creating background jobs, and processing them.
Home-page: https://github.com/nvie/rq/
...
$ pip show redisearch
Name: redisearch
Version: 2.1.1
Summary: RedisSearch Python Client
Home-page: 
Author: RedisLabs

[iamadhee]

@nikhilkandur Please do pip uninstall redisearch and do pip install redis==4.5.5
that'll solve the issue.

[tylerhutcherson]

Sorry about all the chaos. I'll revert the changes done to this error message. I'll just probably add the version of redis (>=4.1.0) that has to be installed by the user, in order to be compatible with langchain here. Would that be fine?

Perfect. I think that will help a ton.

[iamadhee]

Sorry about all the chaos. I'll revert the changes done to this error message. I'll just probably add the version of redis (>=4.1.0) that has to be installed by the user, in order to be compatible with langchain here. Would that be fine?

Perfect. I think that will help a ton.

Thanks, have added the commit

[nikhilkandur]

@nikhilkandur Please do pip uninstall redisearch and do pip install redis==4.5.5 that'll solve the issue.

I installed redis==4.5.5 but i'm getting below error but that version of redisearch doesn't exists.

ValueError: Redis failed to connect: You must add the RediSearch (>= 2.4) module from Redis Stack. Please refer to Redis Stack docs: https://redis.io/docs/stack/

Latest Redisearch 2.1.1 has a version conflict with redis 4.5.5 as it needs to be 3.5.3

[iamadhee]

@nikhilkandur Here's what:

The Redisearch standalone project is discontinued. We won't need theredisearch python package to be installed as it is already available bundled within the standard redis>=4.1.0 package.

So, the error you get right now despite what the message suggests has nothing to do with redisearch (you can refer the previous conversations as well). I doubt it might have something to do with the redis connection itself. Please check on that front.

[tylerhutcherson]

@nikhilkandur Here's what:

The Redisearch standalone project is discontinued. We won't need theredisearch python package to be installed as it is already available bundled within the standard redis>=4.1.0 package.

So, the error you get right now despite what the message suggests has nothing to do with redisearch (you can refer the previous conversations as well). I doubt it might have something to do with the redis connection itself. Please check on that front.

Exactly. The error you have @nikhilkandur is because you need to run the Redis Stack docker container (or sign up for Redis Cloud) that bundles RediSearch functionality into the database. The Python client side is different. redis>=4.1.0 is sufficient.

[nikhilkandur]

@nikhilkandur Here's what:
The Redisearch standalone project is discontinued. We won't need theredisearch python package to be installed as it is already available bundled within the standard redis>=4.1.0 package.
So, the error you get right now despite what the message suggests has nothing to do with redisearch (you can refer the previous conversations as well). I doubt it might have something to do with the redis connection itself. Please check on that front.

Exactly. The error you have @nikhilkandur is because you need to run the Redis Stack docker container (or sign up for Redis Cloud) that bundles RediSearch functionality into the database. The Python client side is different. redis>=4.1.0 is sufficient.

Currently, I'm making use of redis addon for heroku and using redis>=4.1.0 at the client side to make the vector store work. Isn't it sufficient ?

[tylerhutcherson]

@nikhilkandur Here's what:
The Redisearch standalone project is discontinued. We won't need theredisearch python package to be installed as it is already available bundled within the standard redis>=4.1.0 package.
So, the error you get right now despite what the message suggests has nothing to do with redisearch (you can refer the previous conversations as well). I doubt it might have something to do with the redis connection itself. Please check on that front.

Exactly. The error you have @nikhilkandur is because you need to run the Redis Stack docker container (or sign up for Redis Cloud) that bundles RediSearch functionality into the database. The Python client side is different. redis>=4.1.0 is sufficient.

Currently, I'm making use of redis addon for heroku and using redis>=4.1.0 at the client side to make the vector store work. Isn't it sufficient ?

Sounds like Heroku's redis implementation does not include any of the add-on modules like RediSearch, RedisJSON, RedisGraph, etc. But to use Redis as a vector database, you need the RediSearch module...

You can use this free cloud service here if you want...

[nikhilkandur]

There seems like another bug, not sure if it is fixed recently in unreleased version. I'm seeing kwargs field inside kwargs. You can see it here while instantiation

  File ".../site-packages/redis/connection.py", line 956, in __init__
    super().__init__(**kwargs)
TypeError: AbstractConnection.__init__() got an unexpected keyword argument 'kwargs'

[tylerhutcherson]

There seems like another bug, not sure if it is fixed recently in unreleased version. I'm seeing kwargs field inside kwargs. You can see it here while instantiation

  File ".../site-packages/redis/connection.py", line 956, in __init__
    super().__init__(**kwargs)
TypeError: AbstractConnection.__init__() got an unexpected keyword argument 'kwargs'

yup fixing this here: #4936

[tylerhutcherson]

@iamadhee Can you change the name of this PR to be more specific to this fix and then will get this prioritized as well. Thanks!

[iamadhee]

@iamadhee Can you change the name of this PR to be more specific to this fix and then will get this prioritized as well. Thanks!

Have added the new title and description

[iamadhee]

@tylerhutcherson updated the redisearch error message to avoid further confusions

[iamadhee]

Is there anything else that is needed from my end on this PR? @tylerhutcherson

[tylerhutcherson]

2 quick suggestions on wording. but yeah, LGTM!

cc @dev2049

[iamadhee]

Made the suggested changes cc @tylerhutcherson @dev2049

[dev2049]

thanks @iamadhee!