Fix streamed list of union basemodels #1141
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 481d881 in 47 seconds
More details
- Looked at
90lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. instructor/dsl/partial.py:57
- Draft comment:
Consider refactoring the repeated pattern of addingNonetoUnionoptions to a helper function to improve readability and maintainability. This pattern appears in both_make_field_optionaland_process_annotation. - Reason this comment was not posted:
Confidence changes required:50%
The code has a repeated pattern for handling Union types and adding None to the options. This can be refactored to improve readability and maintainability.
2. instructor/dsl/partial.py:41
- Draft comment:
Consider ifdeepcopyis necessary forFieldInfoobjects. If a shallow copy suffices, it could improve performance. - Reason this comment was not posted:
Confidence changes required:50%
The code usesdeepcopyonFieldInfoobjects, which might be unnecessary and could impact performance. It's worth considering if a shallow copy would suffice.
3. instructor/dsl/partial.py:58
- Draft comment:
Review the use of# type: ignorecomments to ensure they are necessary and safe. Consider addressing the root cause of type issues if possible. This applies to other instances of# type: ignorein the code as well. - Reason this comment was not posted:
Confidence changes required:50%
The code usestype: ignorecomments, which can hide potential type issues. It's better to address the root cause or ensure that these are truly safe to ignore.
4. instructor/dsl/partial.py:39
- Draft comment:
The function_make_field_optionalis complex and should have comments explaining its logic, especially around handling generics and Union types. This applies to_process_annotationas well. - Reason this comment was not posted:
Confidence changes required:80%
The function_make_field_optionaland_process_annotationare complex and require comments for better understanding.
5. instructor/dsl/partial.py:78
- Draft comment:
The function_process_annotationis complex and should have comments explaining its logic, especially around handling Union types and BaseModel. - Reason this comment was not posted:
Confidence changes required:80%
The function_process_annotationis complex and requires comments for better understanding.
Workflow ID: wflow_MENGYJmD9vlhFJm8
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
this should ultimtely be handled by pydantic will wait for a fix then |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes Pydantic ValidationError when streaming model with a list of unions of basemodels.
Example:
Before:
After:
Closes #1140
Important
Fixes Pydantic ValidationError for streaming models with lists of unions of BaseModels by updating
_make_field_optional()ininstructor/dsl/partial.py.instructor/dsl/partial.py.Noneto Union options and sets default toNonein_make_field_optional()._process_annotation()to handle nested annotations, especially for Union types._make_field_optional()to handle Union types and other generics correctly.Unionininstructor/dsl/partial.py.This description was created by
for 481d881. It will automatically update as commits are pushed.