-
-
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
fix: add handling for List[non-object] types #521
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.
❌ Changes requested.
- Reviewed the entire pull request up to 2ea78b4
- Looked at
96
lines of code in2
files - Took 2 minutes and 40 seconds to review
More info
- Skipped
0
files when reviewing. - Skipped posting
3
additional comments because they didn't meet confidence threshold of85%
.
1. instructor/anthropic_utils.py:75
:
- Assessed confidence :
80%
- Comment:
The error message 'Invalid array item.' could be more descriptive. Consider including the field name in the error message to make it clear which field is causing the error. For example:
raise ValueError(f"Invalid array item in field '{field_name}'.")
- Reasoning:
The PR author has added a check for 'array' type fields in the model and is raising a ValueError if 'items' is not present in the details of the field. This is a good check to ensure that the array type fields are well defined. However, the error message 'Invalid array item.' is not very descriptive. It would be helpful to include the field name in the error message to make it clear which field is causing the error.
2. instructor/anthropic_utils.py:101
:
- Assessed confidence :
80%
- Comment:
The comment 'Handling for List[] type' is now misleading as it only applies to nested List types. Consider updating the comment to 'Handling for nested List[] type'. - Reasoning:
The PR author has removed the handling for optional List[] type. This seems to be a logical change as the handling for optional List[] type is now covered by the handling for non-nested List types. However, the comment 'Handling for List[] type' is now misleading as it only applies to nested List types. The comment should be updated to reflect this.
3. tests/anthropic/test_simple.py:67
:
- Assessed confidence :
100%
- Grade:
0%
- Comment:
The test case is currently marked with @pytest.mark.skip, which means it will not be executed. Consider removing this decorator to enable the test case. - Reasoning:
The PR author has added a new test case for List[non-object] type. This is a good addition as it tests the new functionality added in this PR. However, the test case is currently marked with @pytest.mark.skip, which means it will not be executed. The PR author should remove this decorator to enable the test case.
Workflow ID: wflow_CkSfAXq1LZ7kr8QS
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. We'll respond in a few minutes. Learn more here.
type_element.text = f"List[{details['title']}]" | ||
list_found = True | ||
nested_list_found = True |
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.
The variable 'nested_list_found' is set to True when a nested List type is found but it is not reset to False for non-nested List types. This could potentially cause issues if a non-nested List type follows a nested List type as 'nested_list_found' would still be True. Consider resetting 'nested_list_found' to False for non-nested List types.
family: List[str] | ||
|
||
resp = create( | ||
model="claude-3-opus-20240229", # Fails with claude-3-haiku-20240307 |
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.
interesting, we should mention this ins some kind of "anthropic tips" part
make sure to fix the poetry lock and ruff issues :) |
Summary:
This PR enhances the handling of
List[non-object]
types in the model and adds a corresponding test case.Key points:
_add_params
in/instructor/anthropic_utils.py
to handleList[non-object]
types.List[non-object]
types in/tests/anthropic/test_simple.py
.Generated with ❤️ by ellipsis.dev