-
Notifications
You must be signed in to change notification settings - Fork 150
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 themetadata
parameter typeany
), 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:
![image](https://private-user-images.githubusercontent.com/774654/328961323-531d20bc-7e22-40ea-a535-f8873e72003a.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkwOTkwMjYsIm5iZiI6MTczOTA5ODcyNiwicGF0aCI6Ii83NzQ2NTQvMzI4OTYxMzIzLTUzMWQyMGJjLTdlMjItNDBlYS1hNTM1LWY4ODczZTcyMDAzYS5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjA5JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIwOVQxMDU4NDZaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1kYjdkNzdkYTc0ZjI1MjE2ZDYxZTlhZjg5Y2RjZDQ4ZGJhZDY4NWRjYjA1MjAxMzllZjNkYjkxNGRiNGUxZTI3JlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.o5C6BTgiNqJ5neecD80QTrr3y2wOwVHQirYtnQgarYM)
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:genkit/js/ai/src/generate.ts
Line 547 in 9445f92
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 toNewAction
should be typeany
, 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>
):genkit/genkit-tools/common/src/types/action.ts
Line 53 in 9445f92
genkit/genkit-tools/reflectionApi.yaml
Line 68 in 9445f92
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 amap[string]any
, or they can pass in a struct that is JSON-marshalable (like the GeneratorCapabilities one you've defined). Both amap[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.
Go code might need/want to use it. This example:
genkit/js/ai/src/generate.ts
Line 547 in 9445f92
Maybe in that case it could be added to the Generator interface?
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 ofNewAction
by typeany
. 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 aGenerator
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