-
Notifications
You must be signed in to change notification settings - Fork 146
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
[Go] adding metadata to models, etc. #71
Conversation
@@ -65,21 +65,22 @@ type Action[I, O, S any] struct { | |||
// See js/common/src/types.ts | |||
|
|||
// NewAction creates a new Action with the given name and non-streaming function. | |||
func NewAction[I, O any](name string, fn func(context.Context, I) (O, error)) *Action[I, O, struct{}] { | |||
return NewStreamingAction(name, func(ctx context.Context, in I, cb NoStream) (O, error) { | |||
func NewAction[I, O any](name string, metadata map[string]any, fn func(context.Context, I) (O, error)) *Action[I, O, struct{}] { |
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.
How do you see metadata
being used? Who is the producer and who is the consumer? Typically in Go we would use specific types here (and make the metadata
parameter type any
), but if this is truly general, or user-controlled, than a map makes sense.
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.
At the action level metadata is not strictly defined, it is an arbitrary map. But at the specific action type level (model, embedder, retriever, etc.) metadata is expected to have specific structure (if it wants to be understood). This is an example of model metadata:
It is important because, for example, the model playground needs to know whether the model supports multiturn input (if it does then it will render a chat style interface) or whether it support multimodal input (it will allow inserting images).
but it's not just the Dev UI, for example the generate
function will check model supported features and throw errors if the developer is trying to do something that's not supported by that specific model:
Line 547 in 9445f92
if (!model.__action.metadata?.model.supports?.tools) { |
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.
BTW, for the generator I defined structs: https://github.com/firebase/genkit/pull/71/files#diff-c8c738aff67a5f8e34971b78f75941a99020e41e6ac1c535bef6e83b4c998278
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.
That suggests that the metadata
parameter to NewAction
should be type any
, and the actual action will type assert to the desired value.
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 the action schema it is defined as map[string]any
(Record<string, any>
):
.record(z.string(), CustomAnySchema) |
genkit/genkit-tools/reflectionApi.yaml
Line 68 in 9445f92
type: object |
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.
Without getting into too much detail, I think the question here comes down to whether this value will ever be used in Go code, or will it just be written to JSON for the benefit of the tooling?
If no Go code needs to access the metadata except to JSON-marshal it, then Ian's way is more type-safe and Go-like. With an any
parameter, a user can still pass in a map[string]any
, or they can pass in a struct that is JSON-marshalable (like the GeneratorCapabilities one you've defined). Both a map[string]any
and a struct with the right field names or struct tags will marshal to the same JSON, but the latter will be easier for Go programmers to write and won't need to be hand-converted to a map in the code.
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.
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.
Using specific types for generators, as you've already written, is consistent with having the metadata
parameter of NewAction
by type any
. Generators can type assert the metadata as needed. Perhaps I misunderstand.
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.
Yes, it should be possible to get the GeneratorCapabilities
from a Generator
without going into the action.
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 tried changing the metadata type on NewAction to any, but I don't think it's possible/easy... I'll leave that to you. :)
Basically:
https://github.com/firebase/genkit/blob/main/go/genkit/action.go#L62
and
https://github.com/firebase/genkit/blob/main/go/genkit/action.go#L173
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.
Looks basically fine to me.
I agree with Ian's comment, and it will also simplify the code.
But I think you've shown the way here, so I'm fine with you submitting as is and me fixing it up.
No description provided.