-
-
Notifications
You must be signed in to change notification settings - Fork 602
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
Adding support for structured outputs #938
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 205469a in 9 seconds
More details
- Looked at
56
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. tests/llm/test_openai/util.py:6
- Draft comment:
Consider adding test cases to verify the functionality of the new 'STRUCTURED_OUTPUTS' mode to ensure it behaves as expected. - Reason this comment was not posted:
Confidence changes required:50%
The new mode 'STRUCTURED_OUTPUTS' is added to the Mode enum and is being used in the handle_response_model function. However, there is no test case added to verify its functionality.
2. tests/llm/test_openai/util.py:6
- Draft comment:
Ensure that tests are added for the newSTRUCTURED_OUTPUTS
mode to verify its functionality. - Reason this comment was not posted:
Confidence changes required:80%
The new modeSTRUCTURED_OUTPUTS
was added to themodes
list in the test file, but there is no indication of new tests being added for this mode.
3. instructor/mode.py:22
- Draft comment:
Ensure that documentation is updated inmkdocs.yml
for the newSTRUCTURED_OUTPUTS
mode. - Reason this comment was not posted:
Confidence changes required:80%
The new modeSTRUCTURED_OUTPUTS
was added to theMode
enum, but there is no documentation update inmkdocs.yml
.
Workflow ID: wflow_HHRghWPv03Benrx9
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
A bit skeptical if this works but https://openai.com/index/introducing-structured-outputs-in-the-api/ we can see that the easiest way to do this is really Which we can just hook into using our |
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 718fd82 in 15 seconds
More details
- Looked at
109
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. instructor/client.py:68
- Draft comment:
Consider maintaining consistency in return type annotations across the codebase. The change from ellipsis to inline return type is stylistic and should be consistent throughout. - Reason this comment was not posted:
Confidence changes required:33%
The PR modifies the return type annotations by removing the ellipsis and placing the return type on the same line. This is a stylistic change and does not affect functionality. However, it is important to maintain consistency across the codebase.
2. instructor/client.py:65
- Draft comment:
Function and method naming should follow consistent patterns. Ensure that methods likecreate
,create_partial
,create_iterable
, andcreate_with_completion
in bothInstructor
andAsyncInstructor
classes, as well as functions likefrom_openai
andfrom_litellm
, follow a consistent naming pattern. - Reason this comment was not posted:
Confidence changes required:80%
The code has multiple instances where the function and method naming pattern is inconsistent. The methodscreate
,create_partial
,create_iterable
, andcreate_with_completion
in bothInstructor
andAsyncInstructor
classes should follow a consistent naming pattern. Additionally, thefrom_openai
andfrom_litellm
functions should also follow a consistent naming pattern.
Workflow ID: wflow_3ZnHe8KIylEv5GpU
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 dd4e0ea in 16 seconds
More details
- Looked at
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. instructor/function_calls.py:267
- Draft comment:
Ensure that thehandle_response_model
ininstructor/process_response.py
is updated to handleSTRUCTURED_OUTPUTS
as mentioned in the PR description. This change is not visible in the current diff. - Reason this comment was not posted:
Confidence changes required:80%
The addition ofMode.STRUCTURED_OUTPUTS
to theparse_tools
method is consistent with the intent of supporting structured outputs. However, the PR description mentions updates tohandle_response_model
ininstructor/process_response.py
, which is not visible in the current diff. This might be an oversight or a missing part of the PR.
2. instructor/function_calls.py:264
- Draft comment:
Assertions should have error messages. Consider adding descriptive error messages to the assertions inparse_functions
andparse_tools
methods. - Reason this comment was not posted:
Confidence changes required:80%
The assertion statements in theparse_functions
andparse_tools
methods lack error messages. Adding error messages will make it easier to understand the cause of the assertion failure.
Workflow ID: wflow_snnZkFQeeo9APo8A
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 3c7cf38 in 34 seconds
More details
- Looked at
102
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. instructor/client.py:65
- Draft comment:
Function and method names should follow a consistent pattern. Consider using verbs to describe actions, such asgenerate
instead ofcreate
. This applies to other methods likecreate_partial
,create_iterable
, andcreate_with_completion
. - Reason this comment was not posted:
Confidence changes required:80%
The function and method naming in this file is inconsistent. The method names should follow a consistent pattern, such as using verbs to describe actions.
Workflow ID: wflow_Pi1FgCKi3UeadGA4
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Skipped PR review on f614934 because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away. Generated with ❤️ by ellipsis.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! Incremental review on 96de065 in 30 seconds
More details
- Looked at
13
lines of code in1
files - Skipped
1
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. pyproject.toml:13
- Draft comment:
Theopenai
package version has been downgraded from^1.40.0
to^1.1.0
. Ensure this is intentional and verify compatibility with the rest of the codebase. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment violates the rule against asking the PR author to confirm their intention or verify compatibility. It is speculative and not actionable, as it does not point out a definite issue but rather suggests a potential one. The downgrade might be intentional, and the comment does not provide a clear code change requirement.
I might be overlooking the importance of highlighting a version downgrade, which could potentially introduce compatibility issues. However, the comment does not provide a specific action to take, making it less useful.
While version downgrades can be significant, the comment should focus on definite issues rather than speculative ones. The rules clearly state not to ask for confirmation or verification from the PR author.
The comment should be removed as it is speculative and does not provide a clear code change requirement.
2. pyproject.toml:13
- Draft comment:
If theopenai
library version is changed, ensure that the documentation and tests are updated accordingly to reflect any changes in functionality or API usage. - Reason this comment was not posted:
Confidence changes required:80%
The openai library version has been downgraded, which is a significant change.
Workflow ID: wflow_pqOrptU96hBnkxcx
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Skipped PR review on 07cf3ba because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away. Generated with ❤️ by ellipsis.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! Incremental review on 7df4691 in 19 seconds
More details
- Looked at
26
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. instructor/process_response.py:250
- Draft comment:
Remove the duplicate import ofMode
frominstructor.mode
. - Reason this comment was not posted:
Confidence changes required:50%
The code has a repeated import statement forMode
frominstructor.mode
. This is unnecessary and should be removed to clean up the code.
2. instructor/process_response.py:254
- Draft comment:
The linefunction_response_model["strict"] = True
is redundant asstrict
is already set in thetools
list. Consider removing this line. - Reason this comment was not posted:
Confidence changes required:50%
The code setsfunction_response_model["strict"] = True
forMode.STRUCTURED_OUTPUTS
, but this is not necessary as it is already set in thetools
list. This line can be removed to avoid redundancy.
3. instructor/process_response.py:253
- Draft comment:
Use a more descriptive variable name thanfunction_response_model
for clarity and maintainability. This applies to other instances in the file as well. - Reason this comment was not posted:
Confidence changes required:80%
The code contains multiple instances where the variablefunction_response_model
is used without a descriptive name. This violates the rule of using descriptive variable names. A more descriptive name would improve code readability and maintainability.
4. instructor/process_response.py:250
- Draft comment:
Ensure function and method naming follows a consistent pattern throughout the codebase. This applies to other functions in the file as well. - Reason this comment was not posted:
Confidence changes required:70%
The code contains multiple instances where the function naming pattern is inconsistent. This violates the rule of having consistent function and method naming patterns, which is important for code readability and maintainability.
Workflow ID: wflow_Ii7S3SLgVEt8PkGZ
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 1a6916f in 19 seconds
More details
- Looked at
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. instructor/retry.py:111
- Draft comment:
Consider adding a check forMode.STRUCTURED_OUTPUTS
inretry_sync
andretry_async
functions to ensure proper handling of this mode. - Reason this comment was not posted:
Confidence changes required:80%
The code is missing a check for the newMode.STRUCTURED_OUTPUTS
in theretry_sync
andretry_async
functions. This could lead to unexpected behavior if this mode is used.
2. instructor/retry.py:111
- Draft comment:
This function is complex and handles multiple modes. Consider adding comments to explain the logic for each mode. - Reason this comment was not posted:
Confidence changes required:50%
The functionreask_messages
is complex and handles multiple modes. It would benefit from comments explaining the logic, especially for each mode.
Workflow ID: wflow_kmSdezFEBHKRvr2g
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 0f9b258 in 15 seconds
More details
- Looked at
35
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. instructor/process_response.py:254
- Draft comment:
Ensure thatpydantic_function_tool
returns a dictionary with a 'function' key. If not, this could lead to a KeyError. Verify the structure of the returned schema. - Reason this comment was not posted:
Confidence changes required:80%
The code inhandle_response_model
function has a potential issue with the use ofpydantic_function_tool
. The functionpydantic_function_tool
is used to convert a response model to a schema, but the way it is used here might not be correct. Thepydantic_function_tool
function is expected to return a schema, but the code assumes it returns a dictionary with a 'function' key, which might not be the case. This could lead to a KeyError if the structure of the returned schema is not as expected.
2. instructor/process_response.py:251
- Draft comment:
This function is complex and would benefit from comments explaining its logic and purpose. - Reason this comment was not posted:
Confidence changes required:80%
The functionhandle_response_model
is quite complex and lacks comments explaining its logic. This violates the rule that extremely complicated code needs comments.
Workflow ID: wflow_26N94f3nwDOdHphQ
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 b4f5c25 in 23 seconds
More details
- Looked at
33
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. pyproject.toml:13
- Draft comment:
The update to theopenai
dependency version is incorrect. The PR description mentions updating from^1.40.0
to^1.1.0
, but the change in the file is the opposite. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment highlights a potential issue with the PR description not matching the code change. This is a valid concern as it could lead to confusion or errors if the change is not as intended. The comment is actionable as it suggests verifying the intended version update.
The comment assumes the PR description should be filled out and correct, which may not always be the case. The focus should be on whether the code change itself is correct.
While the PR description may not always be filled out, discrepancies between the description and code changes can indicate potential issues that need to be addressed.
The comment should be kept as it highlights a potential issue with the version update that needs to be clarified.
2. pyproject.toml:13
- Draft comment:
If theopenai
dependency version is updated, ensure that the documentation and tests are also updated accordingly. - Reason this comment was not posted:
Confidence changes required:80%
The update to theopenai
dependency inpyproject.toml
should be reflected in the documentation and tests, as per the rules.
Workflow ID: wflow_pMwKSLH3fmuYnkWB
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 af5ecd5 in 32 seconds
More details
- Looked at
59
lines of code in3
files - Skipped
1
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. pyproject.toml:74
- Draft comment:
Theopenai
dependency version has been changed from^1.40.0
to^1.1.0
, which appears to be a downgrade. Please verify if this is intentional or a typo. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is not relevant to the diff provided. It mentions a change in theopenai
dependency, which is not present in the diff. Therefore, the comment is incorrect and should be removed.
I might be missing some context if the diff provided is incomplete, but based on the information given, the comment does not apply to the changes shown.
Even if there is missing context, the comment should be relevant to the diff provided. Since it is not, it should be removed.
The comment should be removed as it is not relevant to the changes shown in the diff.
Workflow ID: wflow_Gbng99ridMuSLuK0
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 5ee3b58 in 16 seconds
More details
- Looked at
15
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. tests/llm/test_openai/test_validators.py:28
- Draft comment:
The comment here is informative but could be more concise. Consider rephrasing to:# Skipping test for structured outputs due to lack of validator support.
- Reason this comment was not posted:
Confidence changes required:10%
The comment on line 28 is informative but could be more concise. It should also be clear that the test is being skipped due to current limitations.
2. tests/llm/test_openai/test_validators.py:24
- Draft comment:
Function names should follow a consistent pattern. Consider renamingtest_runmodel_validator_error
to match the pattern used in other test functions, such astest_patch_completes_successfully
. This applies to other test functions in this file as well. - Reason this comment was not posted:
Confidence changes required:80%
The test function names are inconsistent. They should follow a consistent pattern for better readability and maintenance.
Workflow ID: wflow_FhvljvC63oVPRD7r
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 2140a24 in 25 seconds
More details
- Looked at
13
lines of code in1
files - Skipped
1
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. pyproject.toml:74
- Draft comment:
Theopenai
dependency version should be updated to^1.1.0
as mentioned in the PR description. Please update the version here to reflect that change. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is not relevant to the current diff as it refers to a change that is not present. The comment violates the rule of not commenting on code that was not changed in the PR diff. Therefore, it should be removed.
I might be missing context from the PR description that could justify the comment. However, without seeing the actual change in the diff, the comment is not actionable.
Even if the PR description mentions a change, the comment should only be made if the change is visible in the diff. Otherwise, it is not relevant to the current review.
The comment should be removed because it refers to a change not present in the diff, violating the rule against commenting on unchanged code.
2. pyproject.toml:77
- Draft comment:
Theopenai
dependency version should be updated to^1.1.0
as mentioned in the PR description. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_DhVtqIN4FMdFYt6w
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 5b9b0d1 in 15 seconds
More details
- Looked at
40
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. docs/index.md:81
- Draft comment:
The model identifier"gpt-4o"
might be incorrect. Please verify if this is the intended model name or if it should be"gpt-4"
or another valid identifier. - Reason this comment was not posted:
Confidence changes required:80%
The example code in the documentation uses a model identifier 'gpt-4o', which might be a typo or an incorrect model identifier. It should be verified against the available models in the OpenAI API.
2. docs/index.md:54
- Draft comment:
Make sure to add the new example for OpenAI's structured output response to the mkdocs.yml file to ensure it's included in the documentation. - Reason this comment was not posted:
Confidence changes required:80%
The new example for OpenAI's structured output response should be added to the mkdocs.yml file to ensure it's included in the documentation.
Workflow ID: wflow_5QkzVcKOOrkZQE1b
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Most changes are due to the poetry.lock file being regenerated, opted to just ue a more low level approach rather than their sdk |
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 c302a5c in 20 seconds
More details
- Looked at
14
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. instructor/function_calls.py:437
- Draft comment:
The assertion message formessage.refusal
could be more informative. Consider providing additional context or guidance on what the refusal means or how to resolve it. - Reason this comment was not posted:
Confidence changes required:50%
The assertion message formessage.refusal
is not very informative. It should provide more context or guidance on what the refusal means or how to resolve it.
2. instructor/function_calls.py:433
- Draft comment:
Assertions should have error messages for clarity. Consider adding error messages to the assertions on lines 433 and 440. - Reason this comment was not posted:
Confidence changes required:80%
The assertion statements in theparse_tools
function lack error messages.
Workflow ID: wflow_KIiCPFbNZGyBSM8U
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.
this is strict tool calling, not structured outputs as defined by the parse method
Hmm I was following the instructions in their announcement article here which toggles it by default when you use strict = True when calling tools openai.com/index/introducing-structured-outputs-in-the-api See the Docs here too : https://platform.openai.com/docs/guides/function-calling I think this is a better way to do structured output since it allows us to hook into all of the existing functionality that we've already built instead of rebuilding new functionality around the new |
…ing the parsing of the code itself
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 ea1509b in 55 seconds
More details
- Looked at
174
lines of code in10
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. docs/index.md:65
- Draft comment:
The modeMode.STRUCTURED_OUTPUTS
has been changed toMode.TOOLS_STRICT
in the codebase. Update the documentation to reflect this change.
client = from_openai(OpenAI(), mode=Mode.TOOLS_STRICT)
- Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is redundant because the change fromMode.STRUCTURED_OUTPUTS
toMode.TOOLS_STRICT
is already reflected in the documentation as shown in the diff. The suggestion to update the documentation is unnecessary since the change is already present.
I might be missing the context of whether this change is consistent throughout the entire documentation or if there are other instances that need updating. However, based on the provided diff, the change is already made.
The task is to review the specific diff provided, and within this context, the change is already made. Therefore, the comment is unnecessary for this specific instance.
The comment should be removed because the documentation already reflects the change fromMode.STRUCTURED_OUTPUTS
toMode.TOOLS_STRICT
.
2. docs/index.md:65
- Draft comment:
The modeSTRUCTURED_OUTPUTS
has been changed toTOOLS_STRICT
in the codebase. Update the documentation to reflect this change.
client = from_openai(OpenAI(), mode=Mode.TOOLS_STRICT)
- Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_GoAtGG0J42pfFTjB
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Adding in the Structured Outputs announcement article
This seems to be a much easier integration
Summary:
This PR adds
Mode.STRUCTURED_OUTPUTS
andMode.TOOLS_STRICT
, updates JSON handling, and modifies tests and dependencies.Key points:
Mode.STRUCTURED_OUTPUTS
andMode.TOOLS_STRICT
ininstructor/mode.py
.handle_response_model
for both modes.instructor/dsl/iterable.py
andinstructor/dsl/partial.py
.from_response
ininstructor/function_calls.py
.instructor/client.py
for both modes.tests/llm/test_openai/test_validators.py
.^1.40.0
to^1.1.0
.Generated with ❤️ by ellipsis.dev