fix: require falkordb>=1.7 so redis 8.1 installs are not broken - #295
Closed
galshubeli wants to merge 1 commit into
Closed
fix: require falkordb>=1.7 so redis 8.1 installs are not broken#295galshubeli wants to merge 1 commit into
galshubeli wants to merge 1 commit into
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
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
Fresh installs failed on the first query:
ping()still returnedTrue, so health checks passed and only real work broke.Cause
falkordb's cluster probe copiedconnection_kwargsoff the async pool and forwarded them to the syncredis.Redis()constructor, which rejects the pool-internal keys redis 8.1 added. falkordb 1.5.0 had also dropped itsredis<8.0.0ceiling, so pip walked forward into 8.1.0.Fix
Fixed upstream in falkordb 1.7.0 (filters the kwargs, bounds
redis<8.2). This PR is the one-line consequence:falkordb>=1.0,<2→falkordb>=1.7,<2.The bump is what makes the upstream fix guaranteed rather than incidental —
>=1.0still allowed 1.6.x, which declares an unboundedredis>=7.1.0and can resolve against the broken redis. A fresh install happens to pick 1.7.0, but a lockfile or explicitfalkordb==1.6.xreintroduces the bug.redisis not declared here: falkordb owns it, and 1.7 bounds it to>=7.2,<8.2.Verification
Live FalkorDB, connect → create → query → delete:
Full suite on falkordb 1.7.0 + redis 8.1.0: 1096 passed, 40 skipped. No source changes, no new tests.
Follow-ups (separate)
python-dotenvis a core dep with zero imports anywhere;docs/getting-started.md:60tells users to load.envthemselves.transformersis imported (coref_resolvers.py:100) but declared nowhere, relying ongliner/fastcoref— same shape as this bug, though guarded with a working fallback.🤖 Generated with Claude Code