-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix: unchecked key lookup in Python dict #4517
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
|
raise ValueError( | ||
f"Trying to instantiate configuration of type {cls.__name__} from JSON with type {json_map.get('_type')}" |
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 error message improvement is good, but there's a potential inefficiency in accessing json_map.get('_type')
twice. Since you're already checking if '_type'
is in json_map
, you can access it directly in the error message instead of using get
again.
raise ValueError( | |
f"Trying to instantiate configuration of type {cls.__name__} from JSON with type {json_map.get('_type')}" | |
raise ValueError( | |
f"Trying to instantiate configuration of type {cls.__name__} from JSON with type {json_map['_type']}" | |
) |
⚡ 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.
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.
The issue is that json_map might not have '_type'.
This PR improves error handling in the This summary was automatically generated by @propel-code-bot |
Hi, Chroma Cloud does not support 0.6.3 of the python client. You can fix this issue by upgrading to >=1.0.3 |
Description of changes
This pull request refines error handling in the
from_json
method of thechromadb/api/configuration.py
file.Improvements to error handling:
from_json
method to raise a specific error message when the_type
key is missing in the inputjson_map
. This ensures clearer error reporting for debugging.Context
When I was playing around with the Chroma Cloud, I received the following error message when trying to create collection:
The error arises from trying to grab the '_type' key from
json_map
via [] syntax without first checking if the key exists, which leads to the Python KeyError. It's unfortunate that the [] syntax is used instead of the .get() syntax which is safer because it returnsNone
by default.The line above actually uses the correct .get() syntax:
and it's apparent that the '_type' key might not exist on
json_map
, so accessingjson_map['_type']
in the next line is dangerous.My guess is that Chroma Cloud returns a config object when creating a collection, and sometimes that config schema can get out of sync with the client's version. I'm running
chromadb==0.6.3
locally.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?