Skip to content
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: json modes - don't add json schema again in system message #435

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

teome
Copy link
Contributor

@teome teome commented Feb 13, 2024

Using any of the JSON modes, the final action in handle_response_model adds the schema to the system message.

This PR fixes fixes a missing else in the logic for checking the existence of the system message. If there's no system message the code currently adds the schema twice: first by creating a system message, then checking if a system message exists and if so appending the schema.

Just needs the else


Ellipsis 🚀 This PR description was created by Ellipsis for commit e019a3e.

Summary:

This PR fixes a bug in the handle_response_model function that caused the JSON schema to be added twice to the system message.

Key points:

  • Fixed an issue in handle_response_model function in instructor/patch.py.
  • Added an else clause to prevent the JSON schema from being added twice to the system message.

Generated with ❤️ by ellipsis.dev

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 entire PR up to commit e019a3e

Reviewed 17 lines of code across 1 files in 42 second(s).

See details
  • Skipped files: 0 (please contact us to request support for these files)
  • Confidence threshold: 85%
  • Drafted 1 additional comments.
  • Workflow ID: wflow_b7mvQAK1YGtTyuoB
View 1 draft comments

These comments were drafted by Ellipsis, but were filtered out of the final review. They're included here so you can see our internal thought process and help you configure your ellipsis.yaml.

Drafted 1 comments under confidence threshold

Filtered comment at instructor/patch.py:164

Confidence changes required: 50%

Commentary: The change in the PR is to add an 'else' clause to the condition checking if the first message is a system message. This is to prevent the schema from being added twice to the system message. The change seems logical and doesn't appear to introduce any new bugs. However, I would like to confirm that the 'content' field of the system message is a string and can handle the concatenation operation.

Please confirm that the 'content' field of the system message is a string and can handle the concatenation operation.


Something look wrong? You can customize Ellipsis by editing the ellipsis.yaml for this repository.

Generated with ❤️ by ellipsis.dev

@jxnl
Copy link
Owner

jxnl commented Feb 13, 2024

Will this work? I'm pretty sure the message should be part of the system message.

@teome
Copy link
Contributor Author

teome commented Feb 13, 2024

I believe so, unless I'm going crazy, because if new_kwargs["messages"][0]["role"] != "system then you were already adding the message with schema. If we go to the else, it must be system already and it should be added. Before, if there was no system message, it was added, then appended to, resulting in two copies of the json schema message

@jxnl jxnl merged commit 6d72c78 into jxnl:main Feb 13, 2024
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants