fix: require falkordb>=1.7 so redis 8.1 installs are not broken - #298
fix: require falkordb>=1.7 so redis 8.1 installs are not broken#298galshubeli wants to merge 2 commits into
Conversation
falkordb's cluster probe forwarded async-pool connection_kwargs to the sync redis.Redis() constructor, which rejects the pool-internal keys redis 8.1 added. The first query failed with a TypeError surfaced as DatabaseUnavailableError; ping() still returned True, so health checks passed and only real work broke. Fixed upstream in falkordb 1.7.0. Moving our floor to >=1.7 is what makes that guaranteed: >=1.0 still allowed 1.6.x, which declares an unbounded redis>=7.1.0 and can resolve against the broken redis. redis stays undeclared — falkordb owns it, and 1.7 bounds it to >=7.2,<8.2. Verified end-to-end against a live FalkorDB on redis 7.2.0, 7.4.1, 8.0.1 and 8.1.0. Full suite: 1096 passed, 40 skipped. 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 (2)
📝 WalkthroughWalkthroughThe PR raises the minimum ChangesRedis 8.1 compatibility
Estimated code review effort: 1 (Trivial) | ~5 minutes Mergeability Score: ⚪ Minimal · up to The PR raises the minimum FalkorDB version to ensure compatible dependency resolution; no actionable merge-blocking risk remains after normal checks and review. Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR pins the minimum falkordb version to ensure fresh installs don’t break when redis 8.1.x is resolved transitively, aligning GraphRAG-SDK’s dependency floor with the upstream fix in falkordb 1.7.0.
Changes:
- Raise the
falkordbdependency floor from>=1.0,<2to>=1.7,<2to guarantee compatibility withredis8.1.x. - Add an Unreleased changelog entry documenting the redis 8.1 failure mode and the rationale for the new
falkordbminimum.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| graphrag_sdk/pyproject.toml | Bumps falkordb minimum to 1.7 and documents why redis isn’t directly declared. |
| CHANGELOG.md | Documents the install-time breakage and the dependency-floor fix under “Unreleased”. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Replaces #295, which GitHub locked after a force-push and won't reopen.
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
Summary by CodeRabbit
Bug Fixes
Documentation