-
-
Notifications
You must be signed in to change notification settings - Fork 691
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
Feat: Patch Anthropic system message #1112
Conversation
- Enable OpenAI-style system messages with arrays for Anthropic - Allow prompt caching at the system message level for Anthropic - Remove Databricks assertion as they now mimic OpenAI broadly - Enable multimodality without response model and `autodetect_images=False`
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 everything up to 0b6d0d1 in 23 seconds
More details
- Looked at
531
lines of code in5
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. instructor/process_response.py:335
- Draft comment:
The logic for handling system messages is duplicated inhandle_anthropic_tools
andhandle_anthropic_json
. Consider refactoring to avoid duplication. - Reason this comment was not posted:
Confidence changes required:50%
Ininstructor/process_response.py
, thehandle_anthropic_tools
andhandle_anthropic_json
functions both useextract_system_messages
andcombine_system_messages
. The logic for handling system messages is duplicated. Consider refactoring to avoid code duplication.
2. instructor/process_response.py:352
- Draft comment:
The logic for handling system messages is duplicated inhandle_anthropic_tools
andhandle_anthropic_json
. Consider refactoring to avoid duplication. - Reason this comment was not posted:
Confidence changes required:50%
Ininstructor/process_response.py
, thehandle_anthropic_tools
andhandle_anthropic_json
functions both useextract_system_messages
andcombine_system_messages
. The logic for handling system messages is duplicated. Consider refactoring to avoid code duplication.
3. tests/llm/test_anthropic/test_system.py:37
- Draft comment:
Assertions should include error messages for better debugging. This applies to all assertions in this file and intests/test_utils.py
. - Reason this comment was not posted:
Confidence changes required:80%
The assertion statements in the test files lack error messages, which is against the rule that assertions should always have an error message.
4. tests/llm/test_anthropic/test_system.py:1
- Draft comment:
Ensure this new test file is added to the mkdocs.yml for documentation purposes. - Reason this comment was not posted:
Confidence changes required:80%
The new test file should be added to the mkdocs.yml for documentation purposes.
Workflow ID: wflow_z14QsaO8RsmT88IQ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
- Remove excess comments
- ruff
have you tested this all locally? |
- Add standard Anthropic-style system test
yes I tried to make sure that this isn't a breaking change for standard system messages by merging them if they exist. tests are passing locally |
Forked this in #1130 and made changes there. |
@ivanleomk thanks for the cleanup! |
autodetect_images=False
Important
Enhance Anthropic system message handling with OpenAI-style support, prompt caching, and multimodality adjustments, while removing Databricks-specific assertion.
process_response.py
.process_response.py
.client.py
as they now mimic OpenAI broadly.autodetect_images=False
inprocess_response.py
.combine_system_messages()
andextract_system_messages()
inutils.py
to handle system messages.test_system.py
to test Anthropic system message handling.combine_system_messages()
andextract_system_messages()
intest_utils.py
.This description was created by for 0b6d0d1. It will automatically update as commits are pushed.