fix: pin redis to >=7.2,<8.1 to unbreak fresh installs - #295
Closed
galshubeli wants to merge 1 commit into
Closed
Conversation
The SDK imports redis.asyncio directly but never declared redis as a dependency, so pip inherited falkordb's redis>=7.1.0 range and resolved to the newest release. redis 8.1.0 injects an internal himport_registry key into ConnectionPool.connection_kwargs, and falkordb's Is_Cluster() forwards those kwargs verbatim to the synchronous redis.Redis() constructor, which does not accept it: TypeError: Redis.__init__() got an unexpected keyword argument 'himport_registry' surfaced to callers as DatabaseUnavailableError. ping() still returned True, so health checks passed and the failure only appeared on the first real query. Every fresh install from main was affected. The lower bound is a second fix: falkordb 1.6 imports redis.driver_info, absent before redis 7.2, so falkordb's declared >=7.1.0 floor was also unusable. Verified against a live FalkorDB across redis 7.2.0, 7.3.0, 7.4.1, 8.0.0, 8.0.1 (all pass) and 8.1.0 / 7.1.0 (both fail). The regression test asserts the invariant rather than a version number: the async pool kwargs must stay constructible by the sync redis.Redis() that Is_Cluster() calls, so any future release reintroducing the shape fails the suite. It requires no live server. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe SDK now filters async-only pool arguments during FalkorDB’s synchronous Redis cluster probe. The arguments are restored after probing. Tests cover TLS, non-TLS, exceptional exits, unknown keys, and pools without connection kwargs. ChangesRedis 8.1 connection compatibility
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
graphrag_sdk/tests/test_connection.py (2)
342-346: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the concrete initialization exception.
pytest.raises(Exception)accepts unrelated failures. Assert theDatabaseUnavailableErrorraised after the patchedFalkorDBfails. Match"stop after pool"to keep this helper focused on pool construction.Proposed test assertion
- with pytest.raises(Exception): + with pytest.raises(DatabaseUnavailableError, match="stop after pool"): conn._ensure_client()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@graphrag_sdk/tests/test_connection.py` around lines 342 - 346, Update the _ensure_client test to expect the concrete DatabaseUnavailableError instead of the broad Exception, and assert that its message matches "stop after pool" while retaining the existing pool-construction assertion.
348-358: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a
redis-pyversion matrix to CI.The dependency range is
redis>=7.2,<8.1, but CI tests only resolver-selected versions. Testredis-py7.2 and the latest 8.0 release in separate jobs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@graphrag_sdk/tests/test_connection.py` around lines 348 - 358, Add separate CI jobs for redis-py 7.2 and the latest 8.0 release, explicitly installing each version within the supported redis>=7.2,<8.1 range before running the relevant test suite. Keep the existing resolver-selected dependency job unchanged and ensure both matrix jobs exercise test_sync_redis_accepts_async_pool_kwargs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@graphrag_sdk/tests/test_connection.py`:
- Around line 342-346: Update the _ensure_client test to expect the concrete
DatabaseUnavailableError instead of the broad Exception, and assert that its
message matches "stop after pool" while retaining the existing pool-construction
assertion.
- Around line 348-358: Add separate CI jobs for redis-py 7.2 and the latest 8.0
release, explicitly installing each version within the supported redis>=7.2,<8.1
range before running the relevant test suite. Keep the existing
resolver-selected dependency job unchanged and ensure both matrix jobs exercise
test_sync_redis_accepts_async_pool_kwargs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ea258d8-8149-4f0f-9895-851684737f35
📒 Files selected for processing (3)
CHANGELOG.mdgraphrag_sdk/pyproject.tomlgraphrag_sdk/tests/test_connection.py
There was a problem hiding this comment.
Pull request overview
Pins the redis dependency to a known-good range to prevent fresh installs from failing on first query due to a falkordb/redis incompatibility, and adds a regression test to catch future reintroductions of incompatible pool kwarg shapes.
Changes:
- Declare
redis>=7.2,<8.1as an explicit direct dependency (with rationale for both bounds). - Add a regression test asserting that async pool kwargs (after
Is_Cluster()-style fixups) remain constructible by syncredis.Redis(). - Document the install-breakage and the dependency pin in the changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| graphrag_sdk/pyproject.toml | Adds an explicit redis version range to prevent pip resolving to the known-bad 8.1.x line and to enforce a working lower bound. |
| graphrag_sdk/tests/test_connection.py | Introduces a regression test to detect future incompatible kwargs injected into async pool configuration. |
| CHANGELOG.md | Notes the installation breakage and the dependency pin under [Unreleased]. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+350
to
+355
| import redis as sync_redis | ||
| import redis.asyncio as async_redis | ||
|
|
||
| kwargs = self._pool_kwargs(ConnectionConfig(ssl=ssl)) | ||
| # Mirror Is_Cluster()'s own fixups. | ||
| kwargs["ssl"] = kwargs.pop("connection_class", None) is async_redis.SSLConnection |
Comment on lines
+18
to
+21
| `redis.Redis()` constructor — raising | ||
| `TypeError: Redis.__init__() got an unexpected keyword argument | ||
| 'himport_registry'`, surfaced as `DatabaseUnavailableError`. `ping()` | ||
| still succeeded, so health checks passed and the failure only appeared |
galshubeli
changed the title
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| with pytest.raises(RuntimeError): | ||
| with _sync_constructible_pool_kwargs(pool): | ||
| raise RuntimeError("probe blew up") | ||
| assert pool.connection_kwargs == before |
|
|
||
| def test_restores_on_exception(self): | ||
| pool = self._build_pool(ConnectionConfig()) | ||
| before = dict(pool.connection_kwargs) |
galshubeli
force-pushed
the
fix/redis-8.1-connection-kwargs
branch
from
August 12, 2026 14:31
1e34c45 to
7f9fd05
Compare
galshubeli
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Every fresh install fails on the first query:
ping()still returnsTrue, so health checks pass and the failure only shows up on real work.Root cause
falkordb'sIs_Cluster()— reached while constructingfalkordb.asyncio.FalkorDB— copiesconnection_kwargsoff the async pool and forwards them to the synchronousredis.Redis()constructor, which has no**kwargscatch-all:redis 8.1 began injecting pool-internal keys that constructor rejects — five of them:
himport_registry,maint_notifications_pool_handler,orig_host_address,orig_socket_timeout,orig_socket_connect_timeout.Why every install hit this: falkordb used to cap redis and stopped.
falkordb1.4.0 requiredredis>=7.1.0,<8.0.0; 1.5.0 dropped the upper bound to plainredis>=7.1.0, and 1.6.x still has none — so pip walks forward into whatever redis ships next. There is no current falkordb release that excludes 8.1.0, which is why the constraint has to be expressed here.Fix
Declare
redis>=7.2,<8.1independencies. Metadata only — no source changes.<8.1excludes the broken release. This is the load-bearing half.>=7.2is a second fix:falkordb1.6 importsredis.driver_info, which does not exist before redis 7.2, so falkordb's declared>=7.1.0floor is itself unusable. Note this implicitly excludesfalkordb<1.5, which requiredredis<7.0.0.Verification
End-to-end against a live FalkorDB (connect → create → query → delete graph):
himport_registryTypeErrorModuleNotFoundError: redis.driver_infoWith the pin, pip resolves to redis 8.0.1. Full suite on that resolution: 1098 passed, 40 skipped.
Changes
pyproject.toml—redis>=7.2,<8.1, with both bounds explained in a comment.tests/test_connection.py— regression test asserting the invariant rather than a version number: the async pool kwargs must stay constructible by the syncredis.Redis()thatIs_Cluster()calls. Covers TLS and non-TLS pools, needs no live server (constructingRedisopens no socket), and fails onredis==8.1.0with the exactTypeError.CHANGELOG.md— entry under[Unreleased].Trade-off accepted
This declares
redisalongsidefalkordb, so two packages now constrain it. That is deliberate: the alternative considered was leavingredisundeclared and making the connection layer tolerate 8.1's pool kwargs in code, which would have supported redis 8.1+ but added ~58 lines tocore/connection.py. The pin was chosen for the smaller review surface.Consequence: the SDK is capped below redis 8.1 until the upstream issue is fixed.
Follow-ups (not in this PR)
Is_Cluster()should filter the kwargs it forwards to the sync constructor, and falkordb should restore an upper bound on redis — dropping it in 1.5.0 is what exposed every downstream consumer to this. Fixing it upstream is what lets this pin's ceiling be lifted.python-dotenvis a dead core dependency — zero imports anywhere in the package, anddocs/getting-started.md:60explicitly tells users to load.envthemselves.transformersis imported but declared nowhere (coref_resolvers.py:100), relying ongliner/fastcorefto supply it — the same shape as this redis bug, though guarded by atry/exceptwith a working fallback.🤖 Generated with Claude Code