-
Notifications
You must be signed in to change notification settings - Fork 464
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
[WIP] Add support for AI APIs #10493
base: main
Are you sure you want to change the base?
[WIP] Add support for AI APIs #10493
Conversation
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.
Overall this make sense to me. I have some nits about structure, but my big takeaways are:
- What's the decision process between pointer and non-pointer types throughout the API, it's not clear to me.
- Why are we using
iota
for enums in some place, andstring
aliases in others? - The
AIRouteOptions
need more validation akin to the upstream.
// Enrich requests sent to the LLM provider by appending and prepending system prompts. | ||
// This can be configured only for LLM providers that use the `CHAT` API route type. | ||
PromptEnrichment AIPromptEnrichment `json:"prompt_enrichment,omitempty"` | ||
|
||
// Set up prompt guards to block unwanted requests to the LLM provider and mask sensitive data. | ||
// Prompt guards can be used to reject requests based on the content of the prompt, as well as | ||
// mask responses based on the content of the response. | ||
PromptGuard AIPromptGuard `json:"prompt_guard,omitempty"` |
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.
Just out of curiosity, why aren't these pointers? Is there default behavior? If the user leaves these out I think we should do nothing?
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.
Yep, omitempty
should make them not required. I think you're right we do want it to be a pointer to be able to check if it's set or nil in the plugin. A lot of these fields were copied over from the proto API, so in general I think the decision process between pointer vs. non-pointer types should just be if this an object that's optional/can be unset.
type AIUpstream struct { | ||
// Send requests to a custom host and port, such as to proxy the request, | ||
// or to use a different backend that is API-compliant with the upstream version. | ||
CustomHost Host `json:"customHost,omitempty"` |
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.
Why is this at the top level and how is it different from the one located on the individual backend.
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.
The top level one is on the static upstream, this is a different upstream type but reuses the Host field here.
This was just from: https://github.com/solo-io/gloo/blob/4a5953a94de16f26e0284e8aec18d6f997227671/projects/gloo/api/v1/enterprise/options/ai/ai.proto#L62
20c2d0f
to
0f53176
Compare
Description
Adds support for AI APIs: #10494
API changes
Introduces AI APIs in kgateway. This is based on the Gloo AI APIs.
The HTTPRoute selects an Upstream via a targetRef:
Code changes
Plugin and extauth changes will be implemented in a follow up. This PR only introduces the AI APIs.
CI changes
NONE
Docs changes
NONE
Context
Enhancement Proposal: #10495
Testing steps
Manually validated CR validation by install helm chart and applying example configs.
Checklist: