Skip to content

[BUG] Raise Error when can't deserialize configuration json from server #4471

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+13 −22
Open

[BUG] Raise Error when can't deserialize configuration json from server #4471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion chromadb/api/collection_configuration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,19 @@ def load_collection_configuration_from_json(
else:
try:
ef = known_embedding_functions[ef_config["name"]]
ef = ef.build_from_config(ef_config["config"]) # type: ignore
except KeyError:
raise ValueError(
f"Embedding function {ef_config['name']} not found. Add @register_embedding_function decorator to the class definition."
)
try:
ef = ef.build_from_config(ef_config["config"]) # type: ignore
except Exception as e:
warnings.warn(
f"Could not build embedding function {ef_config['name']} from config {ef_config['config']}: {e}",
UserWarning,
stacklevel=2,
)
ef = None
else:
ef = None

Expand Down
25 changes: 4 additions & 21 deletions chromadb/types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,13 @@
from uuid import UUID
from enum import Enum
from pydantic import BaseModel
import warnings

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[CriticalError]

The warnings module is used in this file now (see usage in chromadb/api/collection_configuration.py), but it is not imported. Add the import at the top of the file to prevent runtime errors.

from chromadb.api.configuration import (
ConfigurationInternal,
)
from chromadb.serde import BaseModelJSONSerializable
from chromadb.api.collection_configuration import (
CollectionConfiguration,
HNSWConfiguration,
SpannConfiguration,
collection_configuration_to_json,
load_collection_configuration_from_json,
)
Expand Down Expand Up @@ -149,15 +146,8 @@ def get_configuration(self) -> CollectionConfiguration:
try:
return load_collection_configuration_from_json(self.configuration_json)
except Exception as e:
warnings.warn(
f"Server does not respond with configuration_json. Please update server: {e}",
DeprecationWarning,
stacklevel=2,
)
return CollectionConfiguration(
hnsw=HNSWConfiguration(),
spann=SpannConfiguration(),
embedding_function=None,
raise ValueError(
Comment on lines 146 to +149
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[CriticalError]

Instead of returning a default configuration on deserialization error, now code raises ValueError. Ensure that all consumers of this method are prepared to handle this exception appropriately. If unhandled, it may lead to crashes during deserialization.

f"Could not deserialize configuration_json: {e}",
)

def set_configuration(self, configuration: CollectionConfiguration) -> None:
Expand All @@ -175,19 +165,12 @@ def get_model_fields(self) -> Dict[Any, Any]:
@override
def from_json(cls, json_map: Dict[str, Any]) -> Self:
"""Deserializes a Collection object from JSON"""
configuration: CollectionConfiguration = {
"hnsw": {},
"spann": {},
"embedding_function": None,
}
try:
configuration_json = json_map.get("configuration_json", None)
Comment on lines 168 to 169
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[DataTypeCheck]

Calls to load_collection_configuration_from_json may now receive None for configuration_json. Before calling the loader, consider explicitly validating the presence of configuration_json to provide a clearer error message.

configuration = load_collection_configuration_from_json(configuration_json)
except Exception as e:
warnings.warn(
f"Server does not respond with configuration_json. Please update server: {e}",
DeprecationWarning,
stacklevel=2,
raise ValueError(
f"Could not deserialize configuration_json: {e}",
)
return cls(
id=json_map["id"],
Expand Down
Loading