[ENH] Add validation when multiple embedding functions set on client #4507

jairad26 · 2025-05-08T22:29:45Z

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 if they are incompatible

Test plan

How are these changes tested?

Tests pass locally with pytest for python, yarn test for js, cargo test for rust

Documentation Changes

Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs section?

github-actions · 2025-05-08T22:29:54Z

jairad26 · 2025-05-08T22:30:04Z

[ENH] add Mistral embedding function, fix some naming mismatches across clients #4653
[ENH] Add validation when multiple embedding functions set on client #4507 👈 (View in Graphite)
[BUG] Raise Error when can't deserialize configuration json from server, lazily load ef on CollectionModel, warn on api_key #4471
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

propel-code-bot · 2025-05-08T22:33:21Z

Validation for Conflicting Embedding Functions on Collection Operations

This PR introduces validation logic on Python and TypeScript client APIs (sync, async, and JS) to ensure that if an embedding function is specified both via a direct function argument and within the collection configuration, a conflict is detected and an error is raised if they are incompatible. New helper functions have been added to encapsulate this logic for create_collection, get_collection, and get_or_create_collection. Extensive new tests verify correct validation for all combinations of specified, omitted, matching, and non-matching embedding functions.

Key Changes:
• Added validate_embedding_function_conflict_on_create and validate_embedding_function_conflict_on_get helpers in Python and a hasEmbeddingFunctionConflict utility in TypeScript.
• Refactored all Python client entrypoints to call these helpers during collection create/get/get_or_create operations.
• Added corresponding validation in the TypeScript Chroma client for the same scenarios.
• Extended testing coverage in chromadb/test/configurations/test_collection_configuration.py to ensure error handling on conflicts, including edge cases.
• Removed the unused InvalidConfigurationError class in favor of direct validation logic for embedding function conflicts.

Affected Areas:
• Python: chromadb/api/client.py
• Python: chromadb/api/async_client.py
• Python: chromadb/api/collection_configuration.py
• Python: tests/configurations/test_collection_configuration.py
• TS: clients/js/packages/chromadb-core/src/ChromaClient.ts
• TS: clients/js/packages/chromadb-core/src/CollectionConfiguration.ts

Potential Impact:

Functionality: Prevents ambiguous embedding function assignment and raises explicit errors if two incompatible embedding functions are provided during collection creation or retrieval. This reduces potential runtime errors or silent misconfigurations.

Performance: Negligible; added validation logic is straightforward and only impacts control flow during configuration setup.

Security: No new risks introduced.

Scalability: No significant impact.

Review Focus:
• Correctness and consistency of conflict-check logic cross-language (Python and TS).
• Review for redundant or duplicate code, especially in JS and Python validation helpers.
• Edge cases where one or both embedding functions are 'default' or None/null.
• Robustness of tests, particularly around negative (error) cases.

Testing Needed

• Run all existing and new tests (Python via pytest, JS via yarn test) to validate error handling in all configuration scenarios.
• Verify explicit errors are raised when two different embedding functions are provided in both parameter and configuration.
• Test both matching and mismatching embedding functions, as well as default/fallback behavior.

Code Quality Assessment

clients/js/packages/chromadb-core/src/ChromaClient.ts: Logic is clearly ported; equality checks for JS objects should use deep comparison for future robustness.

clients/js/packages/chromadb-core/src/CollectionConfiguration.ts: Helper function provided, but uses reference equality for object configs, which may need improvement (suggest deep-equals).

chromadb/api/client.py: Encapsulated conflict logic; followup refactoring may further DRY up entrypoints.

chromadb/api/async_client.py: Validation encapsulated in helpers, code is clear.

chromadb/api/collection_configuration.py: Adds clear helpers, docstrings provided, respects error handling.

chromadb/test/configurations/test_collection_configuration.py: Extensive and systematic new tests ensure correctness and prevent regressions.

Best Practices

Validation:
• Helper functions encapsulate repeated logic
• Negative and edge case tests are comprehensive

Error Handling:
• Explicit ValueError and Error thrown on conflict detection

Potential Issues

• JS (TS) config comparison uses object reference (===) rather than deep value comparison for embedding function configs-could produce false negatives on functionally identical but separate objects.
• Conflict checking logic is duplicated in some places; further DRYing possible.
• If new embedding function types are introduced, helpers may need updates.
• Some reviewers suggested further encapsulating config comparison/deep equality.

This summary was automatically generated by @propel-code-bot

propel-code-bot · 2025-05-08T22:59:09Z

clients/js/packages/chromadb-core/src/ChromaClient.ts

+        efConfig.name !== embeddingFunction.name ||
+        efConfig !== collConfigEfConfig


[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.

Suggested change

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.

propel-code-bot · 2025-05-08T23:41:14Z

clients/js/packages/chromadb-core/src/ChromaClient.ts

+        embeddingFunction.name !== configuration.embedding_function.name ||
+        efConfig !== collConfigEfConfig


[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.

Suggested change

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.

propel-code-bot · 2025-05-08T23:41:15Z

chromadb/api/async_client.py

        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."
+                )
+


[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.

HammadB · 2025-05-30T02:07:46Z

chromadb/api/async_client.py

+                    "Multiple embedding functions provided. Please provide only one."
+                )
+
+        # If ef provided in function params and collection config is None, set the collection config to the function params


this comment is wrong

HammadB · 2025-05-30T02:08:08Z

chromadb/api/async_client.py

+
+        # if ef has been loaded from the collection config, and a new ef is provided, check if they are the same
+        # if not, raise an error
+        if (


Can we encapsulate this logic?

HammadB · 2025-05-30T02:08:35Z

chromadb/api/async_client.py

+        configuration_ef = configuration.get("embedding_function")
+
+        # if ef has been loaded from the collection config, and a new ef is provided, check if they are the same
+        # if not, raise an error


can we encapsulate this logic?

HammadB

Encapsulate the shared logic

HammadB

Please move the error into the function

HammadB · 2025-05-30T17:15:16Z

chromadb/api/async_client.py

+
+        configuration_ef = configuration.get("embedding_function")
+
+        has_conflict, error_message = has_embedding_function_conflict_on_create(


Why not just let this raise? why catch the bool and raise?

HammadB · 2025-05-30T17:15:47Z

chromadb/api/async_client.py

+        )
+
+        if has_conflict:
+            raise ValueError(


Or set it to the existing one?

…hroma-core#4507) ## 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 if they are incompatible ## Test plan _How are these changes tested?_ - [ ] Tests pass locally with `pytest` for python, `yarn test` for js, `cargo test` for rust ## Documentation Changes _Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the [docs section](https://github.com/chroma-core/chroma/tree/main/docs/docs.trychroma.com)?_

jairad26 marked this pull request as ready for review May 8, 2025 22:32

jairad26 marked this pull request as draft May 8, 2025 22:37

jairad26 force-pushed the jai/collection-config-nits branch from 7ea7b69 to 872c86d Compare May 8, 2025 22:55

jairad26 requested a review from HammadB May 8, 2025 22:56

jairad26 marked this pull request as ready for review May 8, 2025 22:56

propel-code-bot bot reviewed May 8, 2025

View reviewed changes

jairad26 force-pushed the jai/collection-config-nits branch from 872c86d to 79619c2 Compare May 8, 2025 23:39

propel-code-bot bot reviewed May 8, 2025

View reviewed changes

HammadB reviewed May 30, 2025

View reviewed changes

HammadB requested changes May 30, 2025

View reviewed changes

jairad26 changed the base branch from main to graphite-base/4507 May 30, 2025 02:38

jairad26 force-pushed the jai/collection-config-nits branch from 79619c2 to db8ef03 Compare May 30, 2025 02:38

jairad26 changed the base branch from graphite-base/4507 to jai/fix-list-collections May 30, 2025 02:38

jairad26 mentioned this pull request May 30, 2025

[BUG] Raise Error when can't deserialize configuration json from server, lazily load ef on CollectionModel, warn on api_key #4471

Merged

1 task

jairad26 force-pushed the jai/collection-config-nits branch from db8ef03 to a462980 Compare May 30, 2025 02:53

jairad26 force-pushed the jai/fix-list-collections branch from e8ffaa9 to 1aff970 Compare May 30, 2025 02:53

jairad26 force-pushed the jai/collection-config-nits branch from a462980 to b64e7f3 Compare May 30, 2025 03:03

jairad26 force-pushed the jai/fix-list-collections branch from 1aff970 to 457a780 Compare May 30, 2025 03:03

jairad26 changed the base branch from jai/fix-list-collections to graphite-base/4507 May 30, 2025 04:09

jairad26 force-pushed the graphite-base/4507 branch from 457a780 to 5e58b94 Compare May 30, 2025 04:09

jairad26 force-pushed the jai/collection-config-nits branch from b64e7f3 to 7ae1379 Compare May 30, 2025 04:09

jairad26 changed the base branch from graphite-base/4507 to main May 30, 2025 04:09

jairad26 force-pushed the jai/collection-config-nits branch 2 times, most recently from 13b451c to a2d3143 Compare May 30, 2025 04:12

jairad26 force-pushed the jai/collection-config-nits branch 6 times, most recently from 7f58958 to 2f350ad Compare May 30, 2025 07:06

jairad26 mentioned this pull request May 30, 2025

[ENH] add Mistral embedding function, fix some naming mismatches across clients #4653

Merged

1 task

codetheweb approved these changes May 30, 2025

View reviewed changes

HammadB approved these changes May 30, 2025

View reviewed changes

[ENH] Add validation when multiple embedding functions set on client

bf4d47c

jairad26 force-pushed the jai/collection-config-nits branch from 2f350ad to bf4d47c Compare May 30, 2025 17:36

jairad26 merged commit b7060c9 into main May 30, 2025
72 checks passed

		efConfig.name !== embeddingFunction.name \|\|
		efConfig !== collConfigEfConfig

		embeddingFunction.name !== configuration.embedding_function.name \|\|
		efConfig !== collConfigEfConfig


		configuration_ef = configuration.get("embedding_function")

		has_conflict, error_message = has_embedding_function_conflict_on_create(

[ENH] Add validation when multiple embedding functions set on client #4507

[ENH] Add validation when multiple embedding functions set on client #4507

Uh oh!

Conversation

jairad26 commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Test plan

Documentation Changes

Uh oh!

github-actions bot commented May 8, 2025

Reviewer Checklist

Testing, Bugs, Errors, Logs, Documentation

System Compatibility

Quality

Uh oh!

jairad26 commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

propel-code-bot bot commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

propel-code-bot bot May 8, 2025

Choose a reason for hiding this comment

Uh oh!

propel-code-bot bot May 8, 2025

Choose a reason for hiding this comment

Uh oh!

propel-code-bot bot May 8, 2025

Choose a reason for hiding this comment

Uh oh!

HammadB May 30, 2025

Choose a reason for hiding this comment

Uh oh!

HammadB May 30, 2025

Choose a reason for hiding this comment

Uh oh!

HammadB May 30, 2025

Choose a reason for hiding this comment

Uh oh!

HammadB left a comment

Choose a reason for hiding this comment

Uh oh!

HammadB left a comment

Choose a reason for hiding this comment

Uh oh!

HammadB May 30, 2025

Choose a reason for hiding this comment

Uh oh!

HammadB May 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jairad26 commented May 8, 2025 •

edited

Loading

jairad26 commented May 8, 2025 •

edited

Loading

propel-code-bot bot commented May 8, 2025 •

edited

Loading