feat(stores): add Valkey Search vector store backend#10770
Conversation
f72d11f to
17c3e0a
Compare
17c3e0a to
86ac171
Compare
| # VALKEY_HNSW_M=16 | ||
| # VALKEY_HNSW_EF_CONSTRUCTION=200 | ||
| # VALKEY_HNSW_EF_RUNTIME=10 | ||
| # VALKEY_REQUEST_TIMEOUT_MS=5000 |
There was a problem hiding this comment.
I think at least some of these should go in a model config. Then you can have multiple stores configs.
There was a problem hiding this comment.
Done — the backend now reads its config from the store's model config instead of VALKEY_* env vars. A store is configured with a model YAML (name: = the store, backend: valkey-store, and an options: list like addr:host:6379 / index_algo:HNSW), so different stores can each point at their own Valkey server/index within one process. Dropped the env var block from .env and documented the options in stores.md.
| } | ||
|
|
||
| BeforeEach(func() { | ||
| valkeyAddr = os.Getenv("VALKEY_ADDR") |
There was a problem hiding this comment.
Please avoid putting env accesses throughout the code. Model config is the natural place instead of env vars.
There was a problem hiding this comment.
Done — removed all env access from the backend. loadConfig now parses the model-config options: list threaded through LoadModel (ModelOptions.Options), and core/backend/stores.go resolves each store's ModelConfig and passes its options down. The integration test configures the store through those same options; VALKEY_ADDR remains only as the harness gate that locates the test server, not as backend config.
|
@mudler This PR is ready for a second set of eyes and merge whenever you get a chance 🙏 |
| ### This backend is configured through a model config named after the store, not | ||
| ### environment variables — create a YAML with `backend: valkey-store` and an | ||
| ### `options:` list (addr, index_algo, distance_metric, ...). Different stores can | ||
| ### then use different Valkey servers/indexes. See docs/content/features/stores.md. |
There was a problem hiding this comment.
Done — removed the comment block. The configuration is already documented in docs/content/features/stores.md.
|
Heads up @daric93: this branch now conflicts with master in three files, all of the append-both-sides kind:
New backends landed on master since this was opened, so the resolution is a union: keep master's new entries AND the valkey-store ones (in Once that's done this is unblocked: it already has an approval from richiejp and CI was green. @mudler good to merge after the rebase. |
|
@mudler recommendation: merge this one. Between the two competing implementations of #10708 (this and #10801), this PR is the more complete: it has richiejp's approval after three addressed review rounds, a full green CI run (including both cpu-valkey-store image builds and the darwin metal build), mock-based unit tests that don't need a container in CI, an env-gated integration suite mirroring local-store's, and it is the only one where per-store model config actually works end to end (the Three things before merge:
As a nice follow-up (no blocker), #10801's |
Add a new built-in Go gRPC store backend 'valkey-store' that implements the four Stores RPCs (Set/Get/Delete/Find) against the Valkey Search module (FT.*) using the pure-Go github.com/valkey-io/valkey-go client. It is selected via the existing per-request 'backend' field on /stores, so there is no proto or HTTP API change, and it mirrors the in-memory local-store while adding persistence across restarts and opt-in HNSW. Each vector is a Valkey HASH keyed by hex(little-endian float32); the index is created lazily on first Set (FLAT+COSINE by default), cosine similarity is derived as 1-distance, and namespaces get a collision-resistant token. Includes unit tests (valkey-go mock) and env-gated integration tests against valkey/valkey-bundle, plus build/matrix/gallery wiring and docs. Assisted-by: Kiro:claude-opus-4.8 golangci-lint Signed-off-by: Daria Korenieva <daric2612@gmail.com>
- Load now recovers the persisted vector DIM from FT.INFO (not just index existence), so a post-restart Set/Find validates against the real DIM instead of silently re-learning a wrong one and dropping mismatched vectors from the index. This also restores Find's dimension check after a restart. - StoresFind treats a dropped/missing index as an empty store (empty result, no error) and clears the stale indexCreated flag, matching local-store's empty-store behaviour. - StoresSet reuses checkDims for its per-key length check so the four RPCs share one dimension-guard implementation. - Add unit tests for FT.INFO dimension recovery, loadIndexState, and the dropped-index Find path. Assisted-by: Kiro:claude-opus-4.8 Signed-off-by: Daria Korenieva <daric2612@gmail.com>
…il-fast Addresses external review comments on the valkey-store backend: - StoresFind now rejects a nil/empty query Key before dereferencing it, so a malformed gRPC request can no longer panic the backend. - TLS: derive ServerName (SNI) from the VALKEY_ADDR host so certificate verification works for IP-addressed endpoints, and add VALKEY_TLS_CA_CERT (custom CA bundle) and VALKEY_TLS_SKIP_VERIFY (testing-only) knobs. - Config integer parsing now fails fast on a malformed value (e.g. VALKEY_HNSW_M=1x6) instead of silently defaulting, matching the fail-fast behaviour of the index-algo/distance-metric validation. - Add VALKEY_DB (SELECT n) support for logical-DB isolation. - Cap the human-readable part of a namespace token at 64 chars so a very long model name cannot produce an unbounded key prefix / index name (the appended short hash keeps distinct namespaces collision-free). - Document the KNN-query injection-safety invariant (fields are constants) and why StoresGet uses a single aggregate DoMulti deadline for reads. - Unit tests for the Find nil/empty-key guard, fail-fast HNSW parsing, and VALKEY_DB parsing/validation; docs + .env updated for the new vars. Assisted-by: Kiro:claude-opus-4.8 golangci-lint Signed-off-by: Daria Korenieva <daric2612@gmail.com>
richiejp asked that the valkey-store backend take its configuration from a model config rather than process-wide VALKEY_* environment variables, so multiple stores can each have their own Valkey config within one LocalAI process. This removes every env access from the backend and routes config through the model-config seam every other backend uses. - config.go: loadConfig(opts *pb.ModelOptions) now parses the model config `options:` list (key:value strings, split on the first ':') instead of os.Getenv. Option keys mirror the old VALKEY_* names without the prefix (addr, index_algo, distance_metric, ...). Defaults, fail-fast validation and the mandatory client name are unchanged. - store.go: Load threads opts into loadConfig; TLS comments/errors renamed off the VALKEY_* names. - core/backend/stores.go: StoreBackend and NewVectorStore take a *config.ModelConfigLoader, resolve the per-store ModelConfig by store name, and pass its Options (and Backend when unset) to the backend via WithLoadGRPCLoadModelOpts. No config -> default backend + built-in defaults, preserving the zero-config experience. - Endpoints/routes/application: thread the config loader to StoreBackend. - Unit + integration tests: configure via options; the integration test passes addr through the model-config path (VALKEY_ADDR is now only the test harness locating the server). - docs + .env: document the model-config options, drop the env var table. Assisted-by: Kiro:claude-opus-4.8 Signed-off-by: Daria Korenieva <daric2612@gmail.com>
…nfigured via model config, not env vars — the comment was unnecessary noise in .env. The configuration is already documented in docs/content/features/stores.md. Signed-off-by: Daria Korenieva <daric2612@gmail.com>
…robing Mirror local-store's pattern: reject model names without store.NamespacePrefix so the model loader's greedy autoload probe cannot bind an arbitrary model name to the vector store backend (the mudler#9287 failure mode). Also adds unit tests for the gate covering: prefixed namespace, prefix alone, unprefixed model name, empty model, and nil opts. Signed-off-by: Daria Korenieva <daric2612@gmail.com>
07d05fa to
b7d7287
Compare
|
All three items addressed:
CI should be green again — all valkey-store unit tests pass locally (54/54). |
…ion Add support for resolving Valkey credentials from environment variables named in the model config, mirroring cloud-proxy's api_key_env pattern. This keeps secrets out of model YAML files and lets distinct store configs each reference their own credentials. Options: username_env / password_env name the env var holding the value. The direct username / password options still work and take precedence when both are set (backward compatible). Includes 5 unit tests and updated stores.md documentation. Signed-off-by: Daria Korenieva <daric2612@gmail.com>
|
Also added the options:
- username_env:MY_VALKEY_USER
- password_env:MY_VALKEY_PASSWORDDirect |
|
Re-reviewed the current head after the rebase and the follow-up commits. The three earlier blockers are confirmed addressed: DCO is green (all commits signed off), the However, the conflict resolution in the rebase went wrong in two files, and since CI has not run on this head yet (fork awaiting approval, only the DCO check executed), nothing has caught it:
Neither issue touches the backend code itself, which I have no further findings on: the mock-based unit suite (Ginkgo, no container in CI), the env-gated integration suite mirroring Three small optional pickups that #10801 has and this PR does not: a @mudler recommendation: this PR and #10801 are competing implementations of #10708. I recommend merging this one once the two rebase artifacts above are fixed and CI is approved to run on the head. It was opened first by the issue author, has been through three review rounds with @richiejp (his changes-requested was dismissed with thanks after the fixes), and is the only one of the two where per-store model config works end to end. #10801 should then be closed with credit: its namespace gate and env-indirect credentials arrived there first and have already been adopted here. |
…two issues introduced by the conflict-resolution script during the rebase onto master: 1. .github/backend-matrix.yml: valkey-store entries were merged INTO the cloud-proxy entries (duplicate keys in same YAML map items) instead of being separate list items. This broke cloud-proxy Linux builds and the cloud-proxy darwin entry lost its build-type/lang. Fixed by making them standalone entries and restoring cloud-proxy exactly as on master. 2. Makefile: duplicated .NOTPARALLEL and docker-build-backends lines. Collapsed to single lines that are master's current content plus the valkey-store additions. Also adds the three optional pickups from mudler#10801: - /valkey-store in .gitignore (the built binary) - valkey-store row in docs/content/reference/compatibility-table.md - valkey-store line in backend/README.md Signed-off-by: Daria Korenieva <daric2612@gmail.com>
|
Fixed both rebase artifacts in
Also picked up the three optional items: CI should be clean now. |
|
Follow-up after the merge from master (head
No further findings; the backend code was already clean in my earlier rounds. @mudler this is good to merge. It and #10801 are competing implementations of #10708; my recommendation stands: merge this one (opened first by the issue author, three review rounds, per-store model config works end to end) and close #10801 with credit for the namespace gate + env-indirect credentials that were adopted here. |
Description
Closes: #10708
Adds a new built-in Go gRPC store backend
valkey-storethat implements the fourStores*RPCs (Set / Get / Delete / Find) against the Valkey Search module (FT.*vector similarity), selectable via the existing per-requestbackendfield on the/storesendpoints. It mirrors the in-memorylocal-storeand adds persistence across restarts plus opt-in HNSW."backend": "valkey-store"(alias"valkey").github.com/valkey-io/valkey-gov1.0.76 (official, pure Go, no CGO).hex(little-endian float32); theFTindex is created lazily on firstSet(FLAT + COSINE by default), and cosine similarity is derived as1 - distanceto matchlocal-storesemantics exactly.sanitize(name) + short sha256).VALKEY_*env vars (addr, auth, TLS, index algo + HNSW knobs, distance metric, per-command request timeout, mandatoryClientName).Why
The stores subsystem is designed to be pluggable, but the only shipped backend (
local-store) is in-memory and loses all data on restart, and itsFindis an O(N) scan. Face/voice biometric registries and the router embedding cache all consume this seam, so a durable, scalable Valkey-backed backend benefits them with zero caller changes.Why
valkey-goand notvalkey-glide? The officialvalkey-glideGo client is built on a Rust core via FFI, which forces CGO and a per-arch native library — that breakslocal-store'sCGO_ENABLED=0static build and the Linux/Darwin backend matrix.valkey-gois pure Go with a typedFT.SEARCHbuilder, aVectorString32encoder, and a gomock mock package for unit tests.Testing
valkey-gomock, no container): 35 specs — RPC command-shape/wire contract, empty/len/dim rejects, omit-missing Get, tolerate-missing Delete,topK<1reject, distance→similarity conversion (0→1, 1→0, 2→−1), lazyFT.CREATE, HNSW arg-shape, key-encoding round-trip (incl.-0.0/NaN), namespace-token collision resistance, config parsing.make test/go run ...ginkgo -r backend/go/valkey-store.Label("valkey"), env-gated onVALKEY_ADDR, againstvalkey/valkey-bundle:9.1.0on :6379): 14 specs mirroring thelocal-storesuite one-for-one (set/get/delete/find, exact cosine for orthogonal/opposite unit + non-unit vectors, triangle inequality incl. random 768-d) plus a persistence/restart test and aCLIENT LISTclient-name assertion. Index back-fill is absorbed with a boundedfindpoll (notime.Sleep). Skipped automatically whenVALKEY_ADDRis unset, so unit CI needs no container.golangci-lintclean on the changed packages. All Valkey logic lives inbackend/go/valkey-store/(outsideCOVERAGE_COVERPKG), so the coverage ratchet is unaffected.Run integration locally:
Scope / follow-ups
Purely additive and opt-in per request. Deferred to follow-ups: hybrid / metadata-filter search (needs new API surface beyond the four RPCs), Valkey Cluster mode, and single-instance multi-namespace consolidation.
Notes for Reviewers
cc @mudler
Signed commits