fix: UniversalAPIEmbedder now passes embedding_dims to API calls - #2180
Conversation
🤖 Open Code ReviewTarget: PR #2180 🔍 OpenCodeReview found 3 issue(s) in this PR. 1.
|
|
6bc3968 to
9f25415
Compare
|
|
Closes MemTensor#2177 The UniversalAPIEmbedder previously silently ignored the embedding_dims config field when making embeddings.create() calls. This caused models like text-embedding-3-large to always return the full default dimension embedding, making it impossible to use the dimensions parameter for reduced-dimensional embeddings. Changes: - Added _build_embedding_kwargs() helper that conditionally includes the 'dimensions' parameter when embedding_dims is set - Extracted _call_embeddings_api() method that handles both primary and backup client paths with unified dimension support - Added graceful fallback: if the API rejects the dimensions parameter (e.g. older model versions), automatically retries without it - Both primary and backup client paths now use the same dimensions-aware calling logic - Added comprehensive unit tests in test_universal_api.py Test: python3 -m py_compile src/memos/embedders/universal_api.py Test: python3 -m py_compile tests/embedders/test_universal_api.py
…ck for universal_api fallback tests
…or can await it UniversalAPIEmbedder now awaits client.embeddings.create() via asyncio.wait_for, so the mock must be an async function (AsyncMock) rather than a plain MagicMock. Introduce _awaitable_response/_mock_embedding_response helpers and patch embeddings.create at the instance level instead of relying on the default MagicMock return-value.
dc3f4f4 to
b98b8e0
Compare
✅ Automated Test Results: PASSEDAll tests passed (9/9 executed). memos_python_core/changed-repo-python: 9/9. Duration: 5s Branch: |
|
Hi @RerankerGuo , thanks for working on the embedding_dims issue—the intention of this PR is correct, but I found a blocking regression during local end-to-end testing. UniversalAPIEmbedder imports the synchronous openai.OpenAI and AzureOpenAI clients. Therefore, client.embeddings.create() returns a normal CreateEmbeddingResponse, not a coroutine. The new implementation passes that synchronous response into asyncio.wait_for(), which causes every successful real request to fail with: The current unit tests do not expose this because they replace embeddings.create() with AsyncMock, which does not match the actual SDK contract. In a local MemOS API test, /product/search still returned HTTP 200, but embedding retrieval failed internally and the API silently returned empty results. There are two additional concerns: The fallback currently catches every Exception. Timeouts, authentication failures, rate limits, and unrelated bad requests will all trigger another request without dimensions. The fallback should only handle a BadRequestError that explicitly indicates the provider/model does not support the dimensions parameter. APIConfig.get_embedder_config() does not map EMBEDDING_DIMENSION to UniversalAPIEmbedderConfig.embedding_dims. As a result, the normal API service path configures the vector database dimension but does not pass the same dimension to the embedder. I suggest keeping the client synchronous and using the OpenAI SDK’s request-level timeout: response = client.embeddings.create(
**kwargs,
timeout=timeout,
)Then catch only the relevant BadRequestError, retry without dimensions only when the error clearly says the parameter or matryoshka dimensions are unsupported, and update the tests to use synchronous MagicMock. It would also be helpful to add embedding_dims=int(os.getenv("EMBEDDING_DIMENSION", "1024")) to the API embedder configuration. After applying these changes locally, I verified the complete flow with an OpenAI-compatible test service: the first request included dimensions=1024, received an explicit unsupported-dimensions 400, retried once without dimensions, returned a 1024-dimensional vector, and /product/search completed without the previous awaitable error. |
✅ Automated Test Results: PASSEDAll tests passed (12/12 executed). memos_python_core/changed-repo-python: 12/12. Duration: 5s [advisory, non-gating] AI-generated tests on branch test/auto-gen-026d301585fcc0e3-20260811145635: 118/119 passed, 1 failed — these do NOT affect the PR verdict; review the branch manually. Branch: |
Recognize provider errors that identify an unsupported dimensions parameter through structured param/code fields while preserving invalid-value failures. Add regression coverage for structured fallback and API dimension configuration. Test: .venv/bin/python -m pytest tests/embedders/test_universal_api.py -q\nTest: .venv/bin/python -m pytest tests/api -q\nTest: .venv/bin/python -m pytest tests/embedders -q --ignore=tests/embedders/test_ark.py
|
Thanks @endxxxx for the concrete reproduction and for pushing I kept that synchronous-client fix and added the remaining structured-error coverage in
Verification:
CI update for run
I have kept the unrelated core/optional dependency issue out of this PR. |
✅ Automated Test Results: PASSEDAll tests passed (14/14 executed). memos_python_core/changed-repo-python: 14/14. Duration: 6s Branch: |
## Description This PR promotes `cachetools` from the `all` optional extra to a core MemoryOS dependency. The embedding cache implementation imports `cachetools` at module scope and is part of the core embedder workflow. However, `cachetools` was previously installed only through the `all` extra. As a result, minimal installations using only the main dependency group failed the dependency validation step with: ```text src/memos/embedders/cache.py: Top-level import of unavailable module 'cachetools' ``` This change: - Adds cachetools>=6.0.0 to the main project dependencies. - Removes the duplicate declaration from the all optional dependencies. - Updates poetry.lock without upgrading the existing locked cachetools version (6.2.1). - Ensures standard MemoryOS installations include the dependency required by the embedding cache. No runtime source code or public API behavior is changed. Related context: CI failure discovered while validating #2180 ## Type of change Please delete options that are not relevant. - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] Refactor (does not change functionality, e.g. code style improvements, linting) - [ ] Documentation update ## How Has This Been Tested? Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration - [x] Unit Test - [ ] Test Script Or Test Steps (please provide) - [ ] Pipeline Automated API Test (please provide) ## Checklist - [x] I have performed a self-review of my own code | 我已自行检查了自己的代码 - [x] I have commented my code in hard-to-understand areas | 我已在难以理解的地方对代码进行了注释 - [x] I have added tests that prove my fix is effective or that my feature works | 我已添加测试以证明我的修复有效或功能正常 - [ ] I have created related documentation issue/PR in [MemOS-Docs](https://github.com/MemTensor/MemOS-Docs) (if applicable) | 我已在 [MemOS-Docs](https://github.com/MemTensor/MemOS-Docs) 中创建了相关的文档 issue/PR(如果适用) - [ ] I have linked the issue to this PR (if applicable) | 我已将 issue 链接到此 PR(如果适用) - [ ] I have mentioned the person who will review this PR | 我已提及将审查此 PR 的人 ## Reviewer Checklist - [ ] closes #xxxx (Replace xxxx with the GitHub issue number) - [ ] Made sure Checks passed - [x] Tests have been provided
✅ Automated Test Results: PASSEDAll tests passed (14/14 executed). memos_python_core/changed-repo-python: 14/14. Duration: 5s [advisory, non-gating] AI-generated tests on branch test/auto-gen-889b4a327d218f71-20260813163910: 344/345 passed, 1 failed — these do NOT affect the PR verdict; review the branch manually. Branch: |
Description
Fixes #2177
The
UniversalAPIEmbedderpreviously silently ignored theembedding_dimsconfig field when makingembeddings.create()calls. This caused models liketext-embedding-3-largeto always return the full default dimension embedding (e.g. 3072), making it impossible to use thedimensionsparameter for reduced-dimensional embeddings.Changes
_build_embedding_kwargs()helper — conditionally includes thedimensionsparameter whenembedding_dimsis set in config_call_embeddings_api()method — handles both primary and backup client paths with unified dimension supportdimensionsparameter (e.g. older model versions or non-Ollama providers), automatically retries without ittests/embedders/test_universal_api.pyType of change
How Has This Been Tested?
python3 -m py_compile src/memos/embedders/universal_api.pypassespython3 -m py_compile tests/embedders/test_universal_api.pypasses_build_embedding_kwargs(no dims / with dims / zero dims / empty list / batch)embedding_dims=None(backward compatible)Checklist