mirror of
https://github.com/tiennm99/DocsGPT.git
synced 2026-10-20 02:16:39 +00:00
fix: embeddings registry edge cases and chunk-size accounting
Follow-up to the embeddings work, from a review pass over the branch. - Route the OpenAI/Azure key handling through the model registry instead of matching the canonical name literally, so the `text-embedding-ada-002` alias the registry now accepts also reaches the Azure deployment name rather than failing every embed with DeploymentNotFound. - Fall back to a default width where the embeddings model reports no dimension. A model outside the registry returns None rather than no attribute, so `getattr` with a default did not catch it and the width reached the DDL as `vector(None)` / `list_size=None`. - Point HF_HUB_CACHE at the prefetch directory. Chunking loads the tokenizer through `tokenizers`, which reads the hub cache, so a fresh container fetched over the network on first ingest and an offline one silently fell back to cl100k. - Charge a token that collapses a long unbroken run by its character span. WordPiece emits one [UNK] for any word over its character limit, which made base64 and minified content count as near-zero tokens, so nothing split it and oversized chunks reached the embedding server. - Preserve chunk ids and honour --batch-size when rebuilding a FAISS index. Fresh uuids orphaned GraphRAG's graph_node_chunks rows, and the whole index went out in a single embed call on remote servers. - Document that granite runs an int8-quantised graph, and scope the SentenceTransformer parity claim to mpnet's fp32 graph, which is where it was measured. - Correct the embeddings docs: a matching dimension is not a matching model, so a same-width swap raises nothing and silently degrades retrieval.
This commit is contained in:
1 parent
869d87452b
commit
25e07f5cee
15 files changed
+296
-20
No files matched your search
@@ -591,6 +591,49 @@ class TestGetEmbeddingsResolver:
|
||||
"openai_text-embedding-ada-002", model="embed-deploy"
|
||||
)
|
||||
|
||||
@patch("application.vectorstore.base.settings")
|
||||
@patch("application.vectorstore.base.EmbeddingsSingleton.get_instance")
|
||||
def test_openai_alias_also_reaches_the_azure_deployment(
|
||||
self, mock_get_instance, mock_settings
|
||||
):
|
||||
"""The registry accepts the bare alias, so the key handling must too.
|
||||
|
||||
Matching on the canonical string alone sent the alias down the generic
|
||||
branch, where the deployment name is never passed and Azure answers
|
||||
every embed with DeploymentNotFound.
|
||||
"""
|
||||
mock_settings.EMBEDDINGS_BASE_URL = None
|
||||
mock_settings.OPENAI_API_BASE = "https://azure.openai.com"
|
||||
mock_settings.OPENAI_API_VERSION = "2023-05-15"
|
||||
mock_settings.AZURE_DEPLOYMENT_NAME = "deploy"
|
||||
mock_settings.AZURE_EMBEDDINGS_DEPLOYMENT_NAME = "embed-deploy"
|
||||
mock_settings.EMBEDDINGS_NAME = "text-embedding-ada-002"
|
||||
mock_settings.EMBEDDINGS_KEY = "sk-key"
|
||||
|
||||
get_embeddings()
|
||||
|
||||
mock_get_instance.assert_called_once_with(
|
||||
"text-embedding-ada-002", model="embed-deploy"
|
||||
)
|
||||
|
||||
@patch("application.vectorstore.base.settings")
|
||||
@patch("application.vectorstore.base.EmbeddingsSingleton.get_instance")
|
||||
def test_openai_name_is_matched_case_insensitively(
|
||||
self, mock_get_instance, mock_settings
|
||||
):
|
||||
mock_settings.EMBEDDINGS_BASE_URL = None
|
||||
mock_settings.OPENAI_API_BASE = None
|
||||
mock_settings.OPENAI_API_VERSION = None
|
||||
mock_settings.AZURE_DEPLOYMENT_NAME = None
|
||||
mock_settings.EMBEDDINGS_NAME = "OpenAI_Text-Embedding-Ada-002"
|
||||
mock_settings.EMBEDDINGS_KEY = "sk-from-settings"
|
||||
|
||||
get_embeddings()
|
||||
|
||||
mock_get_instance.assert_called_once_with(
|
||||
"OpenAI_Text-Embedding-Ada-002", openai_api_key="sk-from-settings"
|
||||
)
|
||||
|
||||
@patch("application.vectorstore.base.settings")
|
||||
@patch("application.vectorstore.base.EmbeddingsSingleton.get_instance")
|
||||
def test_explicit_arguments_win_over_settings(
|
||||
|
||||
@@ -5,11 +5,13 @@ than asserting that calls were forwarded to a mock.
|
||||
"""
|
||||
|
||||
import json
|
||||
from unittest.mock import Mock, patch
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import MagicMock, Mock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from application.storage.local import LocalStorage
|
||||
from application.vectorstore.faiss import FaissStore
|
||||
|
||||
|
||||
class _FakeEmbeddings:
|
||||
@@ -243,3 +245,45 @@ class TestGetVectorstore:
|
||||
|
||||
with pytest.raises(ValueError, match="Invalid source_id path"):
|
||||
get_vectorstore(bad)
|
||||
|
||||
|
||||
class TestBuildFromDocumentsBatching:
|
||||
"""The full rebuild path: honour caller ids and embed in batches."""
|
||||
|
||||
def _docs(self, n):
|
||||
return [
|
||||
SimpleNamespace(page_content=f"chunk {i}", metadata={"i": i})
|
||||
for i in range(n)
|
||||
]
|
||||
|
||||
def _store(self, embed):
|
||||
store = FaissStore.__new__(FaissStore)
|
||||
store.embeddings = MagicMock()
|
||||
store.embeddings.embed_documents.side_effect = embed
|
||||
store.documents = {}
|
||||
store.index_to_docstore_id = {}
|
||||
store.index = None
|
||||
return store
|
||||
|
||||
def test_supplied_ids_are_used(self):
|
||||
store = self._store(lambda texts: [[float(len(texts))] * 2 for _ in texts])
|
||||
store._build_from_documents(self._docs(3), ids=["a", "b", "c"])
|
||||
assert list(store.documents) == ["a", "b", "c"]
|
||||
|
||||
def test_embedding_is_split_into_batches(self):
|
||||
sizes = []
|
||||
|
||||
def embed(texts):
|
||||
sizes.append(len(texts))
|
||||
return [[1.0, 2.0] for _ in texts]
|
||||
|
||||
store = self._store(embed)
|
||||
store._build_from_documents(self._docs(5), batch_size=2)
|
||||
assert sizes == [2, 2, 1]
|
||||
assert len(store.index_to_docstore_id) == 5
|
||||
|
||||
def test_defaults_are_unchanged(self):
|
||||
store = self._store(lambda texts: [[1.0, 2.0] for _ in texts])
|
||||
store._build_from_documents(self._docs(3))
|
||||
assert len(store.documents) == 3
|
||||
assert all(len(k) == 36 for k in store.documents), "uuid4 ids by default"
|
||||
Reference in new issue
Block a user