-
-
Notifications
You must be signed in to change notification settings - Fork 730
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
Refactor anthropic message format to support tool messages & Allows Anthropic Bedrock clients #579
Refactor anthropic message format to support tool messages & Allows Anthropic Bedrock clients #579
Conversation
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.
👍 Looks good to me!
- Reviewed the entire pull request up to 9705a6f
- Looked at
117
lines of code in3
files - Took 46 seconds to review
More info
- Skipped
0
files when reviewing. - Skipped posting
1
additional comments because they didn't meet confidence threshold of85%
.
1. instructor/retry.py:45
:
- Assessed confidence :
50%
- Comment:
The error message in the assertion could be more descriptive. Instead of 'Tool use ID not found in the response', it could be 'Expected tool use ID in the response but none found'.
assert tool_use_id is not None, "Expected tool use ID in the response but none found"
- Reasoning:
The changes in the PR seem to be logically correct and follow the best practices. The code is not overly complicated and the variable names are descriptive. The function naming follows a consistent pattern. The changes in the library code are reflected in the tests. The assertions have well-formatted error messages. However, I noticed that the error message in the assertion at line 45 in retry.py could be more descriptive. Instead of 'Tool use ID not found in the response', it could be 'Expected tool use ID in the response but none found'.
Workflow ID: wflow_rMlGfFzdJ9S22wtA
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
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.
- skip anthropic bedrock tests
- add some logic to avoid doing the merge for openai keys
is this good to merge? |
Yeah, I was waiting for it to be merged these days 😂 |
Instead of concatenating strings with "\n\n", we use the following format to merge consecutive Anthropic messages of the same roles:
Which also allows other message types like image objects, tool invocation (for assistant), tool results, etc. As a comparsion, this version of tool support is able to pass the anthropic tests which currently fails.
Summary:
This PR refactors the anthropic message format to support tool messages, updates the
reask_messages
andmerge_consecutive_messages
functions, and modifies the corresponding tests.Key points:
reask_messages
function in/instructor/retry.py
to include the original response and format error messages differently.merge_consecutive_messages
function in/instructor/utils.py
to handle the new message format./tests/test_utils.py
to reflect these changes.Generated with ❤️ by ellipsis.dev