-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ENH] Add validation when multiple embedding functions set on client #4507
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?
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Add Validation for Multiple Embedding Function Configurations This PR implements validation across Python and JavaScript clients to prevent conflicts when embedding functions are provided in multiple places. It adds checks to create, get, and get_or_create collection methods to ensure that when embedding functions are specified both in the function parameters and configuration object, they are either identical or an error is raised. Key Changes: Affected Areas: Potential Impact: Functionality: Users who previously provided embedding functions in multiple places may now receive errors, forcing them to use a single, consistent embedding function specification. Performance: Minimal impact - only adds small validation checks during collection operations Security: No security implications Scalability: No scalability implications Review Focus: Testing Needed• Test using matching embedding functions in both places (should succeed) Code Quality Assessmentchromadb/api/client.py: Good validation logic with clear error messaging chromadb/api/async_client.py: Maintains consistent validation approach with sync client clients/js/packages/chromadb-core/src/ChromaClient.ts: Potential issue with object equality checking using !== instead of deep comparison Best PracticesError Handling: Testing: Potential Issues• JavaScript equality check uses !== which may not correctly identify equal objects with different references This summary was automatically generated by @propel-code-bot |
7ea7b69
to
872c86d
Compare
efConfig.name !== embeddingFunction.name || | ||
efConfig !== collConfigEfConfig |
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.
[DataTypeCheck]
The check for equality of embedding function configurations in the TypeScript client uses efConfig !== collConfigEfConfig
(and similarly for .name
). This will almost always be true for different objects with identical contents. Use a deep comparison method to accurately compare the configs, such as JSON.stringify()
or a dedicated deep-equality utility.
efConfig.name !== embeddingFunction.name || | |
efConfig !== collConfigEfConfig | |
if ( | |
efConfig.name !== embeddingFunction.name || | |
JSON.stringify(efConfig) !== JSON.stringify(collConfigEfConfig) | |
) { | |
throw new Error( | |
"Multiple embedding functions provided. Please provide only one.", | |
); | |
} |
⚡ Committable suggestion
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
872c86d
to
79619c2
Compare
embeddingFunction.name !== configuration.embedding_function.name || | ||
efConfig !== collConfigEfConfig |
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.
[DataTypeCheck]
The equality check between efConfig and collConfigEfConfig on lines 246 and 333 will not work as expected for objects in JavaScript, since !==
only checks reference equality. Use a deep comparison function (like lodash's isEqual
) or JSON serialization for robust configuration checks.
embeddingFunction.name !== configuration.embedding_function.name || | |
efConfig !== collConfigEfConfig | |
if ( | |
embeddingFunction.name !== configuration.embedding_function.name || | |
JSON.stringify(efConfig) !== JSON.stringify(collConfigEfConfig) | |
) { | |
throw new Error( | |
"Multiple embedding functions provided. Please provide only one.", | |
); | |
} |
Repeat this update for all similar blocks checking configuration equality.
⚡ Committable suggestion
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
if ( | ||
embedding_function is not None | ||
and configuration.get("embedding_function") is None | ||
and embedding_function.name() != "default" | ||
and configuration_ef is not None | ||
): | ||
ef_configuration = embedding_function.get_config() | ||
coll_config_ef_configuration = configuration_ef.get_config() | ||
if ( | ||
embedding_function.name() != configuration_ef.name() | ||
or ef_configuration != coll_config_ef_configuration | ||
): | ||
raise ValueError( | ||
"Multiple embedding functions provided. Please provide only one." | ||
) | ||
|
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.
[BestPractice]
The logic for validating embedding function conflicts (checking names and configurations if both an embedding_function
parameter and an embedding function from the configuration
dictionary are present and non-default) is duplicated in create_collection
(here, lines 193-207), get_collection
(lines 245-259), and get_or_create_collection
(lines 285-299). This pattern also exists in the synchronous chromadb/api/client.py
. Consider extracting this validation into a private helper method within each class to improve maintainability and reduce redundancy. The helper could handle the comparison of names and configurations and raise the appropriate ValueError
.
Description of changes
This PR adds validation on create, get, and get_or_create collection to ensure that if embedding functions are provided in both places, it raises an error for the user
Test plan
How are these changes tested?
pytest
for python,yarn test
for js,cargo test
for rustDocumentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs section?