Skip to content

Python: fix(redis): type-check the history provider across the supported redis range - #7604

Open
Chinmay V (chinmayv095) wants to merge 1 commit into
microsoft:mainfrom
chinmayv095:fix/redis-pool-typing-bounds
Open

Python: fix(redis): type-check the history provider across the supported redis range#7604
Chinmay V (chinmayv095) wants to merge 1 commit into
microsoft:mainfrom
chinmayv095:fix/redis-pool-typing-bounds

Conversation

@chinmayv095

Copy link
Copy Markdown

Motivation & Context

The weekly dependency-range validator reported in #7340 that agent-framework-redis cannot move past its current redis bound because pyright fails at redis==8.0.1 with five reportUnnecessaryTypeIgnoreComment errors 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:

redis version original code with the ignores deleted
6.4.0 (declared lower bound) clean 6 errors
7.1.1 (currently locked) clean 6 errors
8.0.1 (the failing candidate) 5 errors clean

The cause is redis-py itself. On the older releases the asyncio client annotates lrange, rpush, llen and ltrim as returning Awaitable[T] | T, so await-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: ignore comment 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?

    • A module-level _redis_result helper accepts the Awaitable[T] | T union and returns T. It uses isawaitable, matching the existing pattern in agent_framework/_types.py. rpush, llen and ltrim now go through it and carry no ignore comment.
    • lrange is 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 explicitly Any-typed local, which makes the call site independent of how precisely redis-py annotates it, with a cast pinning the list[str] that decode_responses=True guarantees. That also removes the two union-attr ignores on the loop below it.
    • The if redis_messages: guard was dropped as dead — iterating an empty list is already a no-op.
  • What is the impact of these changes?

    • No behaviour change. _redis_result awaits exactly what the previous await awaited; the sync arm only exists to satisfy the union that redis-py declares.
    • This unblocks the pyright half of Dependency validation failed: redis (agent-framework-redis) #7340. It does not by itself let the bound move to redis>=8, and I want to flag why: the package also pins redisvl>=0.11.0,<0.16, and redisvl 0.15.0 requires redis<7.2,>=5.0. So redis 8.x is not co-installable with the current redisvl pin regardless of typing, and the validator's proposed redis<8.0.0 is already in tension with it. Raising redisvl is 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?

    • Whether the client: Any escape hatch for lrange is 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.
    • Whether _redis_result belongs in this module or in a shared location, since agent_framework_redis is not the only place that awaits redis-py commands.

Related Issue

Fixes #7340

Verification

  • Pyright, run the way the validator does (--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.
  • Unit tests: packages/redis on main 45 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 --diff and ruff check clean on both touched files.

Three tests added, all offline:

Test Covers
test_returns_messages_when_lrange_is_synchronous the non-awaitable arm end to end through get_messages, which the existing AsyncMock-based tests never exercise
TestRedisResultHelper::test_awaits_an_awaitable_result the awaitable arm
TestRedisResultHelper::test_passes_through_a_plain_result the plain-value arm

Overlap note: #7470, also mine, touches save_messages in 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

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • This PR is linked to an issue and there is no other open PR for this issue (see Related Issue above).
  • This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.

…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.
Copilot AI balanced review requested due to automatic review settings August 10, 2026 18:31
@agent-framework-automation agent-framework-automation Bot added the python Usage: [Issues, PRs], Target: Python label Aug 10, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python Usage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dependency validation failed: redis (agent-framework-redis)

2 participants