feat(embedding): add OpenAI-compatible /v1/embeddings backend - #1671
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for generating embeddings via any OpenAI-compatible /v1/embeddings endpoint by adding the openai-compat embedding model option. It implements the OpenAICompatEmbeddingFunction using standard library urllib to handle batching, response sorting, and L2-normalization, alongside configuration updates and new unit tests. The review feedback focuses on enhancing robustness by catching additional exceptions (http.client.HTTPException and ValueError) during API requests, validating that the JSON response is a dictionary to prevent AttributeError crashes, and adding corresponding unit tests for these error scenarios.
| except (HTTPError, URLError, OSError, json.JSONDecodeError) as e: | ||
| raise EmbeddingAPIError( | ||
| f"Embedding API request to {self._url} failed: {e}. Check that the " | ||
| f"server is reachable and MEMPALACE_EMBEDDING_API_URL / embedding_api_url " | ||
| f"is correct." | ||
| ) from e |
There was a problem hiding this comment.
The current except block misses two critical exception classes that can be raised by urlopen and cause unhandled crashes:
http.client.HTTPException: Raised on low-level HTTP protocol violations (e.g.,BadStatusLine,IncompleteRead), which are common with local or overloaded LLM servers.ValueError: Raised byurlopenif the configured URL has an invalid or missing scheme (e.g.,localhost:1234instead ofhttp://localhost:1234). Note thatjson.JSONDecodeErroris a subclass ofValueError, so catchingValueErrorcovers both cases.
Adding these to the except tuple ensures all connection and configuration failures are cleanly wrapped in EmbeddingAPIError.
| except (HTTPError, URLError, OSError, json.JSONDecodeError) as e: | |
| raise EmbeddingAPIError( | |
| f"Embedding API request to {self._url} failed: {e}. Check that the " | |
| f"server is reachable and MEMPALACE_EMBEDDING_API_URL / embedding_api_url " | |
| f"is correct." | |
| ) from e | |
| except (HTTPError, URLError, OSError, http.client.HTTPException, ValueError) as e: | |
| raise EmbeddingAPIError( | |
| f"Embedding API request to {self._url} failed: {e}. Check that the " | |
| f"server is reachable and MEMPALACE_EMBEDDING_API_URL / embedding_api_url " | |
| f"is correct." | |
| ) from e |
| rows = data.get("data") | ||
| if not isinstance(rows, list): | ||
| raise EmbeddingAPIError( | ||
| f"Embedding API at {self._url} returned no 'data' array: {data.get('error', data)}" | ||
| ) |
There was a problem hiding this comment.
If the API returns a non-dictionary JSON response (such as null, a JSON list, or a raw string), data will not be a dictionary. Calling data.get("data") on a non-dictionary object will raise an unhandled AttributeError and crash the application.
We should explicitly validate that data is a dictionary before attempting to call .get() on it.
| rows = data.get("data") | |
| if not isinstance(rows, list): | |
| raise EmbeddingAPIError( | |
| f"Embedding API at {self._url} returned no 'data' array: {data.get('error', data)}" | |
| ) | |
| if not isinstance(data, dict): | |
| raise EmbeddingAPIError( | |
| f"Embedding API at {self._url} returned a non-object response: {data}" | |
| ) | |
| rows = data.get("data") | |
| if not isinstance(rows, list): | |
| raise EmbeddingAPIError( | |
| f"Embedding API at {self._url} returned no 'data' array: {data.get('error', data)}" | |
| ) |
| import json | ||
| from urllib.error import HTTPError, URLError | ||
| from urllib.request import Request, urlopen |
There was a problem hiding this comment.
To handle low-level HTTP protocol exceptions (like BadStatusLine or IncompleteRead) raised by urlopen, we need to import http.client so we can catch http.client.HTTPException.
| import json | |
| from urllib.error import HTTPError, URLError | |
| from urllib.request import Request, urlopen | |
| import http.client | |
| import json | |
| from urllib.error import HTTPError, URLError | |
| from urllib.request import Request, urlopen |
| def test_raises_on_transport_error(monkeypatch): | ||
| from urllib.error import URLError | ||
|
|
||
| def boom(req, timeout=None): | ||
| raise URLError("connection refused") | ||
|
|
||
| monkeypatch.setattr("urllib.request.urlopen", boom) | ||
| ef = embedding.OpenAICompatEmbeddingFunction("http://h", "m") | ||
| with pytest.raises(RuntimeError, match="failed"): | ||
| ef(["a"]) | ||
|
|
There was a problem hiding this comment.
Let's add unit tests to verify that both http.client.HTTPException and ValueError (e.g., from an invalid URL scheme) are correctly caught and translated into EmbeddingAPIError.
def test_raises_on_transport_error(monkeypatch):
from urllib.error import URLError
def boom(req, timeout=None):
raise URLError("connection refused")
monkeypatch.setattr("urllib.request.urlopen", boom)
ef = embedding.OpenAICompatEmbeddingFunction("http://h", "m")
with pytest.raises(RuntimeError, match="failed"):
ef(["a"])
def test_raises_on_http_exception(monkeypatch):
from http.client import HTTPException
def boom(req, timeout=None):
raise HTTPException("incomplete read")
monkeypatch.setattr("urllib.request.urlopen", boom)
ef = embedding.OpenAICompatEmbeddingFunction("http://h", "m")
with pytest.raises(embedding.EmbeddingAPIError, match="failed"):
ef(["a"])
def test_raises_on_invalid_url_scheme(monkeypatch):
# urlopen raises ValueError on invalid/missing schemes
ef = embedding.OpenAICompatEmbeddingFunction("invalid_scheme://h", "m")
with pytest.raises(embedding.EmbeddingAPIError, match="failed"):
ef(["a"])| def test_raises_and_surfaces_server_error_body(monkeypatch): | ||
| # HTTP 200 with an OpenAI-style error envelope (no "data") — surface it. | ||
| def err(req, timeout=None): | ||
| return _FakeResp(json.dumps({"error": {"message": "model not found"}}).encode()) | ||
|
|
||
| monkeypatch.setattr("urllib.request.urlopen", err) | ||
| ef = embedding.OpenAICompatEmbeddingFunction("http://h", "m") | ||
| with pytest.raises(embedding.EmbeddingAPIError, match="model not found"): | ||
| ef(["a"]) |
There was a problem hiding this comment.
Let's add a unit test to verify that non-dictionary JSON responses (like a JSON list) are handled gracefully and raise a clear EmbeddingAPIError instead of an unhandled AttributeError.
| def test_raises_and_surfaces_server_error_body(monkeypatch): | |
| # HTTP 200 with an OpenAI-style error envelope (no "data") — surface it. | |
| def err(req, timeout=None): | |
| return _FakeResp(json.dumps({"error": {"message": "model not found"}}).encode()) | |
| monkeypatch.setattr("urllib.request.urlopen", err) | |
| ef = embedding.OpenAICompatEmbeddingFunction("http://h", "m") | |
| with pytest.raises(embedding.EmbeddingAPIError, match="model not found"): | |
| ef(["a"]) | |
| def test_raises_and_surfaces_server_error_body(monkeypatch): | |
| # HTTP 200 with an OpenAI-style error envelope (no "data") — surface it. | |
| def err(req, timeout=None): | |
| return _FakeResp(json.dumps({"error": {"message": "model not found"}}).encode()) | |
| monkeypatch.setattr("urllib.request.urlopen", err) | |
| ef = embedding.OpenAICompatEmbeddingFunction("http://h", "m") | |
| with pytest.raises(embedding.EmbeddingAPIError, match="model not found"): | |
| ef(["a"]) | |
| def test_raises_on_non_dict_response(monkeypatch): | |
| def bad(req, timeout=None): | |
| return _FakeResp(json.dumps([1, 2, 3]).encode()) # JSON list instead of dict | |
| monkeypatch.setattr("urllib.request.urlopen", bad) | |
| ef = embedding.OpenAICompatEmbeddingFunction("http://h", "m") | |
| with pytest.raises(embedding.EmbeddingAPIError, match="non-object response"): | |
| ef(["a"]) |
Address the gemini-code-assist review on MemPalace#1671: - Wrap `http.client.HTTPException` (BadStatusLine / IncompleteRead — common with local/overloaded servers) and `ValueError` (invalid/missing URL scheme; also subsumes `json.JSONDecodeError`) in `EmbeddingAPIError` instead of letting them crash the caller. - Reject a non-dict top-level JSON response before calling `.get()` on it, so a JSON list/`null`/string yields a clear `EmbeddingAPIError` rather than an unhandled `AttributeError`. - Add tests for all three cases.
|
Thanks @gemini-code-assist — all three addressed in c3e6692:
For maintainers — design summary (re #1559):
Full suite is green and the unit tests need no server/network. Happy to adjust the config surface or naming before merge — wanted to give a concrete, tested proposal rather than just a design sketch. |
|
The refinements to |
Address the gemini-code-assist review on MemPalace#1671: - Wrap `http.client.HTTPException` (BadStatusLine / IncompleteRead — common with local/overloaded servers) and `ValueError` (invalid/missing URL scheme; also subsumes `json.JSONDecodeError`) in `EmbeddingAPIError` instead of letting them crash the caller. - Reject a non-dict top-level JSON response before calling `.get()` on it, so a JSON list/`null`/string yields a clear `EmbeddingAPIError` rather than an unhandled `AttributeError`. - Add tests for all three cases.
c3e6692 to
11475b9
Compare
Address the gemini-code-assist review on MemPalace#1671: - Wrap `http.client.HTTPException` (BadStatusLine / IncompleteRead — common with local/overloaded servers) and `ValueError` (invalid/missing URL scheme; also subsumes `json.JSONDecodeError`) in `EmbeddingAPIError` instead of letting them crash the caller. - Reject a non-dict top-level JSON response before calling `.get()` on it, so a JSON list/`null`/string yields a clear `EmbeddingAPIError` rather than an unhandled `AttributeError`. - Add tests for all three cases.
11475b9 to
b8f5765
Compare
Address the gemini-code-assist review on MemPalace#1671: - Wrap `http.client.HTTPException` (BadStatusLine / IncompleteRead — common with local/overloaded servers) and `ValueError` (invalid/missing URL scheme; also subsumes `json.JSONDecodeError`) in `EmbeddingAPIError` instead of letting them crash the caller. - Reject a non-dict top-level JSON response before calling `.get()` on it, so a JSON list/`null`/string yields a clear `EmbeddingAPIError` rather than an unhandled `AttributeError`. - Add tests for all three cases.
|
+1 — this is exactly what a shared / multi-machine setup needs. When several hosts share one vector store (e.g. a qdrant or pgvector collection), every host has to embed with the identical model or the vectors are mutually incompatible. Today that means each node runs its own copy of the same local ONNX model. An OpenAI-compatible It would pair naturally with the shared-backend / network-backend efforts (#1963 and the qdrant/pgvector work). Would love to see this land. |
|
Hey maintainers — we'd really love to see this land. 👋 Use case: We run several hosts against one shared vector store (Qdrant), and every node has to embed with the identical model or the vectors are mutually incompatible. Today that means running a local ONNX model copy on every machine — wasteful on CPU-only hosts and hard to keep consistent. An OpenAI-compatible Status: code review done (all bot findings addressed in Would pair naturally with the shared-backend work (#1963, Qdrant/pgvector backends). @milla-jovovich @bensig @igorls — any chance of a review? Happy to rebase or adjust anything if needed. |
Add an `embedding_model: "openai-compat"` option that computes embeddings via any OpenAI-compatible `/v1/embeddings` server (LM Studio, llama.cpp, vLLM, Ollama's OpenAI shim, self-hosted) instead of a local ONNX model. - New OpenAICompatEmbeddingFunction (stdlib urllib, no new dependency): batches requests, asks for `encoding_format: "float"` and a custom User-Agent (avoids Cloudflare 403, see MemPalace#1570), validates the response (contiguous 0..n-1 indices + well-formed vectors) before use, and L2-normalizes for the cosine collection. Exposes `embed_query` (ChromaDB 1.5 dispatches query embedding through it, not `__call__`). `name()` encodes the model id so switching it forces `mempalace repair rebuild-index`. Failures raise a module-specific `EmbeddingAPIError`. - Endpoint settings resolved by MempalaceConfig as a single source of truth: `embedding_api_url` / `embedding_api_model` / `embedding_api_key`, each overridable via the matching `MEMPALACE_EMBEDDING_API_*` env var. Whitespace-only values are treated as unset; the EF cache key fingerprints the key so a token rotation is picked up. - The miner/MCP `Device:` header reports `openai-compat (<url>)` instead of a misleading local accelerator label when this backend is active. - Opt-in; default stays minilm. Mirrors the existing `openai-compat` LLM provider naming; stays local when the endpoint is on the machine/LAN. - Tests: tests/test_embedding_api.py (no server / no network required). - Docs: README requirement note, module docstring, CHANGELOG. Refs MemPalace#1559.
Address the gemini-code-assist review on MemPalace#1671: - Wrap `http.client.HTTPException` (BadStatusLine / IncompleteRead — common with local/overloaded servers) and `ValueError` (invalid/missing URL scheme; also subsumes `json.JSONDecodeError`) in `EmbeddingAPIError` instead of letting them crash the caller. - Reject a non-dict top-level JSON response before calling `.get()` on it, so a JSON list/`null`/string yields a clear `EmbeddingAPIError` rather than an unhandled `AttributeError`. - Add tests for all three cases.
After rebasing MemPalace#1671 onto current develop: - Opt test_embedding_api out of conftest's stable EF mock so the get_embedding_function selection tests exercise the real factory. - Move the CHANGELOG entry from released 3.7.0 Performance into Unreleased Features (rebase context had drifted).
b8f5765 to
d9a24c7
Compare
Maintainer rebase onto current
|
| Commit | What |
|---|---|
d471a9e |
feat: OpenAI-compatible /v1/embeddings backend (original) |
f272c84 |
fix: harden API error handling (original review follow-up) |
d9a24c7 |
post-rebase: opt test_embedding_api out of conftest’s stable EF mock + move CHANGELOG entry to Unreleased |
Why the extra commit was needed
Current develop has an autouse fixture in tests/conftest.py that stubs get_embedding_function for most modules (avoids loading ONNX on Windows). Without opting test_embedding_api into _REAL_EMBEDDING_TEST_MODULES, the selection-branch tests received _StableTestEmbeddingFunction and failed (3 failures). Same pattern as test_embedding / test_embeddinggemma.
CHANGELOG context had drifted: the feature bullet rebased under released 3.7.0 → Performance. Moved it to Unreleased → Features.
Verification (local)
uv run pytest tests/test_embedding_api.py tests/test_embedding.py tests/test_embeddinggemma.py -v
# 75 passed
uv run ruff check mempalace/embedding.py mempalace/config.py tests/test_embedding_api.py tests/conftest.py
# All checks passed
Still open (non-blocking for CI, useful before merge)
- EF cache lock — ONNX path uses
_EF_CACHE_LOCK; openai-compat still does lock-free get/set (double-create race under concurrent first init). - Privacy warning — mirror
llm_client._endpoint_is_localwhen the embedding URL is not loopback/LAN. - CI — waiting on checks after this push.
@maximilize — sorry for the force-push on your branch; maintainerCanModify was on. Happy to adjust anything if you’d rather take the rebase yourself next time.
Closes the “open since June / needs rebase” blocker called out on #1559.
What does this PR do?
Adds an
embedding_model: "openai-compat"option that computes embeddings via any OpenAI-compatible/v1/embeddingsendpoint (LM Studio, llama.cpp, vLLM, Ollama's OpenAI shim, or a self-hosted server) instead of a local ONNX model — useful for larger / multilingual embedders (e.g. Qwen3-Embedding) or GPU offload.OpenAICompatEmbeddingFunction(stdliburllibonly — no new dependency): batches requests, sendsencoding_format: "float"+ amempalace/<ver>User-Agent (avoids the Cloudflare 403 from openai-compat provider probe fails with 403 on Cloudflare-fronted endpoints (Python-urllib User-Agent blocked) #1570), validates the response (contiguous0..n-1indices + well-formed vectors), and L2-normalizes for the cosine collection. Implementsembed_query(ChromaDB 1.5 dispatches query embedding through it).name()encodes the model id so switching it surfaces as a clear rebuild-index signal. Failures raise a module-specificEmbeddingAPIError.MempalaceConfigas a single source of truth:embedding_api_url/embedding_api_model/embedding_api_key, each overridable via the matchingMEMPALACE_EMBEDDING_API_*env var.minilm; nothing changes unless you selectopenai-compat. When the endpoint is on your machine/LAN no content leaves your network — consistent with the zero-API-by-default principle (the endpoint is the user's explicit choice and may be fully local).openai-compatLLM provider naming. The selector reusesembedding_model(notembedding_device) because the device axis is for hardware accelerators.Generalizes the Ollama-only approach in #982 (currently conflicting) to any OpenAI-compatible endpoint — Ollama works via its
/v1shim. Does not touch reranking (#1032).How to test
Point at any OpenAI-compatible server via
~/.mempalace/config.json:{ "embedding_model": "openai-compat", "embedding_api_url": "http://localhost:1234", "embedding_api_model": "<server-side model id>" }(or the equivalent
MEMPALACE_EMBEDDING_API_*env vars), then:Unit tests need no server and no network (
urlopenis mocked):Checklist
uv run pytest tests/ -v— 2304 passed, 3 skipped; no network / API keys needed)ruff check .)Related issues
Closes #1559. Related: #756, #1261, #1563, #903, #1561. Generalizes the Ollama-only #982.