| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
|
||
| 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
Sorry, something went wrong.
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.
Sorry, something went wrong.
|
|
||
| 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.
Sorry, something went wrong.
|
@franciscojavierarceo, I have created an RFC as you requested Docs |
Sorry, something went wrong.
|
@franciscojavierarceo, let me know if you need any more changes, or if this is good to go. |
Sorry, something went wrong.
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.
Sorry, something went wrong.
| "/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?
Sorry, something went wrong.
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
Sorry, something went wrong.
There was a problem hiding this comment.
Auth regression test is out of scope for this PR, will address in a follow-up
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
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
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
There was a problem hiding this comment.
Sure, I remove the vector field from the feature_names
Sorry, something went wrong.
| } | ||
| ``` | ||
|
|
||
| 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.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
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
Sorry, something went wrong.
There was a problem hiding this comment.
Now added this endpoint Give it another look when you get time
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
| ] != 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()?
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
| | `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?
Sorry, something went wrong.
|
|
||
| ```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
Sorry, something went wrong.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
yeah so then let's not add litellm to scope
Sorry, something went wrong.
| @@ -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?
Sorry, something went wrong.
There was a problem hiding this comment.
Currently we are doing only the vector search
Sorry, something went wrong.
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.
Sorry, something went wrong.
|
@patelchaitany can you also document this feature as an alpha API in the docs? |
Sorry, something went wrong.
|
@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 |
Sorry, something went wrong.
We should just call it search() or search_online_store() or openai_search(). I like search() as the public API that's non-OpenAI and then we could have a search() method and openai_search(). method. |
Sorry, something went wrong.
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 retrieve_online_documents_openai to openai_search as that is introduced in this change. But about the search API, I could simply wrap it in a wrapper, but couldn't change it completely as it might be a big change. |
Sorry, something went wrong.
|
yeah just rename to openai_search sounds good to me |
Sorry, something went wrong.
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.
Sorry, something went wrong.
| """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') |
Sorry, something went wrong.
|
⚠️ Please install the Codecov Report❌ Patch coverage is 34.59336% with 571 lines in your changes missing coverage. Please review.
@@ 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:
|
Sorry, something went wrong.
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. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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.
Which issue(s) this PR fixes:
#5615
Misc