-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
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.
Thank you for the PR! I wonder why our integration tests did not catch that
@tylergannon oh also it broke tests https://github.com/sashabaranov/go-openai/actions/runs/10290697434/job/28481256130 |
@sashabaranov there doesn't appear to be an integration test with a |
Glad it was such an easy fix. |
I'm very sorry, the integration tests did not cover this part of the functionality. |
@sashabaranov The integration tests are failing now. Is it safe to use this latest version? |
@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: I use this API for production, so it's important to me that it works well 🚀 |
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