Skip to content

Make reponse format JSONSchema optional#820

Merged
sashabaranov merged 1 commit intosashabaranov:masterfrom
tylergannon:master
Aug 7, 2024
Merged

Make reponse format JSONSchema optional#820
sashabaranov merged 1 commit intosashabaranov:masterfrom
tylergannon:master

Conversation

@tylergannon
Copy link
Copy Markdown
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

@codecov
Copy link
Copy Markdown

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
Copy Markdown
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
@sashabaranov
Copy link
Copy Markdown
Owner

@gspeicher
Copy link
Copy Markdown

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
Copy Markdown
Contributor Author

Glad it was such an easy fix.

@eiixy
Copy link
Copy Markdown
Contributor

eiixy commented Aug 8, 2024

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

@HaraldNordgren
Copy link
Copy Markdown
Contributor

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

@HaraldNordgren
Copy link
Copy Markdown
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