diff --git a/python/semantic_kernel/connectors/chroma.py b/python/semantic_kernel/connectors/chroma.py index e5fa0ef61ada..b5c821b40b25 100644 --- a/python/semantic_kernel/connectors/chroma.py +++ b/python/semantic_kernel/connectors/chroma.py @@ -11,6 +11,7 @@ from chromadb.api.collection_configuration import CreateCollectionConfiguration, CreateHNSWConfiguration from chromadb.api.types import EmbeddingFunction, Space from chromadb.config import Settings +from chromadb.errors import NotFoundError from semantic_kernel.connectors.ai.embedding_generator_base import EmbeddingGeneratorBase from semantic_kernel.data.vector import ( @@ -128,12 +129,22 @@ def _get_collection(self) -> Collection: @override async def collection_exists(self, **kwargs: Any) -> bool: - """Check if the collection exists.""" + """Check if the collection exists. + + Returns False only when Chroma says the collection is not there. Anything else - + an unreachable server, a rejected request - is raised as a + VectorStoreOperationException, because the caller cannot tell it apart from an + absent collection otherwise. + """ try: self.client.get_collection(name=self.collection_name, embedding_function=self.embedding_func) return True - except Exception: + except NotFoundError: return False + except Exception as e: + raise VectorStoreOperationException( + f"Failed to check whether collection {self.collection_name} exists with error: {e}" + ) from e @override async def ensure_collection_exists(self, **kwargs: Any) -> None: @@ -184,7 +195,7 @@ async def ensure_collection_deleted(self, **kwargs: Any) -> None: """Delete the collection.""" try: self.client.delete_collection(name=self.collection_name) - except ValueError: + except NotFoundError: logger.info(f"Collection {self.collection_name} could not be deleted because it doesn't exist.") except Exception as e: raise VectorStoreOperationException( diff --git a/python/tests/unit/connectors/memory/test_chroma.py b/python/tests/unit/connectors/memory/test_chroma.py index 67b4e5471c27..1d944b8abb0a 100644 --- a/python/tests/unit/connectors/memory/test_chroma.py +++ b/python/tests/unit/connectors/memory/test_chroma.py @@ -5,8 +5,15 @@ import pytest from chromadb.api import ClientAPI from chromadb.api.models.Collection import Collection +from chromadb.errors import NotFoundError from semantic_kernel.connectors.chroma import ChromaCollection, ChromaStore +from semantic_kernel.exceptions.vector_store_exceptions import VectorStoreOperationException + + +def _not_found() -> Exception: + """The error Chroma raises for a missing collection.""" + return NotFoundError("Collection test_collection does not exist.") @pytest.fixture @@ -115,6 +122,60 @@ async def test_chroma_collection_search(chroma_collection, mock_client, include_ assert res.score == 0.1 +# -- what a Chroma failure means, and what "not there" means -- +# +# chromadb 1.x raises errors.NotFoundError for a missing collection, which is not a +# ValueError (this connector only imports on chromadb 1.x: it needs +# chromadb.api.collection_configuration). collection_exists caught bare Exception, so every failure +# read as "absent"; ensure_collection_deleted caught only ValueError, so on a modern +# chromadb a genuinely absent collection raised instead of being tolerated. + + +async def test_collection_exists_is_false_when_chroma_says_not_found(chroma_collection, mock_client): + mock_client.get_collection.side_effect = _not_found() + + assert await chroma_collection.collection_exists() is False + + +async def test_collection_exists_raises_when_the_server_cannot_be_reached(chroma_collection, mock_client): + """False means the collection is not there. A refused connection must not say that.""" + mock_client.get_collection.side_effect = ConnectionError("connection refused") + + with pytest.raises(VectorStoreOperationException) as raised: + await chroma_collection.collection_exists() + assert isinstance(raised.value.__cause__, ConnectionError) + + +async def test_ensure_collection_deleted_tolerates_an_absent_collection(chroma_collection, mock_client): + """Deleting something that is not there is the case this method already meant to allow.""" + mock_client.delete_collection.side_effect = _not_found() + + await chroma_collection.ensure_collection_deleted() + + +async def test_ensure_collection_deleted_still_raises_on_a_real_failure(chroma_collection, mock_client): + mock_client.delete_collection.side_effect = ConnectionError("connection refused") + + with pytest.raises(VectorStoreOperationException): + await chroma_collection.ensure_collection_deleted() + + +async def test_collection_exists_raises_on_an_unrelated_value_error(chroma_collection, mock_client): + """Where Chroma has a dedicated NotFoundError, a ValueError is not 'absent'.""" + mock_client.get_collection.side_effect = ValueError("some other problem") + + with pytest.raises(VectorStoreOperationException): + await chroma_collection.collection_exists() + + +async def test_ensure_collection_deleted_does_not_swallow_an_unrelated_value_error(chroma_collection, mock_client): + """The more dangerous half: a swallowed ValueError here would silently skip a delete.""" + mock_client.delete_collection.side_effect = ValueError("some other problem") + + with pytest.raises(VectorStoreOperationException): + await chroma_collection.ensure_collection_deleted() + + def test_chroma_collection_build_filter_keeps_apostrophes(chroma_collection): """Chroma filters are dicts, not SQL, so the value must reach Chroma unchanged.""" assert chroma_collection._build_filter('lambda x: x.content == "O\'Brien"') == {"content": "O'Brien"}