-
-
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
Markdown JSON Mode #246
Markdown JSON Mode #246
Conversation
Warning Rate Limit Exceeded@Anmol6 has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 12 minutes and 54 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe codebase has been updated to include new imports, a Changes
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.
we should motify the messages so tthat
- the last message has a ```json
- the stop token is ```
- add the modes in our evals (lets do the user details eval)
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: 5
Configuration used: CodeRabbit UI
Files selected for processing (4)
- examples/vision/run.py (1 hunks)
- instructor/function_calls.py (3 hunks)
- instructor/patch.py (2 hunks)
- tests/openai/test_modes.py (1 hunks)
Additional comments: 6
examples/vision/run.py (2)
28-28: The radius for the circles is hardcoded to 100, which removes the randomization aspect. Verify if this is intentional or if it should be reverted to the commented-out randomization logic.
43-61: The
client.chat.completions.create
method includes aresponse_model
parameter set toCircle
. Verify that this parameter is supported by the custominstructor.patch
function and that it correctly handles theCircle
model.instructor/function_calls.py (2)
17-17: The addition of
MD_JSON
to theMode
enum is consistent with the summary.20-20: The summary mentions a modification to the
from_response
method of theopenai_function
class to handle the newMD_JSON
mode, but this change is not reflected in the provided code. Please verify if the changes were made in the correct class or if they are missing.instructor/patch.py (1)
- 79-105: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [79-117]
The implementation of the
Mode.MD_JSON
in thehandle_response_model
function appears to be correct and aligns with the summary provided. The addition of a new message block and stop token forMode.MD_JSON
is implemented as expected. The logic for inserting a system message and appending the message to the first system message if it exists is also handled properly. The code is well-structured and follows good practices for error handling by raising aValueError
for an invalid mode.tests/openai/test_modes.py (1)
- 66-83: The addition of
test_markdown_json_mode
is a good practice to ensure the new feature works as expected.
tests/openai/test_modes.py
Outdated
@@ -58,7 +58,28 @@ def test_json_mode(): | |||
], | |||
) | |||
user = UserExtract.from_response(response, mode=Mode.JSON) | |||
assert user.name.lower() == "jason" | |||
assert user.name.lower() |
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 assertion assert user.name.lower()
will pass even if user.name
is an empty string. It should explicitly check for a non-empty string.
- assert user.name.lower()
+ assert user.name.lower() != ''
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.
assert user.name.lower() | |
assert user.name.lower() != '' |
tests/openai/test_modes.py
Outdated
], | ||
) | ||
user = UserExtract.from_response(response, mode=Mode.MD_JSON) | ||
assert user.name.lower() |
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 assertion assert user.name.lower()
will pass even if user.name
is an empty string. It should explicitly check for a non-empty string.
- assert user.name.lower()
+ assert user.name.lower() != ''
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.
assert user.name.lower() | |
assert user.name.lower() != '' |
@@ -1,4 +1,5 @@ | |||
import json | |||
import re |
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 re
module is imported but not used in the provided code. If it's not used elsewhere, consider removing the import to avoid unnecessary dependencies.
context=validation_context, | ||
strict=strict, | ||
) | ||
elif mode == Mode.MD_JSON: | ||
return cls.model_validate_json( | ||
message.content, | ||
context=validation_context, | ||
strict=strict, | ||
) |
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 from_response
method in the OpenAISchema
class has been updated to handle the MD_JSON
mode. However, the logic for MD_JSON
is identical to the JSON
mode. If the handling for MD_JSON
is supposed to be different, this needs to be implemented, otherwise, the MD_JSON
case can be combined with the JSON
case to avoid code duplication.
response = client.chat.completions.create( | ||
model="gpt-4-vision-preview", | ||
max_tokens=1800, | ||
response_model=Circle, | ||
messages=[ | ||
{ | ||
"role": "user", | ||
"content": [ | ||
{"type": "text", "text": 'find the circle'}, | ||
{ | ||
"type": "image_url", | ||
"image_url": { | ||
"url": f"data:image/jpeg;base64,{base64_image}" | ||
}, | ||
}, | ||
], | ||
} | ||
], | ||
) | ||
|
||
print(f"Found circle with center at x: {response.x}, y: {response.y} and color: {response.color}") |
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.
There is no error handling for the API call or file operations. Consider adding try-except blocks to handle potential exceptions and provide a graceful error message to the user.
added some tests and found that reask does not work cause it'll apologize ands hit.
|
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/patch.py (4 hunks)
Additional comments: 4
instructor/patch.py (4)
- 79-105: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [79-117]
The implementation of
Mode.MD_JSON
in thehandle_response_model
function appears to be consistent with the PR objective and the summary. The addition of a new message block and stop token forMode.MD_JSON
is correctly implemented.
185-197: The handling for
Mode.MD_JSON
in theretry_async
function is consistent with the PR objective and the summary. The addition of the ```json message block upon retry is correctly implemented.229-241: The handling for
Mode.MD_JSON
in theretry_sync
function is consistent with the PR objective and the summary. The addition of the ```json message block upon retry is correctly implemented.94-94: The implementation includes the stop token and the ```json message block as suggested in the existing comments. This supports streaming natively and avoids the need for JSON parsing.
maybe in patching add a note that like if you want to try the vision model it only supports md_mode, and add a
|
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, can we update docs and add a warning?
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)
- docs/concepts/patching.md (1 hunks)
Additional comments: 3
docs/concepts/patching.md (3)
40-56: The summary suggests that "Markdown JSON Mode" replaces "JSON Mode," but the hunk shows that both modes are documented, with "Markdown JSON Mode" added as a new section. Please verify if "JSON Mode" is indeed replaced or if both modes are meant to coexist.
45-47: The warning about the experimental nature of "Markdown JSON Mode" is clear and important for users to understand the potential risks of using this feature.
54-54: The code for initializing the
client
withMode.MD_JSON
is correct and aligns with the PR objective to introduce Markdown JSON handling.
Addresses Issue #235
Summary by CodeRabbit
New Features
Circle
class for enhanced image processing.MD_JSON
for extended functionality in handling responses.Enhancements
draw_circle
function to include image encoding to base64.Bug Fixes
response
logic for better accuracy and performance.Tests