refactor(index): remove connect, set_client, connection_args, redis_kwargs by vishal-bala · Pull Request #727 · redis/redis-vl-python

vishal-bala · 2026-09-04T14:15:28Z

Motivation

SearchIndex.connect() and set_client() and their AsyncSearchIndex counterparts have warned since v0.4.0, and the project has shipped 23 further minor versions without removing them. #660 reported that set_client() closes a client the caller still owns; two pull requests tried to repair the ownership logic inside it and both were closed, because patching a method nobody should be on is the wrong fix.

None of these methods had working ownership semantics to preserve. set_client() attached a finalizer based on whatever ownership the index already had, so it closed a caller's client on an index built from redis_url and never took ownership on one built with a client. It also silently abandoned the client it replaced. The method over-claimed or under-claimed depending on how the index was constructed, so this removes a broken capability rather than a working one.

This is the second of three stacked pull requests. The first added owns_client, which is what a caller migrating off set_client() needs.

Changes

The four methods, and the coercion that went with them

AsyncSearchIndex.connect() emitted three DeprecationWarnings per call: its own decorator, a redundant inline warnings.warn(), and set_client()'s decorator via delegation. All three go together.

AsyncSearchIndex._validate_client() goes too, since set_client() was its only caller. It carried the sync-to-async client coercion, so handing an async index a sync client is now rejected rather than silently converted. Both constructors gained a guard for the wrong client flavour, which also restores the TypeError that set_client() used to raise for the sync index — previously the only place that rejection existed.

# before
index = SearchIndex(schema)
index.set_client(my_client)

# after
index = SearchIndex(schema, redis_client=my_client)
# ...or, to have the index close it for you
index = SearchIndex(schema, redis_client=my_client, owns_client=True)

The guard uses isinstance rather than issubclass(type(...)), deliberately: Mock(spec=AsyncRedis) sets __class__, so isinstance catches a mis-flavoured spec'd mock while an unspecced mock still passes, which much of the test suite relies on. It is also deliberately one-sided — it rejects the wrong flavour but does not assert the object is a Redis client at all, because a positive check would reject those mocks.

`connection_args` and `redis_kwargs`

Both are removed, and both had a second entry point the issue discussion never mentioned: they appeared in the nested_connection_keys tuples passed to _split_from_existing_kwargs, so from_existing(name, connection_args={...}) was accepted with no warning at all. Removing only the decorated path would have left the deprecated spelling alive.

**kwargs would otherwise swallow both names in silence, which is worse than the warning it replaces, so a table of old spellings raises a TypeError naming the current one. The message contains parameter names only, never argument values: connection keywords carry passwords, and a message shaped as "unexpected keys: {...}" would put a live credential into an exception string and from there into logs.

With those gone every caller of _split_from_existing_kwargs passed the same tuple, so nested_connection_keys is inlined.

Minor

Four cast calls in the semantic cache were replaced by a TypedDict on BaseCache.redis_kwargs, removing eight casts in that file. They were all working around one missing annotation on a heterogeneous dict literal, and cast() asserted a type the checker could not verify.
Both class docstrings gained a client-injection example. set_client()'s docstring was the only place in the rendered API reference that explained using a client you configured yourself, and docs/api/searchindex.rst renders both classes with a bare autoclass, so deleting the method took the explanation with it.
disconnect() was documented as "Disconnect from the Redis database" on two classes and not at all on the async one, and its log line claimed the index did not own a client it had in fact created.

Release Notes

SearchIndex.connect(), SearchIndex.set_client() and their AsyncSearchIndex counterparts are removed. Pass redis_client or redis_url to the constructor instead, and add owns_client=True if the index should close a client you supplied.

The connection_args and redis_kwargs constructor keywords are removed in favour of connection_kwargs; the value is unchanged. Both now raise TypeError naming the replacement rather than being silently ignored.

AsyncSearchIndex no longer converts a sync Redis client for you. Pass an async client, or convert a non-cluster client yourself with RedisConnectionFactory.sync_to_async_redis(). Passing the wrong flavour of client to either index class now raises TypeError at construction rather than failing at first use, and handing an async client to AsyncSearchIndex's counterpart raises TypeError where it previously raised ValueError.

Notes

Removing @deprecated_argument from the two constructors surfaced four pre-existing type errors in the semantic cache. The decorator returns a bare Callable and wraps the function in an untyped wrapper, which erases the signature, so mypy had never checked a single call site of SearchIndex(...) or AsyncSearchIndex(...) anywhere in the codebase. There are roughly 45 remaining applications of that decorator, each a hole in make check-types; retyping it with ParamSpec is tracked separately, and would surface all of them at once.

RedisConnectionFactory.sync_to_async_redis() survives even though _validate_client() goes, because BaseCache._get_async_redis_client is a second, non-deprecated caller. Its coverage survives too, through the LLM cache integration tests that build a cache on a sync client and then await an async method.

from_existing now checks client flavour before the broader validate_sync_redis, which otherwise fired first with a message that says what is wrong but not what to do. The two checks cannot be merged: validate_sync_redis is an allow-list that rejects any non-client including a bare mock, while the constructor guard is a deny-list that must let mocks through.

disconnect_sync() on an async index is a no-op when a loop is already running, because it cannot await the close from inside one. That is intended — it exists for __del__ and shutdown hooks — but it was undocumented and one of the ownership tests had been asserting await disconnect() under a name promising disconnect_sync(), so the method it was named for was never exercised. Both are fixed.

Note

Medium Risk
Breaking API removal and stricter client/kwarg validation will fail migrations that still use connect, set_client, old kwarg names, or sync clients on AsyncSearchIndex; behavior is intentional but affects all index construction paths.

Overview
This PR removes deprecated index connection APIs and tightens how Redis clients and kwargs are accepted.

SearchIndex / AsyncSearchIndex: connect() and set_client() are deleted; callers must pass redis_client / redis_url (and optional owns_client) in the constructor or from_existing. AsyncSearchIndex no longer auto-converts sync clients—wrong sync/async flavour now raises TypeError at construction via _reject_wrong_client_flavour (also on from_existing, before generic validation). Docstrings add a client-injection example; async disconnect_sync() is documented as a no-op when an event loop is already running.

Connection kwargs: connection_args, redis_kwargs, and _owns_redis_client are rejected with TypeError naming the replacement (_reject_removed_kwargs), including on from_existing paths. _split_from_existing_kwargs no longer takes nested_connection_keys; it always merges connection_kwargs and rejects removed names first. SemanticRouter.from_existing uses the simplified split.

BaseCache: redis_kwargs is typed with _CacheConnectionKwargs TypedDict, removing cast() when building sync/async clients.

Tests drop coverage for removed APIs and add unit tests for rejected kwargs, credential-safe error messages, and wrong-client guards.

^{Reviewed by Cursor Bugbot for commit 9396bc8. Bugbot is set up for automated code reviews on this repo. Configure here.}

BaseCache._get_async_redis_client called get_async_redis_connection, which warns unconditionally, so anyone who built a cache from a redis_url and awaited a cache method saw a DeprecationWarning for an API they never called. A suite-wide filter in pyproject.toml hid it. Point the method at _get_aredis_connection, the async form already used everywhere else, and drop the filter. Cache clients now also report their library name via CLIENT SETINFO like every other RedisVL client, and a connection failure surfaces when the client is created rather than at the first command. The regression guard lives in tests/unit/test_connection_normalization.py and lands with the next commit, which is the first to touch that file.

An index closed only a client it created itself, and callers who needed to override that wrote to the private _owns_redis_client keyword or poked the attribute afterwards. The MCP server did the latter, which never worked as intended: _register_client_finalizer gates on the flag, so a post-construction flip lands after registration has already declined and no finalizer is ever created. owns_client states ownership once, at construction, before the finalizer is registered. It replaces the private keyword rather than sitting alongside it, so there is one spelling and no precedence question. An explicit value also wins over the ownership from_existing would otherwise assume for a client it created, which is why the assignment there uses setdefault. Also documents the accessor asymmetry between the two classes: _redis_client lazily creates on SearchIndex but is a plain nullable attribute on AsyncSearchIndex, whose lazy getter is _get_client.

The four migration modules read index.client, which is None until the client is lazily created, and each handled that differently: one raised, two recorded an error, one dereferenced unguarded, and the planner degraded to an empty key sample that then made the key-sample check vacuously true. All four build their index through from_existing, which always yields a client, so none of this was reachable in practice. Reading through _redis_client and _get_client removes the disagreement and the dead guards with it. The test doubles are renamed to match the real accessors so they still stand in for an index.

The deprecation decorators told users each deprecated argument, function and class would be removed "in the next major release". Every release so far has been 0.x and breaking changes ship on minor bumps per project convention, so that promise has never been accurate and each removal would otherwise need a release note explaining the mismatch. Say "in a future release" instead. Warning text only; no behaviour change.

Moving this method onto _get_aredis_connection introduced an await between its "is the client None" check and the assignment, because the async factory issues a CLIENT SETINFO round trip. BaseCache has no lock, so two concurrent callers each built a client and the first was left unreachable: adisconnect only closes the client currently on the instance, so the orphan's connection pool was never released. Wrap the lazy path in a double-checked lock, the same shape AsyncSearchIndex._get_client already uses over the same factory. The regression test fails without the lock and passes with it.

SemanticRouter.from_existing merges {**init_kwargs, **index_kwargs} with index_kwargs second, and set owns_client there unconditionally on the branch where it creates the client. A caller's owns_client=False was therefore discarded in silence, while SearchIndex.from_existing honoured the same argument via setdefault. Same keyword, same verb, opposite answer. Claim ownership only when the caller has not already answered, so all three public from_existing entry points agree.

Review of the owns_client work turned up several statements that were wrong or missing rather than merely terse. The owns_client entry said the index owns a client it created "from redis_url", but get_redis_connection falls back to REDIS_URL, so an index built with no connection arguments at all still creates and owns one. It also left the caller's obligation unstated: declining ownership of a client the index created means closing it yourself. disconnect was documented as "Disconnect from the Redis database" on the base and sync classes and not at all on the async one, which now misleads: it is a no-op for an unowned client, and with owns_client public that is a state callers choose. Its log line claimed the index did not own the client even when the index had created it. A test docstring asserted set_client() was already gone. It is not, until the next branch removes it, and pointing readers away from it hides the ownership footgun owns_client exists to fix. Also: coerce owns_client with bool(), since the finalizer gate tests truthiness while disconnect tested "is False", so a falsy non-bool made the two paths disagree; reject the retired private _owns_redis_client keyword loudly, because underscore-prefixed keywords are forwarded verbatim and it would otherwise be dropped in silence; hoist a lazily created client out of a per-key loop; drop the last dead client-is-None guard in the migration package; and stop one more warning promising removal in the next major release.

SearchIndex.connect, SearchIndex.set_client and their async counterparts have warned since v0.4.0, and AsyncSearchIndex.connect emitted three DeprecationWarnings per call because it delegated to set_client. None had working ownership semantics: set_client attached a finalizer based on whatever ownership the index already had, so it closed a caller's client on an index built from redis_url and never took ownership on one built with a client. Pass connection parameters to the constructor instead, with owns_client when the index should close a client you supplied. AsyncSearchIndex._validate_client goes with them, since set_client was its only caller. It carried the sync-to-async client coercion, so handing an async index a sync client is now rejected rather than silently converted. Both constructors gained a guard for the wrong client flavour, which also restores the rejection set_client used to provide for the sync index. connection_args and redis_kwargs are removed too. They had a second, undecorated entry point through from_existing that warned about nothing, so the deprecated spelling would otherwise have outlived the removal. Both now raise a TypeError naming connection_kwargs, because **kwargs would otherwise swallow them in silence; the message names keywords only, never values, since connection kwargs carry passwords. With those gone every caller of _split_from_existing_kwargs passes the same tuple, so nested_connection_keys is inlined. One side effect worth recording: @deprecated_argument wraps the function it decorates in an untyped wrapper, so mypy had never checked a single call site of either constructor. Removing it surfaced four pre-existing type errors in the semantic cache, fixed here with the cast idiom the cache base module already uses.

Six review perspectives ran against the removal. Their substantive findings, in one commit because they overlap: The removed-keyword table was unreachable on one path that mattered. AsyncSearchIndex.from_existing checked for a client before splitting kwargs, so a caller passing redis_kwargs — the async alias, and so the likeliest caller — got a generic "must provide redis_url or redis_client" instead of being told the current spelling. Split first. The wrong-flavour guard was duplicated in both constructors and unreachable from from_existing, where validate_sync_redis fired first with a message that says what is wrong but not what to do. It now lives on BaseSearchIndex, driven by two class attributes, and runs ahead of the broader validation on both paths. Its message leads with what the index needs, names the class it got — qualified, since redis.Redis and redis.asyncio.Redis share a bare __name__ — and ends with the advice, which for the async index now includes sync_to_async_redis for callers who relied on the coercion that went away with _validate_client. The guard is deliberately one-sided: it rejects the wrong flavour but does not assert the object is a client at all, because a positive check would reject the unspecced mocks much of the suite injects. That is now recorded in the docstring rather than left for a reader to infer, and the isinstance choice is pinned by a test instead of only asserted in a comment. _owns_redis_client folds into the same table, replacing two inline blocks and leaving one mechanism for "that keyword is gone" rather than two. The table drops its claim that these keywords were "removed rather than renamed", which its own values contradicted, and no longer says "no longer supported" — the callers sharing it never all accepted every name, so the router was telling users a keyword it never took had been withdrawn. Both guard tests move to the unit suite: neither touches Redis, and the sync one requested the async fixture, which pytest-asyncio 0.24 stopped allowing. Also fixes a pre-existing duplicate in the async ownership tests, where the disconnect_sync case awaited disconnect() instead and so left disconnect_sync on an owned client untested.

set_client()'s docstring was the only place in the rendered API reference that explained using a client you configured yourself, and docs/api/searchindex.rst renders both classes with a bare autoclass, so deleting the method took the explanation with it. redis_client= now appears in no example anywhere. Both class docstrings gain one, along with when the index will and will not close such a client. Neither constructor documented the three TypeErrors it can now raise, so the rename of connection_args and redis_kwargs existed only inside an exception string and a private table. Both from_existing docstrings also omitted connection_kwargs, the keyword those exceptions name as the replacement, even though they accept it through **kwargs. _split_from_existing_kwargs gains a docstring: its name promises splitting, it now also rejects, and it consumes the dict it is handed. Replaces the four casts the previous commit added, plus four that predated it, with a TypedDict on BaseCache.redis_kwargs. The casts were all working around one missing annotation on a heterogeneous dict literal, and cast() asserted a type the checker could not verify, so nothing stopped a later edit from making the assertion false. A TypedDict is a plain dict at runtime, so this is annotation-only.

The async ownership tests had two copies of the same assertion: the disconnect_sync case awaited disconnect() instead of calling disconnect_sync(), so the method it was named for was never exercised. Calling it revealed why that mattered — inside a running loop it is a silent no-op, because sync_wrapper reaches loop.run_until_complete on an already-running loop and swallows the RuntimeError. That is the intended design: the method exists for callers outside a loop, such as __del__ and shutdown hooks, and the closing path is covered there by the finalizer unit tests. But an undocumented silent no-op reads as a bug to whoever finds it next, so the test now asserts it and the docstring says it.

…internal-callers Two conflicts, both where main had touched the same lines this branch did. redisvl/extensions/cache/base.py: main removed the now-unused Mapping import while this branch added `import asyncio` for the lazy-client lock. Kept both decisions — asyncio is still used, Mapping is used nowhere. redisvl/migration/planner.py: main replaced the hand-rolled SCAN loop in _sample_keys with scan_iter, because a cluster client replies with a {node_name: cursor} mapping that cannot be fed back as a cursor. Took main's body wholesale; keeping this branch's version would have reintroduced that bug. The cast() this branch added existed only to type the loop it replaced, so it and its import are gone. The SyncRedisClient annotation on the client parameter survives and still applies. One semantic break that merged cleanly but did not work: main's new test_migration_cluster_scan.py stands an _Index double in for the validators, exposing .client, while this branch moved the validators onto _redis_client and _get_client(). The double now mirrors the real accessors, matching the two doubles this branch already updated.

…ct-set-client Brings main's changes down the stack. Two import conflicts, both where this branch and main edited the same lines. redisvl/extensions/cache/base.py: kept TypedDict, which this branch added to replace the eight casts, and took main's removal of the unused Mapping import. No cast() calls remain, so that import stays gone. redisvl/redis/connection.py: kept Mapping, which types _reject_removed_kwargs; dropped Sequence, whose only user was the nested_connection_keys parameter this branch inlined; and took main's unquote, added for the Sentinel quoted-password fix. Also drops a now-dead cast import from redisvl/migration/validation.py. Main replaced the hand-rolled SCAN loop there with scan_iter, which removed the only cast() in the file. Nothing in `make lint` would catch it, since that target runs format and mypy but not pylint.

Main replaced the hand-rolled SCAN loop in the validator with scan_iter, which removed the only cast() in the file. `make lint` runs format and mypy but not pylint, so nothing in the standard checks would flag it.

…callers' into refactor/deprecated-client/02-remove-index-connect-set-client

…lient/01-owns-client-and-internal-callers

…nswered Two review findings on #726, both in code this branch introduced. The lock that serialises lazy async client creation in BaseCache was built in __init__ and held for the cache's lifetime. An asyncio.Lock records the loop that first contends it and raises RuntimeError from any other, so reconnecting after adisconnect() failed whenever it landed on a new loop -- a second asyncio.run, a fresh pytest-asyncio loop, a notebook cell. It now rebuilds when the running loop changes, which is all a lock guarding one object's creation needs. Only the contended acquire reaches the loop check, so the regression test has to race two callers on each loop; a single-caller version passes either way. from_existing claimed a factory-built client with setdefault("owns_client", True), which an explicit owns_client=None satisfies without replacing. __init__ then inferred ownership from redis_client -- already populated by then -- read it as not-owned, and left the client it had just built for nobody to close. A caller forwarding an optional setting lands there. Both index flavours and SemanticRouter now test for None rather than for presence. AsyncSearchIndex._lock carries the same loop-binding flaw on main, untouched by this branch, and is left for its own change.

…callers' into refactor/deprecated-client/02-remove-index-connect-set-client

…lient/01-owns-client-and-internal-callers

…callers' into refactor/deprecated-client/02-remove-index-connect-set-client

limjoobin

LGTM. This PR removes the deprecated connect()/set_client() methods, since passing the client to the constructor (with owns_client for handing over ownership) is the recommended way to proceed.

…lient/02-remove-index-connect-set-client # Conflicts: # redisvl/extensions/cache/base.py # redisvl/index/index.py # tests/unit/test_connection_normalization.py # tests/unit/test_index_gc_finalizer.py

vishal-bala added 11 commits September 3, 2026 09:49

vishal-bala added auto:minor Increment the minor version when merged breakingchange breaking change to API and removed breakingchange breaking change to API labels Sep 4, 2026

vishal-bala changed the title ~~refactor(index)!: remove connect, set_client, connection_args, redis_kwargs~~ refactor(index): remove connect, set_client, connection_args, redis_kwargs Sep 4, 2026

vishal-bala added 9 commits September 10, 2026 13:04

refactor(migration): drop a cast import left dead by the merge

29c37b1

Main replaced the hand-rolled SCAN loop in the validator with scan_iter, which removed the only cast() in the file. `make lint` runs format and mypy but not pylint, so nothing in the standard checks would flag it.

Merge branch 'refactor/deprecated-client/01-owns-client-and-internal-…

3de7821

…callers' into refactor/deprecated-client/02-remove-index-connect-set-client

Merge remote-tracking branch 'origin/main' into refactor/deprecated-c…

3bc0a1b

…lient/01-owns-client-and-internal-callers

Merge branch 'refactor/deprecated-client/01-owns-client-and-internal-…

ad46213

…callers' into refactor/deprecated-client/02-remove-index-connect-set-client

Merge remote-tracking branch 'origin/main' into refactor/deprecated-c…

b620272

…lient/01-owns-client-and-internal-callers

Merge branch 'refactor/deprecated-client/01-owns-client-and-internal-…

39e0da2

…callers' into refactor/deprecated-client/02-remove-index-connect-set-client

vishal-bala marked this pull request as ready for review September 29, 2026 08:30

limjoobin approved these changes Oct 1, 2026

View reviewed changes

Base automatically changed from refactor/deprecated-client/01-owns-client-and-internal-callers to main October 1, 2026 14:19

Merge remote-tracking branch 'origin/main' into refactor/deprecated-c…

9396bc8

…lient/02-remove-index-connect-set-client # Conflicts: # redisvl/extensions/cache/base.py # redisvl/index/index.py # tests/unit/test_connection_normalization.py # tests/unit/test_index_gc_finalizer.py

vishal-bala merged commit a982819 into main Oct 1, 2026
58 checks passed

Navigation Menu

refactor(index): remove connect, set_client, connection_args, redis_kwargs - #727

refactor(index): remove connect, set_client, connection_args, redis_kwargs#727
vishal-bala merged 21 commits into
mainfrom
refactor/deprecated-client/02-remove-index-connect-set-client

vishal-bala commented Sep 4, 2026 •

edited

Loading

Uh oh!

limjoobin left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

vishal-bala commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

The four methods, and the coercion that went with them

connection_args and redis_kwargs

Minor

Release Notes

Notes

Uh oh!

limjoobin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vishal-bala commented Sep 4, 2026 •

edited

Loading

`connection_args` and `redis_kwargs`