-
-
Notifications
You must be signed in to change notification settings - Fork 734
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: fix anthropic reasking #560
Conversation
Deploying instructor with
|
Latest commit: |
b4c64fa
|
Status: | ✅ Deploy successful! |
Preview URL: | https://fcaafdc9.instructor.pages.dev |
Branch Preview URL: | https://fix-anthropic-reask.instructor.pages.dev |
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 b4c64fa
- Looked at
271
lines of code in6
files - Took 1 minute and 36 seconds to review
More info
- Skipped
0
files when reviewing. - Skipped posting
4
additional comments because they didn't meet confidence threshold of85%
.
1. instructor/utils.py:125
:
- Assessed confidence :
0%
- Comment:
The functionmerge_consecutive_messages
is implemented correctly and used appropriately in other files. It merges consecutive messages from the same role into a single message, reducing redundancy and making the code cleaner. - Reasoning:
The functionmerge_consecutive_messages
inutils.py
is used in multiple places (process_response.py
,retry.py
). This function merges consecutive messages from the same role into a single message. This is a good practice as it reduces redundancy and makes the code cleaner.
2. instructor/function_calls.py:135
:
- Assessed confidence :
0%
- Comment:
The modification to the functionparse_anthropic_json
is appropriate. ImportingMessage
fromanthropic.types
and asserting thatcompletion
is an instance ofMessage
ensures that thecompletion
argument is of the expected type, reducing the likelihood of runtime errors. - Reasoning:
The functionparse_anthropic_json
infunction_calls.py
has been modified to importMessage
fromanthropic.types
and assert thatcompletion
is an instance ofMessage
. This is a good practice as it ensures that thecompletion
argument is of the expected type, reducing the likelihood of runtime errors.
3. instructor/process_response.py:339
:
- Assessed confidence :
0%
- Comment:
The modification to the functionhandle_response_model
is appropriate. Merging consecutive user messages into a single message when the mode isMode.ANTHROPIC_JSON
ensures that the messages array alternates between user and assistant roles, which is a requirement for this mode. - Reasoning:
The functionhandle_response_model
inprocess_response.py
has been modified to merge consecutive user messages into a single message when the mode isMode.ANTHROPIC_JSON
. This is a good practice as it ensures that the messages array alternates between user and assistant roles, which is a requirement for this mode.
4. instructor/retry.py:110
:
- Assessed confidence :
0%
- Comment:
The modification to the functionretry_sync
is appropriate. Merging consecutive messages into a single message after extendingkwargs['messages']
withreask_messages(response, mode, e)
ensures that the messages array alternates between user and assistant roles, which is a requirement for this mode. - Reasoning:
The functionretry_sync
inretry.py
has been modified to merge consecutive messages into a single message after extendingkwargs['messages']
withreask_messages(response, mode, e)
. This is a good practice as it ensures that the messages array alternates between user and assistant roles, which is a requirement for this mode.
Workflow ID: wflow_TU98OKSrOB9dCQ0B
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.
👍 Looks good to me!
- Performed an incremental review on abbfbcd
- Looked at
70
lines of code in3
files - Took 1 minute and 50 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/client_anthropic.py:25
:
- Assessed confidence :
100%
- Grade:
0%
- Comment:
The PR description mentions changes toutils.py
,process_response.py
, andretry.py
related to themerge_consecutive_messages
function. However, these changes are not visible in the diff. Please ensure these changes are included in the commit. - Reasoning:
The PR description mentions that a utility functionmerge_consecutive_messages
has been added toutils.py
and used inprocess_response.py
andretry.py
. However, the diff does not show any changes to these files. It's possible that the PR author forgot to include these changes in the commit. I need to comment on this discrepancy and ask the author to include these changes.
Workflow ID: wflow_1knsn5geWIYg71uN
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
#559
Summary:
This PR introduces a fix for anthropic reasking by adding a utility function to merge consecutive user messages, modifying the
parse_anthropic_json
function, updating tests, and modifying the import logic in__init__.py
.Key points:
merge_consecutive_messages
function inutils.py
to merge consecutive user messages.merge_consecutive_messages
inprocess_response.py
andretry.py
.parse_anthropic_json
infunction_calls.py
to assert completion is an instance ofMessage
.test_simple.py
to usefrom_anthropic
function and added a field validator toUser
class.merge_consecutive_messages
intest_utils.py
.__init__.py
to check foranthropic
andgroq
modules.Generated with ❤️ by ellipsis.dev