-
-
Notifications
You must be signed in to change notification settings - Fork 730
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 finish reason exception IncompleteOutputException #279
Conversation
WalkthroughA new custom exception Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ? TipsChat with CodeRabbit Bot (
|
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- instructor/exceptions.py (1 hunks)
- instructor/function_calls.py (4 hunks)
- tests/test_function_calls.py (2 hunks)
Additional comments: 6
instructor/exceptions.py (1)
- 1-6: The implementation of
IncompleteOutputException
is correct and follows Python's standard practices for custom exceptions.instructor/function_calls.py (3)
4-4: The import of
IncompleteOutputException
is correctly added to support the new exception handling logic.125-126: The addition of the conditional check to raise
IncompleteOutputException
when thefinish_reason
is 'length' correctly implements the desired functionality described in the PR.235-235: The
# type: ignore
comment has been added without context. Please provide an explanation for this change, as it is not mentioned in the PR objectives or the AI-generated summaries.tests/test_function_calls.py (2)
16-47: The
mock_completion
fixture is well-implemented using indirect parameterization to test different scenarios. This is a good use of pytest's features to create flexible and reusable test setups.86-104: The new tests for checking the behavior of
IncompleteOutputException
are correctly implemented, using both synchronous and asynchronous versions of thefrom_response
method. The use ofpytest.mark.parametrize
andpytest.mark.asyncio
is appropriate for the intended test scenarios.
instructor/function_calls.py
Outdated
@@ -121,6 +122,9 @@ def from_response( | |||
Returns: | |||
cls (OpenAISchema): An instance of the class | |||
""" | |||
if completion.choices[0].finish_reason == 'length': | |||
raise IncompleteOutputException() |
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 indentation for the raise IncompleteOutputException()
line is inconsistent with the surrounding code. It should be aligned with the rest of the code block for readability and to adhere to PEP 8 standards.
- raise IncompleteOutputException()
+ raise IncompleteOutputException()
Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
raise IncompleteOutputException() | |
raise IncompleteOutputException() |
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- instructor/function_calls.py (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- instructor/function_calls.py
Based off this issue.
Needed an exception raised when the finish_reason for the completion was due to exceeding
max_tokens
("length") versus when thefinish_reason
isstop
.Maybe better if this exception was called
FinishReasonException
? Or isIncompleteOutputException
okay?Also, do we want this exceptions file located somewhere else?
Summary by CodeRabbit
New Features
Bug Fixes
IncompleteOutputException
when output is incomplete in synchronous and asynchronous response handling functions.Tests
IncompleteOutputException
under specified conditions.