-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[CHORE] Propogate error messages correctly to user #4235
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
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. |
1addfda
to
2802145
Compare
2112de5
to
e0c61ce
Compare
@@ -94,8 +94,12 @@ def wrapper(self: Any, *args: Any, **kwargs: Any) -> T: | |||
try: | |||
return func(self, *args, **kwargs) | |||
except Exception as e: | |||
# modify the error message |
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.
nit: this comment doesn't add much value, i can see the msg is modified already
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.
Please add a test case for validation before merging that checks the error message, we can use that to make error message expectations documented in code and also harder to change without care.
## Description of changes This PR adds tests for validation_context to ensure the errors raised match the ones given by the caller, in this case an embedding function. tests for #4235 ## 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 repository](https://github.com/chroma-core/docs)?*
Description of changes
This PR propogates the error message caught with a modified error message. It fixes this issue
#3688
previous error message:
the info is all there but the final output is confusing because of this line
raise type(e)(msg).with_traceback(e.__traceback__)
this line is trying to recreate the error with the new message, but does not track the rest of the arguments in the error. The simplest way to modify the error message is to recreate e.args with the new error message
new error
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 repository?