feat: add client_settings to ChromaDocumentStore
#2651
Conversation
github-actions
bot
added
integration:chroma
type:documentation
|
Hey @SaraCalla, thanks for the contribution and sorry for the wait. Could you please resolve the conflicts in the meantime? |
anakin87
left a comment
anakin87
left a comment
There was a problem hiding this comment.
Thank you again. I left some comments.
Have you investigated how Settings interfere with other client parameters?
HttpClient(host: str = "localhost",
port: int = 8000,
settings: my_custom_settings)| method `create_collection`. If it contains the key `"hnsw:space"`, the value will take precedence over the | ||
| `distance_function` parameter above. | ||
| :param client_settings: a dictionary of Chroma Settings configuration options passed to | ||
| `chromadb.config.Settings`. These settings configure the underlying Chroma client behavior. |
There was a problem hiding this comment.
- Let's add a link to the supported settings. I haven't found docs, so we could reference this file: https://github.com/chroma-core/chroma/blob/main/chromadb/config.py
- Let's also add that specifying these settings may interfere with the standard client initialization, and it's intended for advanced customization
| if self._client_settings: | ||
| try: | ||
| client_kwargs["settings"] = Settings(**self._client_settings) | ||
| except Exception as e: |
There was a problem hiding this comment.
Let's catch the specific error we get - if possible
| ) | ||
| raise ValueError(error_message) | ||
|
|
||
| client_kwargs: dict[str, Any] = {} |
There was a problem hiding this comment.
nit: instead of a dictionary with a single key, we can define define a client_settings object with a None default value.
client_settings_obj = None
and later
client_settings_obj = Settings(**self._client_settings)
| Chroma's in-memory client uses a singleton pattern with an internal cache. | ||
| Once a client is created with certain settings, Chroma rejects creating another | ||
| with different settings in the same process. We clear the cache before and after | ||
| this test to avoid conflicts with other tests that use default settings. |
There was a problem hiding this comment.
Ah, this is a good catch, but I'd prefer to extract a fixture to do the same. Something like this (not tested)
@pytest.fixture
def clear_chroma_system_cache():
SharedSystemClient.clear_system_cache()
yield
SharedSystemClient.clear_system_cache() 2 participants
Related Issues
Proposed Changes:
Added
client_settingsparameter to__init__to allow users to configure the underlying Chroma client's Settings without accessing private attributes.How did you test it?
client_settingsinitializationNotes for the reviewer
I also took the liberty of fixing the async test class marking. Previously, running
hatch run test:unitwould fail because tests required a Chroma server instance. The entireTestDocumentStoreAsyncclass is now marked as@pytest.mark.integration.Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:.