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: [Go] added middleware framework for actions and models #429

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

pavelgj
Copy link
Collaborator

@pavelgj pavelgj commented Jun 18, 2024

Middleware allows providing reusable request/response pre/post processors. On the JS side we use middleware for polyfiling various model behaviours like system prompt and fetching media from URLs.

@pavelgj pavelgj changed the title feat: [Go] added middleware fremework for actions and models feat: [Go] added middleware framework for actions and models Jun 18, 2024
@pavelgj pavelgj marked this pull request as ready for review June 18, 2024 21:09
go/core/action.go Outdated Show resolved Hide resolved
go/core/action.go Outdated Show resolved Hide resolved
go/core/action.go Outdated Show resolved Hide resolved
@@ -28,6 +28,26 @@ import (
"github.com/invopop/jsonschema"
)

// Middleware is a functions that take in the request object, response object,
Copy link
Contributor

Choose a reason for hiding this comment

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

In general I think @jba 's goal was for the core package to be for plugins and the genkit and ai packages to be for users. This makes me wonder who is going to write Middleware values: the user or plugins?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Middleware is a feature of the core framework at the action level and ai package will contain a couple of common model middlewares (see: https://github.com/firebase/genkit/blob/main/js/ai/src/model/middleware.ts).
There are ideas to potentially implement some retriever middleware as well (for advances RAG patterns).

Copy link
Contributor

Choose a reason for hiding this comment

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

What I mean is: which code is going to pass the slice of middleware functions? Will an application that uses Genkit be writing its own middleware functions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, middleware functions are not meant to be written by the application/plugin developer (it's OK if they want to -- middleware is a powerful design pattern), but meant to be used by them. Here's an example of reusable middleware used to add common features to models: https://github.com/firebase/genkit/blob/main/js/plugins/vertexai/src/gemini.ts#L450-L464

go/core/action.go Outdated Show resolved Hide resolved
go/core/action.go Show resolved Hide resolved
go/core/action.go Outdated Show resolved Hide resolved
@@ -178,7 +215,10 @@ func (a *Action[In, Out, Stream]) Run(ctx context.Context, input In, cb func(con
}
var output Out
if err == nil {
output, err = a.fn(ctx, input, cb)
dispatch := chainMiddleware(a.middleware...)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm missing something, but I don't think we need to pass down the middlewares as a separate thing.

Say we changed middleware handlers so they have the same type as the action function itself, by including the callback argument.

Then chaining the handlers followed by the action function doesn't have to happen here, it could happen higher up.
So instead of

DefineAction(..., Middlewares(h1, h2), f)

we could write

DefineAction(..., h1(h2(f)))

This effectively removes the machinery of middlewares from the implementation. A middleware handler is just a function from action-functions to action-functions. We can keep the MiddlewareHandler type and perhaps provide a couple of common ones, but I don't think we need the rest of it.

func DefineStreamingAction[In, Out, Stream any](provider, name string, atype atype.ActionType, metadata map[string]any, fn Func[In, Out, Stream]) *Action[In, Out, Stream] {
return defineStreamingAction(globalRegistry, provider, name, atype, metadata, fn)
func DefineStreamingAction[In, Out, Stream any](provider, name string, atype atype.ActionType, metadata map[string]any, middleware []Middleware[In, Out, Stream], fn Func[In, Out, Stream]) *Action[In, Out, Stream] {
return defineStreamingAction(globalRegistry, provider, name, atype, metadata, middleware, fn)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still unclear about why the middlewares are getting passed all the way down. I can just apply them to the action function at the top level. I don't think anything in action.go needs to change.

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.

None yet

3 participants