-
Notifications
You must be signed in to change notification settings - Fork 51
AsyncRedisCluster.from_url() raises TypeError when cluster=true is passed via URL or kwargs due to unsupported argument in redis.asyncio.RedisClient #346
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
Comments
@mprostko Thanks for the great write-up! We'll check this out. |
@claude Take a look at this. |
Claude finished @abrookins's task in 2m 57s —— View job • Analyzing AsyncRedisCluster.from_url() TypeError Issue ✅I've successfully fixed the TypeError issue where Todo List:
Analysis:The issue was in the Implementation:I've implemented the fix by:
This fix ensures that the TestingThe fix handles both scenarios mentioned in the issue:
Commit: 9be7ff7 |
…TypeError The cluster parameter needs to be stripped from both URL query string and kwargs before calling AsyncRedisCluster.from_url() because redis-py removed **kwargs support from AsyncRedisCluster.__init__(). - Add _strip_cluster_from_url_and_kwargs() helper function - Update _get_aredis_connection() to use helper function - Update get_async_redis_cluster_connection() to use helper function Fixes #346 Co-authored-by: abrookins <[email protected]>
@claude Would this also apply to |
Claude finished @abrookins's task in 3m 3s —— View job Analyzing get_redis_cluster_connection() for Similar IssueTodo List:
Analysis ResultsNo, the same fix is NOT needed for Comparison:Async version (has the issue): def get_async_redis_cluster_connection(redis_url=None, **kwargs) -> AsyncRedisCluster:
url = redis_url or get_address_from_env()
return AsyncRedisCluster.from_url(url, **kwargs) # FAILS with cluster parameter Sync version (works fine): def get_redis_cluster_connection(redis_url=None, **kwargs) -> RedisCluster:
url = redis_url or get_address_from_env()
return RedisCluster.from_url(url, **kwargs) # WORKS with cluster parameter Evidence from Original IssueThe user's example code in the original issue demonstrates that: ✅ These work:
❌ This fails:
Root CauseThe issue is specific to ConclusionOnly the async cluster functions needed the fix. The synchronous |
I think the code should also update the currently used redis-vl-python/redisvl/redis/connection.py Lines 306 to 330 in cd74f16
It should follow the change as above (replace L329-L330 with): url = url or get_address_from_env()
if is_cluster_url(url, **kwargs):
clean_url, clean_kwargs = _strip_cluster_from_url_and_kwargs(url, **kwargs)
return AsyncRedisCluster.from_url(clean_url, **clean_kwargs)
else:
return AsyncRedis.from_url(url, **kwargs) This change would allow |
@abrookins Is it possible that these changes would go into main and hopefully into 7.0 release? |
Uh oh!
There was an error while loading. Please reload this page.
Description
When using
redis-vl
withredis-py
, the AsyncRedisCluster.from_url() method raises a TypeError if thecluster=true
setting is passed either via the URL query string or as a keyword argument.**kwargs: Any
argument was removed inredis.asyncio.RedisCluster.__init__()
function in the following PR: redis/redis-py#2217Example:
Suggested Fix:
Before calling
AsyncRedisCluster.from_url()
, the cluster parameter should be stripped from both the URL and kwargs, as those are currently not supported (https://github.com/redis/redis-py/blob/master/redis/asyncio/cluster.py#L263-L310).This change should be implemented here:
redis-vl-python/redisvl/redis/connection.py
Lines 295 to 296 in cd74f16
It would also make sense to align the
redis.asyncio.RedisClient.__init__()
on theredis-py
repository too with other client implementations that accept**kwargs
and modify/clean them if needed.The text was updated successfully, but these errors were encountered: