Python: fix(redis): type-check the history provider across the supported redis range - #7604
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes Redis history-provider type checking across supported redis-py versions without changing runtime behavior.
Changes:
- Adds a helper that normalizes awaitable and synchronous Redis results.
- Removes version-dependent type-ignore comments and adds focused tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
python/packages/redis/agent_framework_redis/_history_provider.py |
Normalizes Redis command results across annotation versions. |
python/packages/redis/tests/test_providers.py |
Tests both helper branches and synchronous lrange results. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
…ted redis range The dependency-range validator fails pyright on agent-framework-redis at redis 8.0.1 with five unnecessary-type-ignore errors. Those same ignores are required at redis 7.1.1 and 6.4.0, where redis-py annotates the asyncio commands as returning the sync/async union, so deleting them is not a fix either: no single ignore comment satisfies the whole supported range. Normalise the affected results through one helper that accepts the union, and route lrange through an explicitly Any-typed client so the call site does not depend on how precisely redis-py annotates it. Pyright is now clean at 6.4.0, 7.1.1 and 8.0.1.
ef82e2f to
cb0a891
Compare
|
Rebased onto current The conflict was only the import block: Re-ran the version matrix from the PR body against the rebased branch:
So the report in #7340 still reproduces on |
Eduard van Valkenburg (eavanvalkenburg)
left a comment
There was a problem hiding this comment.
small nit on the code (I hate unnecessary reassignments), but overall looks good.
| client: Any = self._redis_client | ||
| redis_messages = cast("list[str]", await _redis_result(client.lrange(key, 0, -1))) |
There was a problem hiding this comment.
| client: Any = self._redis_client | |
| redis_messages = cast("list[str]", await _redis_result(client.lrange(key, 0, -1))) | |
| redis_messages = cast("list[str]", await _redis_result(self._redis_client.lrange(key, 0, -1))) |
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
Motivation & Context
The weekly dependency-range validator reported in #7340 that
agent-framework-rediscannot move past its currentredisbound because pyright fails atredis==8.0.1with fivereportUnnecessaryTypeIgnoreCommenterrors in_history_provider.py.Removing those five comments does not fix it. I reproduced the validator locally and checked each version in the supported range:
The cause is redis-py itself. On the older releases the asyncio client annotates
lrange,rpush,llenandltrimas returningAwaitable[T] | T, soawait-ing them directly does not type-check ("int" is not awaitable) and the ignore is required. Newer releases narrow those annotations to the awaitable alone, which makes the very same comment unnecessary. No single# type: ignorecomment satisfies the whole supported range, which is why this needs a code change rather than a comment sweep.This matters beyond the bound bump:
reportUnnecessaryTypeIgnoreComment = "error"is enabled precisely so these comments do not accumulate, and today five of them are load-bearing on one redis version and dead weight on another.Description & Review Guide
What are the major changes?
_redis_resulthelper accepts theAwaitable[T] | Tunion and returnsT. It usesisawaitable, matching the existing pattern inagent_framework/_types.py.rpush,llenandltrimnow go through it and carry no ignore comment.lrangeis a slightly different case: it is partially unknown on the older annotations rather than merely union-typed, so even the helper leaves a strict-mode complaint about the argument expression. It now reads through an explicitlyAny-typed local, which makes the call site independent of how precisely redis-py annotates it, with acastpinning thelist[str]thatdecode_responses=Trueguarantees. That also removes the twounion-attrignores on the loop below it.if redis_messages:guard was dropped as dead — iterating an empty list is already a no-op.What is the impact of these changes?
_redis_resultawaits exactly what the previousawaitawaited; the sync arm only exists to satisfy the union that redis-py declares.redis>=8, and I want to flag why: the package also pinsredisvl>=0.11.0,<0.16, andredisvl 0.15.0requiresredis<7.2,>=5.0. So redis 8.x is not co-installable with the currentredisvlpin regardless of typing, and the validator's proposedredis<8.0.0is already in tension with it. Raisingredisvlis a separate call I have deliberately left to you — I have not touched any bound in this PR.What do you want reviewers to focus on?
client: Anyescape hatch forlrangeis acceptable, or whether you would rather keep an ignore there and accept one error on newer redis. I went this way because it is the only form I found that is clean on all three versions, but it is a real trade of local type precision and it is your call._redis_resultbelongs in this module or in a shared location, sinceagent_framework_redisis not the only place that awaits redis-py commands.Related Issue
Fixes #7340
Verification
--project pyproject.toml, strict, against each installed redis): 0 errors at 6.4.0, 7.1.1 and 8.0.1 on this branch. The table above is the before/after.packages/redisonmain45 passed / 0 failed / 0 errors, branch 48 passed / 0 failed / 0 errors. The FAILED/ERROR sets are identical (both empty) and the delta is exactly the three new tests.ruff format --diffandruff checkclean on both touched files.Three tests added, all offline:
test_returns_messages_when_lrange_is_synchronousget_messages, which the existingAsyncMock-based tests never exerciseTestRedisResultHelper::test_awaits_an_awaitable_resultTestRedisResultHelper::test_passes_through_a_plain_resultOverlap note: #7470, also mine, touches
save_messagesin this same file. The two changes are independent and either merge order works, but whichever lands second will want a trivial rebase — happy to do that whenever you tell me which you prefer first.Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.