Support redis-py 8 - #695
Conversation
vishal-bala
left a comment
vishal-bala
left a comment
There was a problem hiding this comment.
Automated review, generated with Claude Code.
Reproduced against Redis 8.4.6 (Docker) on redis-py 6.0.0 / 6.1.0 / 6.2.0 / 6.3.0 / 7.4.0 / 8.0.0 / 8.1.0, running this branch and origin/main side by side to separate regressions from pre-existing bugs.
convert_index_info_to_schema is one of three FT.INFO consumers. The other two still parse positionally and raise on dict-shaped replies. The protocol=2 default that would otherwise hide that is bypassable via the URL (see the inline comment on connection.py:612), so both are reachable.
Shared context: the reply shape is mixed, not uniformly dict
Measured on redis-py 8.1.0. Several findings below depend on this:
| client mode | index_definition |
attributes entries |
|---|---|---|
| default (8.x) | dict | flat list |
protocol=2 |
list | flat list |
protocol=3 |
dict | dict |
legacy_responses=False |
dict | dict |
Note also that _info() runs convert_bytes first, and it recurses into dict keys — so post-conversion keys are always str even though protocol=3 puts bytes on the wire. The str-keyed unit fixtures are right; worth a comment saying why, since "should these be bytes?" is the obvious wrong correction.
Findings in files this PR does not touch
A. redisvl/mcp/config.py:445 — MCP schema inspection still parses positionally (blocking)
storage_type = index_info["index_definition"][1].lower()
for raw_field in index_info.get("attributes", []):
name = raw_field[1] if storage_type == "hash" else raw_field[3]
...
"type": str(raw_field[5]).lower(),inspected_schema_from_index_info consumes the same _info() output as convert_index_info_to_schema. Every subscript above raises when that output is dict-shaped:
=== protocol=2 (factory default) : index_definition=list attrs[0]=list
MCP inspected_schema ... OK fields: ['cat', 'emb']
=== ?protocol=3 in URL : index_definition=dict attrs[0]=dict
MCP inspected_schema ... FAIL KeyError: 1
This runs during MCP server startup, so the failure is "server does not boot".
B. redisvl/cli/index.py — rvl index info table mode still parses positionally (blocking)
_display_in_table calls make_dict(index_info.get("index_definition")); make_dict indexes values[i], which raises on a dict. Same for make_dict(attrs) in the per-field loop. Reproduced end to end:
=== URL carries ?protocol=3 =====
from_existing ......................... OK
rvl index info (table) ................ FAIL KeyError: 1
--json mode survives; only the table path breaks.
Suggested fix for A and B
Extract one shape-tolerant accessor pair (index_definition(info), field_entries(info)) in redisvl/redis/connection.py, and route convert_index_info_to_schema, inspected_schema_from_index_info and _display_in_table through it. Four independent positional readers is how this recurs. Doing this also means the protocol=2 default no longer has to be load-bearing.
C. PR body — no ## Release Notes section for a breaking floor bump
Raising the floor from >=5.0 to >=6.3.0 breaks downstreams pinned to 5.x or 6.0–6.2. .autorc hoists ## Release Notes into the changelog with _From #N_ attribution; that is the only place users will see this. (Title correctly omits ! — keep it that way, conventional-commits would resolve a major bump.)
Inline comments
| # | Anchor | Severity | Finding |
|---|---|---|---|
| 1 | pyproject.toml:28 |
blocking | range admits redis-py 8.0.0, which returns zero results; 6.3.0 floor unexplained |
| 2 | connection.py:467 |
blocking | dict branch silently drops WITHSUFFIXTRIE |
| 3 | connection.py:482 |
blocking | legacy path raises IndexError, and this PR makes it the default |
| 4 | connection.py:612 |
important | protocol=2 default is bypassable via the URL |
| 5 | index.py:1802 |
important | PR body overstates what RESP3 support lands |
| 6 | storage.py:12 |
important | dead 5.x shims left behind by the floor bump |
| 7 | test_search_index.py:623 |
important | new tests do not reach the new dict branches |
| 8 | test.yml:115 |
important | 8.x cell cannot install the one broken 8.x release |
| 9 | connection.py:473 |
minor | blanket passthrough silently drops unmodelled keys |
| 10 | storage.py:617 |
minor | cast asserts rather than checks |
| 11 | test_transport.py:250 |
minor | unrelated scope, fragile hand-rolled HTTP client |
Out of scope
Vector index_missing=True fails index creation with DataError: Invalid input of type: 'bool' — identically on 6.3.0, 7.4.0 and 8.1.0. Pre-existing, unrelated to this PR, worth a separate issue.
| "numpy>=1.26.0,<3", | ||
| "pyyaml>=5.4,<7.0", | ||
| "redis>=5.0,<8.0", | ||
| "redis>=6.3.0,<9.0", |
There was a problem hiding this comment.
Blocking. Two things on this line.
The range admits redis-py 8.0.0, which returns zero results. 8.0.0 returns zero results for every query on the RESP3 wire, with no exception. 8.1.0 is unaffected. On this branch:
=== redis-py 8.0.0 / PR #695 ===
default (RESP3 wire) filter=0 vector=0 batch_search=0
protocol=2 filter=5 vector=5 batch_search=5
=== redis-py 8.1.0 / PR #695 ===
default (RESP3 wire) filter=5 vector=5 batch_search=5
protocol=2 filter=5 vector=5 batch_search=5
protocol=2 masks it for factory-built clients. Since this PR's stated purpose is support for user-supplied clients, and those default to RESP3, it reaches users as a silent wrong-answer bug.
The 6.3.0 floor has no stated reason. redis-py's VectorField rejects SVS-VAMANA through 6.2.0 (DataError: Realtime vector indexing supporting 2 Indexing Methods: 'FLAT' and 'HNSW') and accepts it from 6.3.0. RedisVL exposes that algorithm via SVSVectorFieldAttributes, so 6.3.0 is correct — but undocumented it reads as arbitrary and invites being lowered.
| "redis>=6.3.0,<9.0", | |
| # 6.3.0 is the first redis-py whose VectorField accepts SVS-VAMANA, which | |
| # RedisVL exposes. 8.0.0 silently returns zero results for every search on | |
| # RESP3 connections (redis-py #4107); 8.1 is the supported 8.x floor. | |
| "redis>=6.3.0,!=8.0.0,<9.0", |
There was a problem hiding this comment.
Resolved in 83215fd: the dependency now excludes redis-py 8.0.0, documents the 6.3.0 floor, and CI installs redis-py 8.1 or later for the 8.x job.
| } | ||
|
|
||
| if isinstance(attrs, dict): | ||
| flags = attrs.get("flags", []).copy() |
There was a problem hiding this comment.
Blocking — silent data loss.
boolean_attrs has no WITHSUFFIXTRIE entry, so it stays in flags, and line 477 excludes flags from the passthrough. TextFieldAttributes sets no extra= policy, so pydantic's default ignore swallows the loss.
A field created with withsuffixtrie=True round-trips through from_existing as withsuffixtrie: False. Schema comparison then reports two on-disk-different indices as equal.
The TODO on line 453 — 'WITHSUFFIXTRIE' is another boolean attr, but is not returned by ft.info — is stale. Redis 8.4.6 returns it: in flags under RESP3, in the tail under RESP2. Please delete it in the same commit.
One line fixes this and the IndexError in the comment on line 482:
"NOINDEX": "no_index",
+ "WITHSUFFIXTRIE": "withsuffixtrie",
}Before:
protocol=2 FAIL IndexError: Error parsing index attributes [...]
default(8.x) FAIL IndexError: Error parsing index attributes [...]
protocol=3 OK {..., 'withsuffixtrie': False, ...} <-- silent loss
After:
protocol=2 OK {..., 'withsuffixtrie': True, ...}
default(8.x) OK {..., 'withsuffixtrie': True, ...}
protocol=3 OK {..., 'withsuffixtrie': True, ...}
There was a problem hiding this comment.
Resolved in 83215fd: WITHSUFFIXTRIE is preserved from FT.INFO flags in both alternating-list and mapping response forms, with RESP2 and RESP3 coverage.
| ) | ||
| return parsed_attrs | ||
|
|
||
| original = attrs.copy() |
There was a problem hiding this comment.
Blocking. The legacy branch starting here is broken for withsuffixtrie fields, and this PR makes it the default path.
Because WITHSUFFIXTRIE is not in boolean_attrs it is never .remove()d, so it leaves an odd-length tail and the range(6, len(attrs), 2) comprehension on line 501 raises IndexError (re-raised at 504).
Pre-existing on main, so not a regression — but protocol=2 is now RedisVL's default, so the two readers disagree and the one users get by default is the broken one. Same one-line fix as the comment on line 467.
(Anchored here because line 501 itself falls between diff hunks.)
There was a problem hiding this comment.
Resolved in 83215fd: WITHSUFFIXTRIE is treated as a boolean flag before key-value parsing, and the stale TODO was removed.
| for j, query_results in enumerate(results): | ||
| _built_query = batch_built_queries[j] | ||
| parsed_result = search._parse_search( # type: ignore | ||
| parsed_result = search._parse_results( # type: ignore |
There was a problem hiding this comment.
Important — scope claim, not the code. This call is correct and _parse_results exists at the 6.3.0 floor, so no compatibility issue here.
batch_search is fixed. query and paginate are not, on this branch or on main:
##### PR #695, redis-py 8.1.0, protocol=3 client #####
query(FilterQuery) FAIL AttributeError: 'dict' object has no attribute 'docs' @ index.py:457
query(VectorQuery) FAIL AttributeError: 'dict' object has no attribute 'docs' @ index.py:457
search('@cat:{a}') OK -> dict len=5 <-- returns dict, annotated -> Result
batch_search OK -> list len=1 <-- fixed by this PR
paginate FAIL AttributeError: 'dict' object has no attribute 'docs' @ index.py:457
SQLQuery breaks in the default mode too, because sql-redis 0.7.1 parses raw FT.SEARCH replies positionally:
protocol=2 (#695 factory default) -> 5 rows
default (8.x wire RESP3) -> FAIL KeyError: 2
protocol=3 -> FAIL KeyError: 2
Scoping the fixes out is fine. Accepting a client configuration that breaks query() without saying so is not. Either narrow the PR body claim to batch_search plus index introspection, or reject protocol=3 at connection validation with an actionable error.
There was a problem hiding this comment.
Resolved in 83215fd: batch_search now parses explicit RESP3 pipeline results on redis-py 6.3 through 8.1. The PR summary is narrowed to FT.INFO and batch_search behavior.
| from redis.asyncio.cluster import ClusterPipeline as AsyncClusterPipeline | ||
| from redis.typing import EncodableT | ||
|
|
||
| # Redis 5.x compatibility (6 fixed the import path) |
There was a problem hiding this comment.
Important. Unreachable now that the floor is 6.3.0. Three sites, only this one is in the diff:
redisvl/index/storage.py:12-20(here)redisvl/index/index.py:56redisvl/redis/utils.py:27, plus stale 5.x TODOs at:269and:293
Each imports redis.__version__ solely to test startswith("5"). The floor bump in this PR is what makes them dead; leaving them implies 5.x is still supported.
There was a problem hiding this comment.
Resolved in 83215fd: removed the redis-py 5 compatibility imports and stale 5.x-specific TODOs.
| assert results[1].docs[0]["id"] == "rvl:2" | ||
|
|
||
|
|
||
| def test_default_client_from_existing_and_batch_search(redis_url, redis_test_name): |
There was a problem hiding this comment.
Important — the new tests do not reach the new code.
Redis.from_url(redis_url) gives the default mode, which per the table in the review body returns a dict index_definition but flat-list attributes. So this covers the index_definition dict branch and none of the dict branches in parse_attrs / parse_vector_attrs — most of the new code has no integration coverage. The resp3_index name and the RESP3 docstring claim coverage the test does not have.
Rename to default_client_index and parametrize:
@pytest.mark.parametrize("client_kwargs", [{}, {"protocol": 3}, {"legacy_responses": False}])That parametrization would have caught the findings on connection.py:467 and index.py:1802.
Same comment applies to test_async_search_index.py:546.
There was a problem hiding this comment.
Resolved in 83215fd: sync and async integration tests now cover default, explicit protocol=3, and redis-py 8 native response modes.
| case "$REDIS_PY_VERSION" in | ||
| "6.x") spec="redis>=6,<7" ;; | ||
| "7.x") spec="redis>=7,<8" ;; | ||
| "8.x") spec="redis>=8,<9" ;; |
There was a problem hiding this comment.
Important. redis>=8,<9 resolves to the newest 8.x, so CI never installs the one broken 8.x release — this cell cannot catch the 8.0.0 zero-results bug described in the pyproject.toml comment. Pin to redis>=8.1,<9 to make the supported floor explicit.
Separately: this row takes the matrix from 30 to 45 jobs (5 python × 3 redis-py × 3 images). If that is a problem, restricting the 8.x row to the ends of the Python range halves the added cost.
There was a problem hiding this comment.
Resolved in 83215fd: the 8.x CI install range is now redis>=8.1,<9 to avoid 8.0.0.
| parsed_attrs[python_attr] = True | ||
| if "UNF" in flags and field_type == "TEXT": | ||
| parsed_attrs["unf"] = True | ||
| parsed_attrs.update( |
There was a problem hiding this comment.
Minor. This forwards every non-excluded key, lowercased, into field attrs, and the attrs models ignore unknown keys. So anything Redis adds to FT.INFO that RedisVL does not model disappears silently — WITHSUFFIXTRIE (comment on line 467) is the instance already in the tree. A logger.debug on unmapped flags would make the next one findable.
There was a problem hiding this comment.
Resolved in 83215fd: unmapped FT.INFO flags are retained as ignored behavior with a debug log for visibility.
| client.hset(name=key, mapping=obj) | ||
| client.hset( | ||
| name=key, | ||
| mapping=cast(Mapping[EncodableT, EncodableT], obj), |
There was a problem hiding this comment.
Minor. No runtime effect; silences mypy whether or not obj is encodable. Narrow where obj is built instead.
There was a problem hiding this comment.
Resolved in 83215fd: the broad cast was replaced with explicit validation of hash values that redis-py can encode.
| _GUARD_ORIGIN_REASON = "Origin not allowed" | ||
|
|
||
|
|
||
| async def _post_mcp(port: int, headers: dict) -> httpx.Response: |
There was a problem hiding this comment.
Minor — unrelated scope. Replacing httpx with a hand-rolled socket client is unrelated to redis-py 8 and would be easier to review as its own PR. Two issues in the implementation:
int(status_line.split()[1])raisesIndexErroron an empty or malformed status line, so a dropped connection surfaces as a parse error rather than the real cause.body = await reader.read()does not handleTransfer-Encoding: chunked, so.textwould include chunk framing. The assertions compare.textagainst exact guard strings, so this fails confusingly if the ASGI server ever chunks these bodies.
httpx sets an explicit Host header directly via headers={"Host": ...} on an absolute-URL request.
There was a problem hiding this comment.
Resolved in 83215fd: reverted the complete MCP transport-test hunk to the origin/main version.
2 participants
Summary
batch_searchreplies for synchronous and asynchronous user-provided clients.Release Notes
redis-py 5.x and 6.0-6.2 are no longer supported. Upgrade to redis-py 6.3 or later. redis-py 8.0.0 is excluded because of its RESP3 empty-search-results defect.
Validation
uv sync --all-extras --frozenmake formatmake check-sort-importsmake check-typesgit diff --checkCloses #686
Note
Medium Risk
Changes touch core search parsing, index introspection, and default Redis protocol behavior across many redis-py versions; regressions could affect batch search and schema reconstruction when users supply redis-py 8 clients.
Overview
Adds redis-py 8.1+ support while tightening the supported floor to redis-py ≥6.3 (drops 5.x and blocks 8.0.0). CI now matrix-tests 6.x / 7.x / 8.x, and the lockfile moves to redis 8.1.0.
Because redis-py 8 defaults to RESP3, factory-created clients now
setdefault(protocol=2)so existing Search reply shapes stay stable unless callers opt into RESP3 or redis-py 8’s native response modes.FT.INFO handling is unified via
normalize_index_definition/normalize_index_fields, so schema introspection, MCP binding inspection, andrvl index infowork with both alternating-list (RESP2) and dictionary-shaped (RESP3) metadata—including flags likeWITHSUFFIXTRIE.Pipelined
batch_searchno longer assumes raw RESP2 lists;_parse_batch_search_resultbridges redis-py 6/7/8 RESP3 maps and legacy parsers for sync and async paths. Related cleanups drop redis 5 import shims, tighten hashhsetmappings for stricter typing, and coerce embedding cachehgetallresults throughdict(...).New unit and integration tests cover RESP3 FT.INFO conversion, default RESP2 connections, and user-supplied clients under default,
protocol=3, andlegacy_responses=False(redis-py 8).Reviewed by Cursor Bugbot for commit 83215fd. Bugbot is set up for automated code reviews on this repo. Configure here.