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

Make reponse format JSONSchema optional #820

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

tylergannon
Copy link
Contributor

Describe the change
This morning's PR #813 makes it impossible to do ChatCompletionRequest that doesn't supply a json schema.

Describe your solution
I followed the suggestion in #818, changing the JSONSchema attached to a request format object to a pointer type, so that the JSON marshaler can properly exclude it when not provided.

Tests
I ran the go tests. (did you guys know you have a bunch of broken tests in the repo?)
All tests pass except for the ones in the jsonschema directory. Those tests were broken both before and after my changes.

I updated my own software to use my fork, and everything works.

Issue: #818

Copy link

codecov bot commented Aug 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.88%. Comparing base (774fc9d) to head (7d18593).
Report is 36 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #820      +/-   ##
==========================================
+ Coverage   98.46%   98.88%   +0.42%     
==========================================
  Files          24       26       +2     
  Lines        1364     1347      -17     
==========================================
- Hits         1343     1332      -11     
+ Misses         15        9       -6     
  Partials        6        6              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@sashabaranov sashabaranov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR! I wonder why our integration tests did not catch that

@sashabaranov sashabaranov merged commit 6439e1f into sashabaranov:master Aug 7, 2024
3 checks passed
@sashabaranov
Copy link
Owner

@gspeicher
Copy link

Thank you for the PR! I wonder why our integration tests did not catch that

@sashabaranov there doesn't appear to be an integration test with a ResponseFormat that uses ChatCompletionResponseFormatTypeJSONObject, which would have caught the oversight.

@tylergannon
Copy link
Contributor Author

Glad it was such an easy fix.

@eiixy
Copy link
Contributor

eiixy commented Aug 8, 2024

I'm very sorry, the integration tests did not cover this part of the functionality.

@HaraldNordgren
Copy link
Contributor

@sashabaranov The integration tests are failing now. Is it safe to use this latest version?

@HaraldNordgren
Copy link
Contributor

@sashabaranov @tylergannon @eiixy Follow-up to make sure the integration tests run for a PR (most are skipped), but it still verifies the overall syntax I believe:

#823

I use this API for production, so it's important to me that it works well 🚀

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.

5 participants