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

fix(_redis.py): allow all supported arguments for redis cluster #5554

Conversation

Kakadus
Copy link
Contributor

@Kakadus Kakadus commented Sep 6, 2024

Title

Support password protected redis clusters

Relevant issues

Fixes #5552

Type

🐛 Bug Fix

Changes

  • use cleanup_kwargs instead of custom kwarg filtering

[REQUIRED] Testing - Attach a screenshot of any new tests passing local

If UI changes, send a screenshot/GIF of working UI fixes

  • adapted local test to include a password for redis cluster

Copy link

vercel bot commented Sep 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
litellm ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 6, 2024 5:20am

@ishaan-jaff
Copy link
Contributor

hi @Kakadus - I tried this and it failed for me locally - I was seeing this

_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

env_overrides = {'connection_pool': BlockingConnectionPool<Connection<host=localhost,port=6379,db=0>>}, redis_kwargs = {'connection_pool': BlockingConnectionPool<Connection<host=localhost,port=6379,db=0>>}
ClusterNode = <class 'redis.cluster.ClusterNode'>, cleanup_kwargs = <function cleanup_kwargs at 0x127e78a60>, cluster_kwargs = {'connection_pool': BlockingConnectionPool<Connection<host=localhost,port=6379,db=0>>}

    def get_redis_async_client(**env_overrides):
        redis_kwargs = _get_redis_client_logic(**env_overrides)
        if "url" in redis_kwargs and redis_kwargs["url"] is not None:
            args = _get_redis_url_kwargs(client=async_redis.Redis.from_url)
            url_kwargs = {}
            for arg in redis_kwargs:
                if arg in args:
                    url_kwargs[arg] = redis_kwargs[arg]
                else:
                    litellm.print_verbose(
                        "REDIS: ignoring argument: {}. Not an allowed async_redis.Redis.from_url arg.".format(
                            arg
                        )
                    )
            return async_redis.Redis.from_url(**url_kwargs)

        if "startup_nodes" in redis_kwargs:
            from redis.cluster import ClusterNode, cleanup_kwargs

            cluster_kwargs = cleanup_kwargs(**redis_kwargs)
            print("cluster_kwargs: ", cluster_kwargs)

            new_startup_nodes: List[ClusterNode] = []

            for item in redis_kwargs["startup_nodes"]:
                new_startup_nodes.append(ClusterNode(**item))
            redis_kwargs.pop("startup_nodes")
>           return async_redis.RedisCluster(
                startup_nodes=new_startup_nodes, **cluster_kwargs
            )
E           TypeError: RedisCluster.__init__() got an unexpected keyword argument 'connection_pool'

../_redis.py:206: TypeError
------------------------------------------------------------------------------------------------------------------ Captured log call ------------------------------------------------------------------------------------------------------------------
ERROR    asyncio:base_events.py:1729 Task exception was never retrieved
future: <Task finished name='Task-2' coro=<RedisCache.ping() done, defined at /Users/ishaanjaffer/Github/litellm/litellm/caching.py:852> exception=TypeError("RedisCluster.__init__() got an unexpected keyword argument 'connection_pool'")>
Traceback (most recent call last):
  File "/Users/ishaanjaffer/Github/litellm/litellm/caching.py", line 853, in ping
    _redis_client = self.init_async_client()
  File "/Users/ishaanjaffer/Github/litellm/litellm/caching.py", line 280, in init_async_client
    return get_redis_async_client(
  File "/Users/ishaanjaffer/Github/litellm/litellm/_redis.py", line 206, in get_redis_async_client
    return async_redis.RedisCluster(
TypeError: RedisCluster.__init__() got an unexpected keyword argument 'connection_pool'
==========================================================================================

@ishaan-jaff
Copy link
Contributor

support for setting password is done here @Kakadus 9a9c0e4

@krrishdholakia krrishdholakia changed the base branch from main to litellm_minor_fixes_07_08_2024 September 7, 2024 19:48
@krrishdholakia krrishdholakia merged commit f2191ef into BerriAI:litellm_minor_fixes_07_08_2024 Sep 7, 2024
2 checks passed
krrishdholakia added a commit that referenced this pull request Sep 7, 2024
krrishdholakia added a commit that referenced this pull request Sep 7, 2024
@Kakadus
Copy link
Contributor Author

Kakadus commented Sep 8, 2024

I'm wondering why redis did not filter connection_pool... Thanks for fixing this so fast yourself!

krrishdholakia added a commit that referenced this pull request Sep 10, 2024
* fix(litellm_logging.py): set completion_start_time_float to end_time_float if none

Fixes #5500

* feat(_init_.py): add new 'openai_text_completion_compatible_providers' list

Fixes #5558

Handles correctly routing fireworks ai calls when done via text completions

* fix: fix linting errors

* fix: fix linting errors

* fix(openai.py): fix exception raised

* fix(openai.py): fix error handling

* fix(_redis.py): allow all supported arguments for redis cluster (#5554)

* Revert "fix(_redis.py): allow all supported arguments for redis cluster (#5554)" (#5583)

This reverts commit f2191ef.

* fix(router.py): return model alias w/ underlying deployment on router.get_model_list()

Fixes #5524 (comment)

* test: handle flaky tests

---------

Co-authored-by: Jonas Dittrich <58814480+Kakadus@users.noreply.github.com>
@ishaan-jaff
Copy link
Contributor

Hi @Kakadus , curious do you use LiteLLM today ? If so, I'd love to hop on a call and learn how we can improve LiteLLM for you and solve any pending issues

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.

[Bug]: cache does not support password protected Redis Clusters
3 participants