langchain icon indicating copy to clipboard operation
langchain copied to clipboard

Bug fixes and error handling in Redis - Vectorstore

Open iamadhee opened this issue 2 years ago • 20 comments

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

iamadhee avatar May 18 '23 15:05 iamadhee

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.

tylerhutcherson avatar May 18 '23 15:05 tylerhutcherson

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

iamadhee avatar May 18 '23 15:05 iamadhee

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.

tylerhutcherson avatar May 18 '23 15:05 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?

iamadhee avatar May 18 '23 15:05 iamadhee

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

nikhilkandur avatar May 18 '23 15:05 nikhilkandur

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

iamadhee avatar May 18 '23 15:05 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.

tylerhutcherson avatar May 18 '23 15:05 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.

Thanks, have added the commit

iamadhee avatar May 18 '23 15:05 iamadhee

@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

nikhilkandur avatar May 18 '23 15:05 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.

iamadhee avatar May 18 '23 16:05 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.

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.

tylerhutcherson avatar May 18 '23 16:05 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 ?

nikhilkandur avatar May 18 '23 16:05 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 ?

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...

tylerhutcherson avatar May 18 '23 16:05 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'

nikhilkandur avatar May 18 '23 16:05 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'

yup fixing this here: https://github.com/hwchase17/langchain/pull/4936

tylerhutcherson avatar May 18 '23 16:05 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!

tylerhutcherson avatar May 18 '23 16:05 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!

Have added the new title and description

iamadhee avatar May 18 '23 17:05 iamadhee

@tylerhutcherson updated the redisearch error message to avoid further confusions

iamadhee avatar May 19 '23 06:05 iamadhee

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

iamadhee avatar May 19 '23 17:05 iamadhee

Made the suggested changes cc @tylerhutcherson @dev2049

iamadhee avatar May 19 '23 18:05 iamadhee

thanks @iamadhee!

dev2049 avatar May 19 '23 19:05 dev2049