weiqingy commented on code in PR #870:
URL: https://github.com/apache/flink-agents/pull/870#discussion_r3556633134
##########
python/flink_agents/api/vector_stores/vector_store.py:
##########
@@ -344,9 +349,15 @@ def update(
def _ensure_embeddings(self, documents: List[Document]) -> None:
"""Auto-embed any documents whose ``embedding`` field is ``None``."""
+ embedding_model = self._get_embedding_model()
Review Comment:
This now calls `self._get_embedding_model()` unconditionally before the
loop, only so `request_metric_group = self.metric_group` can be captured once.
On `origin/main` the resolution was lazy, inside the branch that actually needs
it:
```python
for doc in documents:
if doc.embedding is None:
doc.embedding = self._get_embedding_model().embed(doc.content)
```
The effect is that a store which never auto-embeds — mem0 embeds server-side
— now raises on `add`/`get`/`delete`. That's the 13 failures in
`test_mem0_vector_store.py`: `TypeError: No embedding model configured on this
vector store.` Could the resolution move back inside the `if doc.embedding is
None` branch, and the metric group be read only on the path that embeds?
##########
python/flink_agents/api/vector_stores/vector_store.py:
##########
@@ -344,9 +349,15 @@ def update(
def _ensure_embeddings(self, documents: List[Document]) -> None:
"""Auto-embed any documents whose ``embedding`` field is ``None``."""
+ embedding_model = self._get_embedding_model()
+ request_metric_group = self.metric_group
for doc in documents:
if doc.embedding is None:
- doc.embedding = self._get_embedding_model().embed(doc.content)
+ result = embedding_model.embed_with_usage(doc.content)
Review Comment:
The call moved from `embed(...)` to `embed_with_usage(...)`, and that
quietly changes the extension point. `BaseEmbeddingModelSetup.embed()`
delegates *to* `embed_with_usage()`, but `embed_with_usage`
(`embedding_model.py:157`) goes straight to `self._get_connection()` — so a
subclass that overrides `embed()` is no longer on the vector-store path. That's
the 7 `test_chroma_vector_store.py` failures: `TypeError: Expect
BaseEmbeddingModelConnection, but is str`, because `MockEmbeddingModel.embed()`
is bypassed and the mock's `connection` is the string `'mock'`. Beyond the test
mock, this is a public-API behavior change — is `embed()` still meant to be an
override point? If so, having the vector store go through it (with usage
surfaced some other way) would keep that contract intact.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
