[BUG](embedding-functions): compare values in validate_config_update#7115
Open
adityasingh2400 wants to merge 1 commit into
Open
Conversation
Several builtin EFs rejected any config update whose new config contained a key like model_name, even when the value matched the existing one. That made overwrite_embedding_function() impossible to use to change other fields like api_base or dimensions, since get_config() always includes model_name in the new config it produces. The check now mirrors the existing pattern in HuggingFaceEmbeddingServer and ChromaCloudSpladeEmbeddingFunction: only raise when the new value differs from the old. Affects amazon_bedrock, cloudflare_workers_ai, chroma_cloud_qwen, cohere, google, huggingface, jina, mistral, morph, nomic, ollama, open_clip, openai, perplexity, together_ai, voyageai. Adds parametrized regression tests covering same-value updates (must not raise) and changed-value updates (must still raise).
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Several builtin embedding functions reject any config update whose new config contains a key such as
model_name, even when the value matches the existing one. The check insidevalidate_config_updateisif "model_name" in new_config:andoverwrite_embedding_functionpassesupdate_embedding_function.get_config()asnew_config, which always includesmodel_name. The result is that you cannot change any other persisted field, e.g.api_base,dimensions,organization_id,project_id, without also changing the model name, and the resulting error message claims the model name changed even when it did not.This PR changes the check to compare values against
old_config, matching the existing pattern inHuggingFaceEmbeddingServerandChromaCloudSpladeEmbeddingFunction. The same shape is applied to every immutability guard inamazon_bedrock,cloudflare_workers_ai,chroma_cloud_qwen,cohere,google(Generative AI, Genai, Palm, Vertex),huggingface,jina,mistral,morph,nomic,ollama,open_clip,openai,perplexity,together_ai, andvoyageai. Same-name updates now succeed, and changed values still raise the original error.Adds parametrized regression tests in
chromadb/test/ef/test_ef.pythat construct each affected EF with a dummy API key, then assert thatvalidate_config_update(cfg, cfg)does not raise and that swapping in a differentmodel_name(ormodel) still raises. EFs whose third-party SDK is not installed locally are skipped rather than failing.