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

Improve handling of JSON Schema in OpenAI API Response Context #819

Merged
merged 21 commits into from
Aug 24, 2024

Conversation

eiixy
Copy link
Contributor

@eiixy eiixy commented Aug 7, 2024

  • add: jsonschema.Unmarshal
  • add: jsonschema.Validate
  • add: jsonschema.GenerateSchemaForType
  • update: openai.ChatCompletionResponseFormatJSONSchema

@eiixy eiixy marked this pull request as draft August 7, 2024 16:14
Copy link

codecov bot commented Aug 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.01%. Comparing base (774fc9d) to head (8680e7b).
Report is 44 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #819      +/-   ##
==========================================
+ Coverage   98.46%   99.01%   +0.55%     
==========================================
  Files          24       26       +2     
  Lines        1364     1418      +54     
==========================================
+ Hits         1343     1404      +61     
+ Misses         15        8       -7     
  Partials        6        6              

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

return string(bytes)
}

func Warp[T any](v T) SchemaWrapper[T] {

Choose a reason for hiding this comment

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

@eiixy heads up: I think you meant Wrap instead of Warp here.

Copy link
Owner

Choose a reason for hiding this comment

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

Or even better: GenerateSchemaForType

}
d.Required = requiredFields
d.Properties = properties
default:
Copy link
Owner

Choose a reason for hiding this comment

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

I suggest we either return an error or panic here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion!

KebabCase string `json:"kebab_case" required:"true" description:"KebabCase"`
SnakeCase string `json:"snake_case" required:"true" description:"SnakeCase"`
}
schema := jsonschema.Warp(MyStructuredResponse{})
Copy link
Owner

Choose a reason for hiding this comment

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

Great stuff, I guess we should put a proper README example on how to use this. That should be one of the top examples for sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added an example use case from OpenAI documentation to the README. For reference, please see: https://platform.openai.com/docs/guides/structured-outputs/examples

@@ -53,3 +58,90 @@ func (d Definition) MarshalJSON() ([]byte, error) {
Alias: (Alias)(d),
})
}

type SchemaWrapper[T any] struct {
Copy link
Owner

Choose a reason for hiding this comment

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

Pardon my ignorance: do we even need this type?

Copy link
Contributor Author

@eiixy eiixy Aug 9, 2024

Choose a reason for hiding this comment

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

The SchemaWrapper type is intended to bind the Definition and type together to ensure consistency during unmarshalling. This way, the schema’s type remains aligned with the initial definition, reducing the risk of errors or mismatches during data handling.

sw, err := jsonschema.Wrap(MyStructuredResponse{})
result, err := sw.Unmarshal(`{...}`)

or

var result MyStructuredResponse{}
schema, err := jsonschema.GenerateSchemaForType(result)
schema.Unmarshal(`{...}`, &result)

@sashabaranov
Copy link
Owner

Great work, super excited for this!

@eiixy eiixy changed the title feat: add jsonschema.Validate and jsonschema.Unmarshal Improve handling of JSON Schema in OpenAI API Response Context Aug 9, 2024
@gspeicher
Copy link

If this PR is not going to be finished soon, would it be possible to lift the small change to openai.ChatCompletionResponseFormatJSONSchema promoting Schema from jsonschema.Definition to json.Marshaler as a separate PR? The requirement to use jsonschema is currently a roadblock for myself and others who use this package, as evidenced by PR #824.

Also, I feel that using json.Marshaler is both cleaner and more in the spirit of Go than adding a new SchemaRaw field side-by-side with Schema as proposed by that PR. If you already have a schema in string form then you can simply wrap it in a custom type that satisfies json.Marshaler like so:

// MySchema is a custom type that wraps a string
type MySchema string

// MarshalJSON implements the json.Marshaler interface for MySchema
func (m MySchema) MarshalJSON() ([]byte, error) {
	return []byte(m), nil
}

@eiixy eiixy marked this pull request as ready for review August 13, 2024 08:06
@eiixy
Copy link
Contributor Author

eiixy commented Aug 13, 2024

@sashabaranov @gspeicher I have updated the PR status to "ready." Please review the latest changes and let me know if there are any further adjustments needed. Thank you for your suggestions and patience!

@gspeicher
Copy link

@eiixy thanks for pulling this together quickly. My feedback is centered around GenerateSchemaForType.

  1. The name feels a little misleading to me, because it implies that you would pass the type corresponding to the schema you want to generate, but instead it expects an object of that type. Seems like either the function name or the parameter type should change so they agree. I defer to @sashabaranov since the name was his suggestion and it's his package.
  2. Do we still need SchemaWrapper and its Wrap method if we have GenerateSchemaForType? It's not clear to me when you would use SchemaWrapper and it feels like an unnecessarily abstraction of the native JSON package.

@eiixy
Copy link
Contributor Author

eiixy commented Aug 14, 2024

@eiixy thanks for pulling this together quickly. My feedback is centered around GenerateSchemaForType.

  1. The name feels a little misleading to me, because it implies that you would pass the type corresponding to the schema you want to generate, but instead it expects an object of that type. Seems like either the function name or the parameter type should change so they agree. I defer to @sashabaranov since the name was his suggestion and it's his package.
  2. Do we still need SchemaWrapper and its Wrap method if we have GenerateSchemaForType? It's not clear to me when you would use SchemaWrapper and it feels like an unnecessarily abstraction of the native JSON package.

jsonschema.Wrap is indeed not necessary; it's just an alternative way of doing it. You can refer to the example below.

type Result struct {
	Steps []struct {
		Explanation string `json:"explanation"`
		Output      string `json:"output"`
	} `json:"steps"`
	FinalAnswer string `json:"final_answer"`
}
sw, err := jsonschema.Wrap(Result{})
if err != nil {
	log.Fatalf("JSONSchema Wrap error: %v", err)
}
resp, err := client.CreateChatCompletion(ctx, openai.ChatCompletionRequest{
	Model: openai.GPT4oMini,
	Messages: []openai.ChatCompletionMessage{
		{
			Role:    openai.ChatMessageRoleSystem,
			Content: "You are a helpful math tutor. Guide the user through the solution step by step.",
		},
		{
			Role:    openai.ChatMessageRoleUser,
			Content: "how can I solve 8x + 7 = -23",
		},
	},
	ResponseFormat: &openai.ChatCompletionResponseFormat{
		Type: openai.ChatCompletionResponseFormatTypeJSONSchema,
		JSONSchema: &openai.ChatCompletionResponseFormatJSONSchema{
			Name:   "math_reasoning",
			Schema: sw,
			Strict: true,
		},
	},
})
if err != nil {
	log.Fatalf("CreateChatCompletion error: %v", err)
}
result, err := sw.Unmarshal(resp.Choices[0].Message.Content)
if err != nil {
	log.Fatalf("Unmarshal error: %v", err)
}

@gspeicher
Copy link

Taking your example and converting it to use GenerateSchemaForType, it seems like the only difference would be these two lines:

sw, err := jsonschema.Wrap(Result{})
// check for errors and call the API
result, err := sw.Unmarshal(resp.Choices[0].Message.Content)

would become:

var result Result
sw, err := jsonschema.GenerateSchemaForType(result)
// check for errors and call the API
err := json.Unmarshal([]byte(resp.Choices[0].Message.Content), &result)

Again, this is not my library so I defer to @sashabaranov but I personally don't see the point in including that alternative since the standard encoding/json library is already simple and ubiquitous, not to mention the complexity and cognitive load are effectively the same between the two options. Overall, adding this as an alternative actually adds unnecessary complexity to the package.

Having said that, I'm chomping at the bit to see this PR get merged and start using Structured Output so I'm not going to die on this hill.

@eiixy
Copy link
Contributor Author

eiixy commented Aug 14, 2024

Taking your example and converting it to use GenerateSchemaForType, it seems like the only difference would be these two lines:

sw, err := jsonschema.Wrap(Result{})
// check for errors and call the API
result, err := sw.Unmarshal(resp.Choices[0].Message.Content)

would become:

var result Result
sw, err := jsonschema.GenerateSchemaForType(result)
// check for errors and call the API
err := json.Unmarshal([]byte(resp.Choices[0].Message.Content), &result)

Again, this is not my library so I defer to @sashabaranov but I personally don't see the point in including that alternative since the standard encoding/json library is already simple and ubiquitous, not to mention the complexity and cognitive load are effectively the same between the two options. Overall, adding this as an alternative actually adds unnecessary complexity to the package.

Having said that, I'm chomping at the bit to see this PR get merged and start using Structured Output so I'm not going to die on this hill.

I've removed the jsonschema.SchemaWrapper. Thanks for your suggestion!

jsonschema/json.go Outdated Show resolved Hide resolved
_, ok := data.(string)
return ok
case Number: // float64 and int
_, ok := data.(float64)
Copy link
Owner

Choose a reason for hiding this comment

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

should we also support float32 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.

Great work, thank you!!

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