-
Notifications
You must be signed in to change notification settings - Fork 247
Save embedding_model_name in the Chunk node #575
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -83,6 +83,7 @@ async def _async_embed_chunk( | |
| 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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Small one — since |
||
| return TextChunk( | ||
| text=text_chunk.text, | ||
| index=text_chunk.index, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |
| import asyncio | ||
| import json | ||
| import os | ||
| import warnings | ||
| from typing import Any, Optional | ||
|
|
||
| from neo4j_graphrag.embeddings.base import Embedder | ||
|
|
@@ -82,15 +83,21 @@ def __init__( | |
| "Could not import boto3 python client. " | ||
| 'Please install it with `pip install "neo4j-graphrag[bedrock]"`.' | ||
| ) | ||
| super().__init__(rate_limit_handler) | ||
| self.model_id = model_id | ||
| self.dimensions = dimensions | ||
| super().__init__(model_id, dimensions, rate_limit_handler) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Am I right that Bedrock is staying on |
||
| self.normalize = normalize | ||
| client_kwargs: dict[str, Any] = {**kwargs} | ||
| if region_name: | ||
| client_kwargs["region_name"] = region_name | ||
| self.client = boto3.client("bedrock-runtime", **client_kwargs) | ||
|
|
||
| @property | ||
| def model_id(self) -> str | None: | ||
| warnings.warn( | ||
| "model_id is deprecated. Use model instead.", | ||
| DeprecationWarning, | ||
| ) | ||
| return self.model | ||
|
|
||
| def _invoke_embedding(self, text: str) -> list[float]: | ||
| """Invoke the Bedrock embedding model and return the embedding vector.""" | ||
| body = json.dumps( | ||
|
|
@@ -102,7 +109,7 @@ def _invoke_embedding(self, text: str) -> list[float]: | |
| ) | ||
| response = self.client.invoke_model( | ||
| body=body, | ||
| modelId=self.model_id, | ||
| modelId=self.model, | ||
| accept="application/json", | ||
| contentType="application/json", | ||
| ) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be misreading this — the changelog says chunk metadata now holds
embedding_model_nameandembedding_dimensions, but I can only findembedding_model_namebeing set incomponents/embedder.py. Wasembedding_dimensionsmeant 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 onembedding_dimensions.)