-
-
Notifications
You must be signed in to change notification settings - Fork 547
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
Added support for response_model = None in cohere #867
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 everything up to 157c68e in 25.955599 seconds
More details
- Looked at
42
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. instructor/process_response.py:534
- Draft comment:
The removal ofstrict
fromnew_kwargs
is done without checking if it exists. This is generally safe but could be explicitly checked to avoid potential confusion or errors in future modifications.
new_kwargs.pop("strict", None)
- Reason this comment was not posted:
Confidence changes required:33%
The PR adds support for handlingresponse_model = None
specifically forMode.COHERE_JSON_SCHEMA
andMode.COHERE_TOOLS
. The code checks ifresponse_model
isNone
and if the mode is eitherCOHERE_JSON_SCHEMA
orCOHERE_TOOLS
, then processes the messages to format them according to Cohere's requirements. The code seems to handle the chat history and the last message correctly, and it also handles the case wheremodel_name
is provided instead ofmodel
. However, the code removes thestrict
key fromnew_kwargs
without checking if it exists, which is generally safe but could be explicitly checked to avoid confusion.
Workflow ID: wflow_FBvxOAQstYXab6jU
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Deploying instructor with Cloudflare Pages
|
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! Incremental review on 0525df4 in 40.51001 seconds
More details
- Looked at
105
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. tests/llm/test_cohere/test_none_response.py:1
- Draft comment:
The new test filetest_none_response.py
should be added tomkdocs.yml
to ensure it is included in the documentation. This is important for maintaining the discoverability and documentation of new features and tests. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 85%.
Workflow ID: wflow_hfMGGHxBYBJ8QBAm
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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! Incremental review on cc84040 in 58.238597 seconds
More details
- Looked at
11
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. tests/llm/test_cohere/test_none_response.py:1
- Draft comment:
The assertionassert response.text
might not be appropriate ifresponse_model=None
alters the expected attributes of the response object. Consider adjusting the test to reflect the expected output whenresponse_model
is set toNone
. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 85%.
Workflow ID: wflow_6A8qeyzOZjPKHjyO
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
cc84040
to
4d063c8
Compare
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! Incremental review on 4d063c8 in 43.598173 seconds
More details
- Looked at
11
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. tests/llm/test_cohere/test_none_response.py:5
- Draft comment:
The assertionassert response.text
might not be appropriate ifresponse_model=None
is expected to alter the response structure. Consider revising the assertion to align with the expected output whenresponse_model
isNone
. This comment applies to both synchronous and asynchronous test cases. - Reason this comment was not posted:
Confidence of 80% on close inspection, compared to threshold of 85%.
Workflow ID: wflow_rdzWS2fj4BNP2UJR
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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! Incremental review on 9ef4f5a in 43.868312 seconds
More details
- Looked at
16
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. instructor/retry.py:247
- Draft comment:
The PR description mentions changes related toresponse_model=None
, but the diff provided does not reflect these changes. Please clarify if the correct changes are included in the diff or if there is a discrepancy in the PR description. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 85%.
Workflow ID: wflow_M90H5ZLVRCn6Mbf2
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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! Incremental review on 0aa258e in 49.091434 seconds
More details
- Looked at
135
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. tests/llm/test_cohere/test_json_schema.py:16
- Draft comment:
The functiontest_parse_user_sync
is defined twice (lines 16 and 48), which will cause the second definition to overwrite the first one. Consider renaming one of these functions to ensure both functionalities are tested separately. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 85%.
Workflow ID: wflow_PWvpjyrElrHRAB1y
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
This adds support for passing in response_model = None for Cohere Sync and Async clients
Summary:
Added support for
response_model=None
in Cohere Sync and Async clients, updated relevant functions, tests, and documentation.Key points:
response_model=None
in Cohere Sync and Async clients.instructor/retry.py
to handleresponse_model=None
inretry_sync
andretry_async
functions.reask_messages
function to extendchat_history
ormessages
based on the mode.new_kwargs
dictionary with formatted messages and chat history.tests/llm/test_cohere/test_none_response.py
andtests/llm/test_cohere/test_json_schema.py
to verify the new functionality.docs/index.md
with examples demonstrating the new feature.Generated with ❤️ by ellipsis.dev