feat: Making feast vector store with open ai search api compatible#6121
Conversation
e45f167 to
c8392a9
Compare
7e8adfb to
3f541ad
Compare
cd692b9 to
086bed5
Compare
086bed5 to
9778002
Compare
68c843c to
e29e557
Compare
f0552c7 to
dafe9fd
Compare
|
|
||
| If you've tried to connect an AI agent to Feast's vector search, you've probably hit this wall: the agent needs to search your feature store, but Feast expects a raw embedding vector. The agent doesn't have one. It has a question in English. | ||
|
|
||
| Until now, the workaround was ugly. You'd call an embedding provider (OpenAI, Ollama, whatever) to turn the text into a float array, then pass that array to Feast's `retrieve-online-documents` endpoint. Every client had to know both APIs, carry both sets of credentials, and run glue code whose only job was bridging the gap. |
There was a problem hiding this comment.
we should deprecate the retrieve-online-documents API and rename it as search API to highlight the more generic primitive we want to support, which will offer a lot more long term flexibility.
CC @ntkathole
There was a problem hiding this comment.
Okay I have added the new endpoint /search and deprecated the retrieve-online-documents as it is still available for the backward compatibility.
|
|
||
| When Feast receives this request, it: | ||
|
|
||
| 1. Embeds the query server-side using the model configured in `feature_store.yaml` (via [LiteLLM](https://docs.litellm.ai/), which supports OpenAI, Ollama, Azure, Cohere, HuggingFace, and 100+ other providers). |
There was a problem hiding this comment.
yeah this seems acceptable if it's opt-in but we should also support sentence transformers.
|
@franciscojavierarceo, I have created an RFC as you requested Docs |
|
@franciscojavierarceo, let me know if you need any more changes, or if this is good to go. |
franciscojavierarceo
left a comment
There was a problem hiding this comment.
Thanks for pushing this through. I think we need another pass before merge. The main blockers are that the new OpenAI-compatible endpoint bypasses the existing online-read permission path, exposes Feast distances as OpenAI scores, and accepts OpenAI-shaped request fields that are not actually applied. I also left one existing remote-store mutation issue because it still reproduces on this head.
CI note: the latest unit-test-python (3.12, macos-14) check is still failing with two 120s CLI/apply timeouts, so this is not green yet.
| "/v1/vector_stores/{vector_store_id}/search" | ||
| ): | ||
| try: | ||
| result = await store.retrieve_online_documents_openai( |
There was a problem hiding this comment.
i think there's a bug here: this new endpoint bypasses _get_features, so auth-enabled servers never run the existing READ_ONLINE check for the target feature view. /search goes through _get_features() before retrieval, but this path calls store.retrieve_online_documents_openai(...) directly. Can we resolve the feature view, assert AuthzedAction.READ_ONLINE, and add an auth regression test for this endpoint?
There was a problem hiding this comment.
Okay I will add those test as well as the authentication checks for this newly added api endpoints
There was a problem hiding this comment.
Auth regression test is out of scope for this PR, will address in a follow-up
| for key, values in response_dict.items(): | ||
| val = values[i] if i < len(values) else None | ||
| if key == "distance": | ||
| score = float(val) if val is not None else 0.0 |
There was a problem hiding this comment.
i think this is returning the wrong OpenAI semantics. distance is lower-is-better for the default pgvector L2 path, but the OpenAI response field is score, which clients treat as higher-is-better relevance and use with score_threshold. We should either normalize per metric before putting it in score, or avoid claiming OpenAI score semantics here.
There was a problem hiding this comment.
Added _distance_to_score(distance, metric) in utils.py and applied it at the store level in Postgres, SQLite, and Milvus. Elasticsearch and Qdrant already return higher-is-better scores, so no changes needed there
| query=request.query, | ||
| max_num_results=request.max_num_results or 10, | ||
| filters=request.filters, | ||
| ranking_options=( |
There was a problem hiding this comment.
This accepts and forwards ranking_options, including score_threshold, but retrieve_online_documents_openai() never applies it. That makes clients think thresholding is enforced when it is silently ignored. Can we either implement score_threshold now or reject unsupported ranking_options/rewrite_query with a 4xx until they are wired?
| if requested_features is None: | ||
| requested_features = [] | ||
| if "distance" not in requested_features: | ||
| requested_features.append("distance") |
There was a problem hiding this comment.
This still mutates the caller-owned requested_features list. When this is called through FeatureStore._retrieve_from_online_store_v2, Feast later builds features_to_request = requested_features + ["distance"], so distance can appear twice in the response metadata. Please copy before appending, or keep synthetic fields owned by one layer only.
| if features_to_retrieve: | ||
| feature_names = features_to_retrieve | ||
| else: | ||
| feature_names = [f.name for f in feature_view.features] |
There was a problem hiding this comment.
By default this requests every feature from the feature view, which includes the embedding/vector field itself. The response conversion below then copies every non-entity, non-distance field into attributes, so OpenAI-compatible search responses can include the raw embedding vector as metadata. Can we exclude vector fields by default and only return scalar metadata/content fields unless explicitly requested?
There was a problem hiding this comment.
Sure, I remove the vector field from the feature_names
| } | ||
| ``` | ||
|
|
||
| String equality filters work on all backends. Numeric and boolean filters require `enable_openai_compatible_store: true` in the online store config. |
There was a problem hiding this comment.
This doc says string equality filters work on all backends without the new store flag, but the Postgres and SQLite implementations reject any filter when enable_openai_compatible_store is false. Either the runtime should allow text-only filters on existing schemas, or the docs should say all filtering requires the flag for those backends.
| content={ | ||
| "error": { | ||
| "message": str(e), | ||
| "type": "invalid_request_error", | ||
| } | ||
| }, |
|
|
||
| Until now, the workaround was ugly. You'd call an embedding provider (OpenAI, Ollama, whatever) to turn the text into a float array, then pass that array to Feast's vector search endpoint (`POST /search`, formerly `retrieve-online-documents`). Every client had to know both APIs, carry both sets of credentials, and run glue code whose only job was bridging the gap. | ||
|
|
||
| Feast now has a new endpoint: `POST /v1/vector_stores/{feature_view}/search`. It follows the [OpenAI Vector Store Search API](https://platform.openai.com/docs/api-reference/vector-stores-search) format. You send text, Feast handles the embedding internally, and you get results back in the same JSON shape that OpenAI returns. No float arrays, no extra SDK. |
There was a problem hiding this comment.
but this isn't openai compatible.
There was a problem hiding this comment.
I initially went with feature view names for simplicity from implementation and user perspective but you're right it should follow the OpenAI vs_{hash} format for proper compatibility.
it is an easy addition, plan is to hash the project:feature_view_name and cache it at server startup time and it refresh at every registry TTL, Will also add GET /v1/vector_stores so user can discover the vs_ IDs for their feature views
working on it
There was a problem hiding this comment.
Now added this endpoint Give it another look when you get time
| import requests | ||
|
|
||
| result = requests.post( | ||
| "http://feast-server:6566/v1/vector_stores/product_catalog/search", |
There was a problem hiding this comment.
the product_catalog is not vector store compatible. it should be some vs_{hash} identifier.
franciscojavierarceo
left a comment
There was a problem hiding this comment.
Thanks for the update. CI is green now and some of the earlier concerns are fixed, but I still don't think this is ready to merge. The main blocker is that the OpenAI score fix is currently changing the existing Feast distance contract for native /search / retrieve_online_documents_v2 callers. I also left notes on the remaining OpenAI-compat/doc mismatches.
| ] != float("inf"): | ||
| dist_val = ValueProto() | ||
| dist_val.double_val = entity_data["vector_distance"] | ||
| dist_val.double_val = _distance_to_score( |
There was a problem hiding this comment.
i think this fix landed in the wrong layer. retrieve_online_documents_v2() and /search still expose this synthetic field as distance, but Postgres now stores a higher-is-better OpenAI score in it. That changes the native Feast contract for non-OpenAI callers, and the same pattern appears in SQLite/Milvus. Can we keep the stores returning raw distances and convert to OpenAI score only in retrieve_online_documents_openai()?
| unsupported.append("ranking_options.score_threshold") | ||
| if ranking_options.get("ranker") is not None: | ||
| unsupported.append("ranking_options.ranker") | ||
| if rewrite_query is not None: |
There was a problem hiding this comment.
this rejects rewrite_query: false, even though that is the no-op/default shape an OpenAI-compatible client can send. OGX keeps this field for API compatibility and ignores false at the store layer. Can we only reject rewrite_query=True until query rewriting is implemented?
| | `query` | `string` or `list[string]` | (required) | Plain text search query. Lists are joined with spaces before embedding. | | ||
| | `max_num_results` | `int` | `10` | Maximum number of results to return. | | ||
| | `filters` | `object` | `null` | OpenAI-style filters (see below). | | ||
| | `ranking_options` | `object` | `null` | Accepted but not yet applied. | |
There was a problem hiding this comment.
the docs now say ranking_options and rewrite_query are accepted but not applied, but the implementation raises 422 when score_threshold, ranker, or any rewrite_query value is present. Can we make the docs match the actual behavior, or wire the supported no-op/default cases?
|
|
||
| ```yaml | ||
| embedding_model: | ||
| provider: litellm # default; can be omitted |
There was a problem hiding this comment.
i'm a little worried about requiring litellm for embeddings, why wouldn't we embed within feast? in fact, i think this is a good usage of this. even though it's not as efficient as using a GPU, it's good enough.
CC @ntkathole
There was a problem hiding this comment.
We want to support wide range of the embedding model and using litellm for embedding looks feasible option to me but in feast we support the sentence-transformers for doing it entirely in the feast.
There was a problem hiding this comment.
yeah so then let's not add litellm to scope
| @@ -0,0 +1,382 @@ | |||
| --- | |||
| title: "Making Feast Speak OpenAI: Vector Search Without the Glue Code" | |||
There was a problem hiding this comment.
| title: "Making Feast Speak OpenAI: Vector Search Without the Glue Code" | |
| title: "Using Feast's OpenAI Compatible Search API" |
are we limiting this only to vector search? or are we supporting hybrid as well?
There was a problem hiding this comment.
Currently we are doing only the vector search
franciscojavierarceo
left a comment
There was a problem hiding this comment.
we can address this in a follow up PR but right now the diagram includes LiteLLM still but we took that out, so we should remove that from the diagram but let's get this merged.
|
@patelchaitany can you also document this feature as an alpha API in the docs? |
|
@franciscojavierarceo, I had question regarding adding the /search endpoint should we Include the retrieve_online_documents_openai() under that as well or should it remain only accessible through this endpoint |
We should just call it I like |
Thanks @franciscojavierarceo ,so it's a change for the name, but there is not any public method for search() in the Python SDK yet (search is available as an API endpoint). If renaming the method / introducing something like search could cost more changes, so I could rename |
|
yeah just rename to openai_search sounds good to me |
jyejare
left a comment
There was a problem hiding this comment.
This PR adds OpenAI-compatible vector store search endpoints to Feast, enabling text-based search with server-side embedding. The implementation is comprehensive with good documentation and multi-backend support. However, there are several security concerns around input validation and potential injection attacks that need to be addressed before merging.
| """Escape a string for safe use inside a Milvus single-quoted literal. | ||
|
|
||
| Backslashes must be escaped first; otherwise a trailing backslash in the | ||
| input would combine with the escaped quote to break out of the literal. | ||
| """ |
There was a problem hiding this comment.
[Critical] SQL Injection vulnerability in Milvus string escaping
The Milvus string escaping function only escapes backslashes and single quotes, but doesn't handle other potential injection vectors. Milvus boolean expressions could be vulnerable to injection if field names or other components aren't properly validated.
Suggested:
| """Escape a string for safe use inside a Milvus single-quoted literal. | |
| Backslashes must be escaped first; otherwise a trailing backslash in the | |
| input would combine with the escaped quote to break out of the literal. | |
| """ | |
| def _milvus_escape_string(s: str) -> str: | |
| """Escape a string for safe use inside a Milvus single-quoted literal.""" | |
| # More comprehensive escaping for Milvus | |
| return s.replace("\\", "\\\\").replace("'", "\\'") | |
| .replace('"', '\\"').replace('\n', '\\n').replace('\r', '\\r') |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6121 +/- ##
==========================================
- Coverage 45.94% 45.74% -0.21%
==========================================
Files 412 414 +2
Lines 48864 49668 +804
Branches 6913 7078 +165
==========================================
+ Hits 22452 22722 +270
- Misses 24859 25366 +507
- Partials 1553 1580 +27
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Signed-off-by: Chaitany patel <patelchaitany93@gmail.com>
Signed-off-by: Chaitany patel <patelchaitany93@gmail.com>
…gistration fastapi_mcp 0.4.0 resolve_schema_references() has no cycle detection. Feast's OpenAPI schema contains self-referential protobuf types (Value -> Struct -> Value) which trigger a RecursionError. The error is silently caught, so the /mcp route never gets registered and CI gets a 404. Add _resolve_schema_references_safe() that tracks a seen-refs set to break circular chains, and monkey-patch it into fastapi_mcp before FastApiMCP processes the schema. Non-circular schemas produce identical output to the original. Signed-off-by: Chaitany patel <patelchaitany93@gmail.com>
…in alpha-vector-database.md; enhance error handling in feature_server.py for invalid requests; clarify unsupported parameters in feature_store.py; ensure requested_features is a list in remote.py Signed-off-by: Chaitany patel <patelchaitany93@gmail.com> Signed-off-by: Chaitany Patel <patelchaitany93@gmail.com>
true
- Document score conversion formulas and distance
metrics in
alpha-vector-database.md
- Add Sentence Transformers as a supported embedding
provider
- Fix embedding_model config example in docstring
Signed-off-by: Chaitany Patel <patelchaitany93@gmail.com>
/v1/vector_stores/{id} endpoints
with RBAC enforcement (DESCRIBE permission)
- Introduce VectorStoreRegistry cache that derives
vs_{sha256} IDs from
project + feature view name, refreshed on registry
TTL cycle
- Replace raw feature view names with stable vs_
identifiers in search
responses (file_id, filename fields)
- Update docs, blog post, and integration tests for
new ID scheme
- Add unit tests for VectorStoreRegistry, ID
generation, and object building
Signed-off-by: Chaitany Patel <patelchaitany93@gmail.com>
Signed-off-by: Chaitany Patel <patelchaitany93@gmail.com>
Signed-off-by: Chaitany Patel <patelchaitany93@gmail.com>
|
@franciscojavierarceo let me know if any changes are required. |
What this PR does / why we need it:
This PR making the feast vector store api with open ai search api compatible so.
This are the changes are made.
POST /v1/vector_stores/{vector_store_id}/searchthat matches the OpenAI vector store search APIvector_store_idjust maps to a feature view nameretrieve_online_documents_v2, and returns results in OpenAI'svector_store.search_results.pageformatfeature_store.yamlunder a newembedding_modelsection (model, api_key, api_base, api_version, dimensionsfilter_models.pywith two Pydantic models:ComparisonFilter(eq, ne, gt, gte, lt, lte, in, nin) andCompoundFilter(and/or, nestable)field == 'value'entity_key IN (SELECT ...)enable_openai_compatible_storeconfig flag on every store backendvalue_numcolumn that stores int, float, double, and bool values natively alongside the existing `value_textvalueinstead ofvalue_textqueryparam renamed toembeddingsince that's what it actually isWhich issue(s) this PR fixes:
#5615
Misc