-
Notifications
You must be signed in to change notification settings - Fork 240
feat(go/genkit): add DefineSchema and Dotprompt reference support #2769
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
Conversation
go/genkit/schema.go
Outdated
// | ||
// personSchema := genkit.DefineSchema("Person", Person{}) | ||
func DefineSchema(name string, schema Schema) Schema { | ||
if name == "" { |
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.
Doubt, if core.RegisterSchema already has those validations, is it okay to have them also here? Just learning :D
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.
You raise an interesting point, genkit.DefineSchema not only calls core.RegisterSchema but also adds to pendingSchemas. refactor to remove duplication of validation is a good idea, I'm thinking to refactor pendingSchemas to core package.
go/genkit/schema.go
Outdated
} | ||
|
||
// registerSchemaResolver registers a schema resolver with Dotprompt to handle schema lookups | ||
func registerSchemaResolver(dp *dotprompt.Dotprompt) { |
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.
what is the purpose of the private function? Maybe the Public one (RegisterGlobalSchemaResolver) could instance the private one (registerSchemaResolver) to factorize code, what are your thoughts about it?
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.
Good observation! and your suggestion follows DRY (Don't Repeat Yourself) principle. I'll expose it out by adding func RegisterGlobalSchemaResolver(dp *dotprompt.Dotprompt) { registerSchemaResolver(dp) }
go/internal/registry/schema.go
Outdated
} | ||
|
||
r.Dotprompt.DefineSchema(name, jsonSchema) | ||
r.RegisterValue("schema/"+name, structType) |
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.
what about adding 'schema/' as a const?
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.
EditGood suggestion! I'll add a const SchemaPrefix = "schema/" .
go/internal/registry/schema.go
Outdated
|
||
r.Dotprompt.DefineSchema(name, jsonSchema) | ||
r.RegisterValue("schema/"+name, schema) | ||
r.setupSchemaLookupFunction() |
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.
maybe RegisterSchemaWithDotprompt also could call DefineSchema and only ejecute r.setupSchemaLookupFunction()
after calling it and validating there is no error.
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.
Good idea! I'll update the code to call setupSchemaLookupFunction() only after successful schema registration to ensure proper error handling.
go/internal/registry/schema.go
Outdated
schema.Properties = orderedmap.New[string, *jsonschema.Schema]() | ||
schema.Required = []string{} | ||
|
||
for i := 0; i < t.NumField(); i++ { |
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.
Maybe this part of code could be factorized in another function to be recursively? Well, supporting more nesting levels could be another issue for itself. Let me know what you think! :D
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.
I agree this section could benefit from refactoring! I'll create a helper function to handle the field-to-schema conversion recursively, which would improve code organization and support nested struct types more elegantly.
Tests are not passing, can you resolve that first? |
go/core/schema.go
Outdated
// SchemaType is the type identifier for schemas in the registry. | ||
const SchemaType = "schema" | ||
|
||
// schemaRegistry maintains registry of schemas. |
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.
Is this comment needed here?
go/core/schema.go
Outdated
schemasMu.RLock() | ||
defer schemasMu.RUnlock() | ||
|
||
// First check local registry |
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.
This code is pretty clear to read, these comments are not needed at all.
A good suggestion from Alex was that we should remove the comments that explains what
and only keep the comments that explains why
go/core/schema.go
Outdated
|
||
// GetPendingSchemas returns a copy of pending schemas that need to be | ||
// registered with Dotprompt. | ||
func GetPendingSchemas() map[string]Schema { |
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.
func GetPendingSchemas() map[string]Schema { | |
func PendingSchemas() map[string]Schema { |
go/core/schema.go
Outdated
} | ||
|
||
// GetSchemas returns a copy of all registered schemas. | ||
func GetSchemas() map[string]any { |
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.
func GetSchemas() map[string]any { | |
func Schemas() map[string]any { |
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.
In general, avoid having function names with Get/Set prefixes 👍🏽
go/genkit/schema.go
Outdated
|
||
// GetSchema retrieves a registered schema by name. | ||
// It returns an error if no schema exists with that name. | ||
func GetSchema(name string) (Schema, error) { |
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.
What's the reason behind having GetSchema
if it internally calls LookupSchema
?
go/genkit/schema.go
Outdated
|
||
// LookupSchema retrieves a registered schema by name. | ||
// It returns nil and false if no schema exists with that name. | ||
func LookupSchema(name string) (Schema, bool) { |
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.
Wouldn't be cleaner and easier to use if the return value would be to return Schema
if found and nil
if not found?
go/genkit/schema.go
Outdated
return nil | ||
} | ||
|
||
// Convert the schema to a JSON schema |
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.
nit: not needed, same for comment in line 113
go/internal/registry/schema.go
Outdated
|
||
schema := &jsonschema.Schema{ | ||
Type: "object", | ||
Properties: orderedmap.New[string, *jsonschema.Schema](), |
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.
Curious, why do we need an ordered map?
go/core/schema.go
Outdated
|
||
// ClearSchemas removes all registered schemas. | ||
// This is primarily for testing purposes. | ||
func ClearSchemas() { |
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.
Where's this function used?
go/internal/registry/schema.go
Outdated
} | ||
|
||
// convertStructToJsonSchema converts a Go struct to a JSON schema | ||
func convertStructToJsonSchema(structType any) (*jsonschema.Schema, error) { |
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.
Wouldn't it be better if we use map[string]any
as the type instead of just any?
This PR adds a schema definition system to genkit with Dotprompt reference support.
Key Components
DefineSchema
API for defining prompt data structuresExample Usage