feat: Add Valkey Integration#1668
Conversation
Signed-off-by: Edward Liang <edward.liang@improving.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThis PR introduces comprehensive Valkey support as an optional backend across the PraisonAI persistence and storage layers, including memory configuration, state/knowledge stores, storage adapters, registry wiring, and full test coverage with optional dependency declaration. ChangesValkey persistence and storage support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes This PR introduces two new backend implementations (state and knowledge stores) and storage wrappers across multiple modules with dense logic around FT indexing, vector packing, and serialization. The changes are cohesive, follow established patterns for lazy loading and error handling, and include comprehensive mocked unit tests and integration tests. Review effort is moderate due to the mix of new data structures, glide client API usage, and test coverage spread across persistence and storage layers. Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Valkey support across the PraisonAI ecosystem, including a new ValkeyVectorKnowledgeStore, ValkeyStateStore, and a ValkeyStorageAdapter. The changes include adding the valkey-glide-sync dependency, registering the new backends in the persistence registry, and providing comprehensive unit and integration tests. Feedback highlights a potential issue with document ID extraction when IDs contain colons, a consistency requirement to strip prefixes from search results, and a suggestion to maintain the lazy-loading pattern by removing a top-level import.
| documents = [] | ||
| for doc_id_raw, fields in raw[1].items(): | ||
| doc_id_str = _decode(doc_id_raw) | ||
| bare_id = doc_id_str.split(":")[-1] |
There was a problem hiding this comment.
The current implementation of bare_id extraction using split(":")[-1] will fail if the doc_id itself contains colons (e.g., user:123). It is safer to strip the known prefix and collection name from the start of the string to ensure the full original ID is recovered.
| bare_id = doc_id_str.split(":")[-1] | |
| prefix_to_strip = f"{self.prefix}{collection}:" | |
| bare_id = doc_id_str[len(prefix_to_strip):] if doc_id_str.startswith(prefix_to_strip) else doc_id_str |
| results = [] | ||
| for doc_id, fields in raw[1].items(): | ||
| doc: Dict[str, Any] = {} | ||
| doc["id"] = doc_id.decode('utf-8') if isinstance(doc_id, bytes) else doc_id |
There was a problem hiding this comment.
The search method currently returns the full Valkey key (including the index name prefix) as the document ID. To maintain consistency with other backends and the add_document method, the prefix should be stripped from the returned ID.
doc_id_str = doc_id.decode('utf-8') if isinstance(doc_id, bytes) else doc_id
prefix_to_strip = f"{self.index_name}:"
doc["id"] = doc_id_str[len(prefix_to_strip):] if doc_id_str.startswith(prefix_to_strip) else doc_id_str| These implementations are kept in the wrapper to avoid bloating the core SDK. | ||
| """ | ||
| import os | ||
| from .valkey_adapter import ValkeyStorageAdapter |
There was a problem hiding this comment.
This top-level import of ValkeyStorageAdapter defeats the lazy-loading pattern established for other storage adapters in this module. Since ValkeyStorageAdapter is already handled in __getattr__, this import should be removed from the top level and moved inside _make_valkey_backend_class where it is needed as a base class.
Greptile SummaryThis PR adds Valkey as a first-class storage backend across state storage, vector knowledge, and session CRUD, using
Confidence Score: 3/5The new Valkey wiring is structurally sound but several concrete defects need fixing before the feature is production-ready. The eager import in storage/init.py breaks lazy loading for the entire praisonai.storage module the moment it lands. The incomplete ValkeySearch filter escaping means any filter value containing characters like ':', '(', or '@' will raise a runtime error. Both unit test fixture helpers are missing required instance attributes, so the majority of the added test cases will fail with AttributeError rather than exercise the code they claim to cover. src/praisonai/praisonai/storage/init.py, src/praisonai/praisonai/persistence/knowledge/valkey_vector.py, src/praisonai/praisonai/persistence/tests/test_valkey_backends.py, src/praisonai/tests/unit/storage/test_valkey_backend.py Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| These implementations are kept in the wrapper to avoid bloating the core SDK. | ||
| """ | ||
| import os | ||
| from .valkey_adapter import ValkeyStorageAdapter |
There was a problem hiding this comment.
from .valkey_adapter import ValkeyStorageAdapter at module level loads valkey_adapter.py — and therefore tries to import glide_sync — the moment any code imports from praisonai.storage, even for Redis or Postgres. The __getattr__ branch for "ValkeyStorageAdapter" added a few lines later is dead code and will never be reached, because the name is already present in the module namespace. Every other adapter (RedisStorageAdapter, MongoDBStorageAdapter, etc.) is intentionally lazy; the direct import here breaks that contract for all callers of this module.
Remove the top-level import and let the existing __getattr__ branch handle ValkeyStorageAdapter on demand (the same pattern used for the other adapters).
| def _make_knowledge_store(): | ||
| """Return a ValkeyVectorKnowledgeStore with a pre-injected MagicMock client.""" | ||
| from praisonai.persistence.knowledge.valkey_vector import ValkeyVectorKnowledgeStore | ||
|
|
||
| store = ValkeyVectorKnowledgeStore.__new__(ValkeyVectorKnowledgeStore) | ||
| mock_client = MagicMock() | ||
| store._client = mock_client | ||
| store._get_client = lambda: mock_client | ||
|
|
||
| return store, mock_client | ||
|
|
||
|
|
||
| def _make_state_store(): | ||
| """Return a ValkeyStateStore with a pre-injected MagicMock client.""" | ||
| from praisonai.persistence.state.valkey import ValkeyStateStore | ||
|
|
||
| store = ValkeyStateStore.__new__(ValkeyStateStore) | ||
| mock_client = MagicMock() | ||
| store._client = mock_client | ||
| store._get_client = lambda: mock_client | ||
|
|
||
| return store, mock_client |
There was a problem hiding this comment.
__init__, leaving required instance attributes unset
Both _make_knowledge_store() and _make_state_store() call __new__ directly and then only inject _client / _get_client. Instance attributes set in __init__ — notably self.prefix, self.host, self.port, and self.password — are never assigned. Every test that exercises a method touching those attributes (e.g. test_insert, test_search, test_get, test_get_existing, test_keys, etc.) will raise AttributeError: '...' object has no attribute 'prefix' before even reaching the mock assertion. The tests in src/praisonai/tests/unit/storage/test_valkey_backend.py share the same pattern and have the same gap.
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review - Valkey IntegrationPhase 1: Architecture Review per AGENTS.md
Phase 2: Critical Issues Analysis
Phase 3: Final Verdict
Architecture Assessment✅ Protocol-driven compliance: Excellent - follows existing Redis/MongoDB patterns with proper lazy loading in the agents package ✅ Backward compatibility: Perfect - purely additive changes, no breaking modifications Critical Issues Found1. 🚨 BLOCKING: Eager Import Breaks Lazy LoadingFile: # ❌ WRONG: This breaks lazy loading for ALL users
from .valkey_adapter import ValkeyStorageAdapter # Top-level import
# The corresponding __getattr__ branch is now dead codeImpact: Any import of 2. 🚨 BLOCKING: Incomplete ValkeySearch Filter EscapingFile: # ❌ WRONG: Only escapes - and . but ValkeySearch requires escaping many more
escaped = str(value).replace("-", "\\-").replace(".", "\\.")Impact: Any filter value with 3. 🚨 BLOCKING: Document ID CorruptionFile: # ❌ WRONG: Truncates IDs containing colons
bare_id = doc_id_str.split(":")[-1] # "user:123" becomes "123"Impact: Document IDs like 4. 🚨 BLOCKING: Test Fixtures BrokenFiles: Multiple test files use # ❌ WRONG: Missing required instance attributes
def _make_knowledge_store():
store = ValkeyVectorKnowledgeStore.__new__(ValkeyVectorKnowledgeStore)
store._client = mock_client
return store # Missing: prefix, host, port, passwordImpact: Most tests will fail with 5. 🚨 BLOCKING: Score Threshold Only Works for CosineFile: # ❌ WRONG: Only valid for cosine distance
similarity = 1 - score # Breaks for L2 and IP metricsImpact: Incorrect filtering when using Minor Issues6. Inconsistent ID Prefix HandlingFile: Recommendation❌ DO NOT MERGE - Multiple Blocking IssuesWhile the architectural approach is sound and follows AGENTS.md principles well, there are 5 critical bugs that make this PR unsuitable for production:
Required Actions:
Architecture Quality: ⭐⭐⭐⭐⭐The overall design is excellent - proper lazy loading, protocol-driven approach, follows existing patterns, and maintains backward compatibility. Once the implementation bugs are fixed, this would be a high-quality addition to PraisonAI. Job Run: View workflow |
2 participants
Summary
Adds Valkey as a supported backend for state storage, vector knowledge, and session CRUD. Uses
valkey-glide-syncthroughout. Vector search goes through ValkeySearch FT with HNSW indexing.Issue
N/A
Changes
src/praisonai-agents/praisonaiagents/config/feature_configs.py— addedVALKEYto theMemoryBackendenumsrc/praisonai-agents/praisonaiagents/storage/backends.py— registeredValkeyBackendandValkeySearchBackendin the lazy-loader; addedvalkeybranch inget_backend()src/praisonai/praisonai/persistence/state/valkey.py(new) —ValkeyStateStore: get/set/delete/exists/keys/ttl/expire/hash opssrc/praisonai/praisonai/persistence/knowledge/valkey_vector.py(new) —ValkeyVectorKnowledgeStore: HNSW index via ValkeySearch FT, with pre-filter and score threshold support on searchsrc/praisonai/praisonai/storage/valkey_adapter.py(new) —ValkeyStorageAdapterfor session CRUD;ValkeySearchBackendfor basic vector index/searchsrc/praisonai/praisonai/storage/__init__.py— exports the new adapters;_make_valkey_backend_class()builds aValkeyBackendsubclass that pulls connection params fromVALKEY_HOST/VALKEY_PORT/VALKEY_PASSWORDsrc/praisonai/praisonai/persistence/registry.py— registersvalkeyin bothKNOWLEDGE_STORESandSTATE_STORESsrc/praisonai/pyproject.toml— addedvalkeyextras group (valkey-glide-sync>=2.3.1)src/praisonai/praisonai/persistence/tests/test_valkey_backends.py(new) — unit tests forValkeyVectorKnowledgeStore,ValkeyStateStore, and registry entries (all mocked)src/praisonai/tests/unit/storage/test_valkey_backend.py(new) — unit tests forValkeyStorageAdapterandValkeySearchBackend(mocked)src/praisonai-agents/tests/managed/test_managed_persistence_all_dbs.py— addedTestValkeyStateStorefor live server tests; auto-skips if nothing is running onlocalhost:6379src/praisonai-agents/tests/unit/config/test_precedence_ladder.py— added test thatresolve_memory("valkey")returnsMemoryConfigwithMemoryBackend.VALKEYTests
Unit tests mock
glide_syncdirectly, so they pass in CI without a live server. The managedTestValkeyStateStoretests hit a real instance and skip when one isn't available.Additional context
The
glide_syncimport is try/except guarded at module level, so nothing breaks if the package isn't installed. Install the extras withpip install 'praisonai[valkey]'.Summary by CodeRabbit
Release Notes
New Features
Chores
valkeyextra withvalkey-glide-sync>=2.3.1dependency for Valkey backend support.Tests