Conversation
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
|
@yuzisun PTAL |
| ModelNameHeaderKey, selectedBackendHeaderKey string | ||
| modelNameHeaderKey, selectedBackendHeaderKey string |
There was a problem hiding this comment.
note: shouldn't have been exported
| p.costs.TotalTokens, | ||
| ) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to evaluate CEL expression: %w", err) |
There was a problem hiding this comment.
Out of curiosity, what does it mean for ext_proc to fail at this stage, when the response headers may have been sent? Does it make sense to make it fail?
There was a problem hiding this comment.
this will result in connection termination i guess
|
@envoyproxy/ai-gateway-maintainers ping |
|
@envoyproxy/ai-gateway-maintainers ping |
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
| expr := *cost.CELExpression | ||
| // Sanity check the CEL expression. | ||
| _, err := llmcostcel.NewProgram(expr) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid CEL expression: %w", err) | ||
| } | ||
| fc.CELExpression = expr |
There was a problem hiding this comment.
Could we validate this in a webhook instead of controller as user is not going to get the error feedback if they specify the invalid cel expression ?
There was a problem hiding this comment.
even if that's possible, still we need to do the type validation.
There was a problem hiding this comment.
even if that's possible, still we need to do the type validation.
There was a problem hiding this comment.
let me see how much k8s API can validate a CEL expression (not sure even if it's doable)
There was a problem hiding this comment.
It is doable, need to setup a validating web hook to achieve this, we can do this post 0.1 though.
There was a problem hiding this comment.
hmm, i did a quick research but is it really possible to write a k8s validation to verify a string field content itself being a valid CEL expression?
There was a problem hiding this comment.
Essentially you add the same code in a webhook validation handler and then user can get the error feedback, though we still need this code to deal with the standalone setup with configmap.
There was a problem hiding this comment.
ah ok i see not the CRD CEL validation, but instead k8s validation webhook. let's do that later after v0.1.0
| } | ||
|
|
||
| // Sanity check by evaluating the expression with some dummy values. | ||
| _, err = EvaluateProgram(prog, "dummy", "dummy", 0, 0, 0) |
There was a problem hiding this comment.
This seems like getting called on the data path as well for each request, can we only initialize once?
There was a problem hiding this comment.
prog is initialized on the config load time which is the compiled CEL program. so i don't think we could do beyond that (the other args are depending on the request context)
There was a problem hiding this comment.
btw to be clear, this function itself NewProgram is only called on config path and prog is initialized once here. And then on the request path only EvaluateProgram is called using the precompiled prog
There was a problem hiding this comment.
You are right I read the code wrong.
✅ Deploy Preview for envoy-ai-gateway ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Thanks for the review Dan! |
This commit implements the CEL expression API
left as a TODO in #103. Closes #97.