Save embedding_model_name in the Chunk node - #575
Conversation
stellasia
changed the title
|
|
||
| - `AnthropicLLM` now supports structured output via the `response_format` argument, accepting a Pydantic model or an Anthropic `output_config` dict, alongside `OpenAILLM` and `VertexAILLM`. | ||
| - Chunk node metadata now contains `embedding_model_name` and `embedding_dimensions | ||
|
|
There was a problem hiding this comment.
Might be misreading this — the changelog says chunk metadata now holds embedding_model_name and embedding_dimensions, but I can only find embedding_model_name being set in components/embedder.py. Was embedding_dimensions meant to land too? If so it looks like a one-liner and would be genuinely handy; if not, just checking the wording before we ship it. (Also the entry's missing a closing backtick on embedding_dimensions.)
| """ | ||
| embedding = self._embedder.embed_query(text_chunk.text) | ||
| metadata = text_chunk.metadata if text_chunk.metadata else {} | ||
| metadata["embedding"] = embedding |
There was a problem hiding this comment.
Quick question — _async_embed_chunk sets embedding_model_name but _embed_chunk doesn't. I might be missing something, but run() looks like it always goes async and I couldn't find any callers of the sync method, so I'm guessing this doesn't matter in practice.
Curious whether the intent is to keep them in sync anyway, or the sync path is on its way out.
| embedding = await self._embedder.async_embed_query(text_chunk.text) | ||
| metadata = text_chunk.metadata if text_chunk.metadata else {} | ||
| metadata["embedding"] = embedding | ||
| metadata["embedding_model_name"] = getattr(self._embedder, "model", None) |
There was a problem hiding this comment.
Small one — since getattr(embedder, "model", None) defaults to None, a custom embedder with no model would store embedding_model_name: None rather than omit the key. Curious if that's deliberate (predictable schema) or if it'd be cleaner to skip the key when there's no model. Either's fine by me.
| super().__init__(rate_limit_handler) | ||
| self.model_id = model_id | ||
| self.dimensions = dimensions | ||
| super().__init__(model_id, dimensions, rate_limit_handler) |
There was a problem hiding this comment.
Am I right that Bedrock is staying on model_id for now, with model exposed as a deprecated read-only property? If so, the changelog's "subclasses now require a model argument" note doesn't quite cover it. Not sure whether keeping Bedrock on model_id is intentional or it's meant to move across like the others — whichever way you want to go, I'm happy to accommodate.
2 participants
Description
What
Save the embedding model name in the
Chunknode.How
model(+dimensions) attribute to allEmbedderclassesType of Change
Complexity
Complexity:
How Has This Been Tested?
Checklist
The following requirements should have been met (depending on the changes in the branch):