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

feat: update ChatCompletionResponseFormatJSONSchema to use jsonschema… #927

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alejandrojnm
Copy link

Describe the change
This pull request includes changes to the chat.go file to integrate the jsonschema package for handling JSON schemas in chat completion responses. The most important changes include adding the new import and updating the type of the Schema field in the ChatCompletionResponseFormatJSONSchema struct.

Integration of jsonschema package:

  • chat.go: Added the jsonschema package to the import list.
  • chat.go: Changed the Schema field type in the ChatCompletionResponseFormatJSONSchema struct from json.Marshaler to jsonschema.Definition.

Improvements to JSON schema handling:

  • chat.go: Added the jsonschema package to the imports.
  • chat.go: Changed the Schema field type in the ChatCompletionResponseFormatJSONSchema struct from json.Marshaler to jsonschema.Definition. or what feature it adds.

Provide OpenAI documentation link
Provide a relevant API doc from https://platform.openai.com/docs/api-reference

Describe your solution
The solution was use jsonschema.Definition

Tests
Nothing to test, all the tests pass locally

Issue: #884

….Definition this will fix the issue sashabaranov#884

Signed-off-by: alejandrojnm <[email protected]>
…o github.com/alejandrojnm/go-openai

Signed-off-by: alejandrojnm <[email protected]>
@kiriloman
Copy link

@sashabaranov this in fact fixes the critical issue that blocks clients from properly using the Structured Outputs. Could you please check this out?

@gspeicher
Copy link

@alejandrojnm I beg you please do not do this. Please refer to issues #819 and #824 for examples of why forcing jsonschema.Definition on everyone is not desirable. Your suggested change will break existing code for people like me who are importing a schema as a string and wrapping it using json.RawMessage() to pass to this API.

If you need Unmarshal support then as an alternative to your solution, why not define a new MarshalUnmarshaler interface (or pick a better name) as a composite of Marshaler and Unmarshaler and redefine Schema as MarshalUnmarshaler instead of jsonschema.Definition which will also satisfy that interface?

@alejandrojnm
Copy link
Author

I will try to fix that @gspeicher thanks for point in the right directions

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.

3 participants