-
Notifications
You must be signed in to change notification settings - Fork 0
Add Valkey backend support with conditional SORTBY handling #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| _never_supports_sortby, | ||
| raising=True, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Add assertion/ test for sorting: scores = [extract_score(item) for item in res]; assert is_nondecreasing(scores), f"KNN results must be sorted: {scores}"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added assertion
|
|
||
| _sortby_supported: bool | None = None | ||
|
|
||
| def _check_sortby_support(self, index_name: str, sort_field: str) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update docstring: Add "NOTE: Valkey KNN results are ALWAYS sorted by distance. 'The first name/value pair is for the distance computed' - Ref: https://github.com/valkey-io/valkey-search/blob/main/COMMANDS.md"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
| self._sortby_supported = False | ||
| else: | ||
| self._sortby_supported = True | ||
| return self._sortby_supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: verify it's Valkey before skipping SORTBY, as it is uncertain if there might be other cases (not Valkey) where SORT BY is not supported and no default sorting is guaranteed.
except ResponseError as e:
if "SORTBY" in str(e):
try:
info = self._client.info("server")
is_valkey = info.get("server_name") == "valkey" or "valkey_version" in info
self._sortby_supported = not is_valkey
except Exception:
self._sortby_supported = False
else:
self._sortby_supported = True
return self._sortby_supportedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added verification
docs/index.rst
Outdated
| * [x] Support `Chroma <https://github.com/chroma-core/chroma>`_\ , the AI-native open-source embedding database. | ||
| * [x] Support `DocArray <https://github.com/docarray/docarray>`_\ , DocArray is a library for representing, sending and storing multi-modal data, perfect for Machine Learning applications. | ||
| * [x] Support `Redis <https://redis.io/>`_. | ||
| * [x] Support `ValkeySearch <https://github.com/valkey-io/valkey-search/>`_\ , provided as a Valkey module, is a high-performance Vector Similarity Search engine optimized for AI-driven workloads. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of ValkeySearch - suggesting to add Valkey (with valkey-search)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with @alexey-temnikov suggested changes
This PR introduces Valkey integration for the Redis-based vector store in GPTCache. Key changes include:
Valkey compatibility:
Conditional SORTBY logic:
No breaking changes: