nic-6443 commented on code in PR #13676:
URL: https://github.com/apache/apisix/pull/13676#discussion_r3575795850
##########
apisix/plugins/ai-proxy/schema.lua:
##########
@@ -303,7 +360,35 @@ _M.ai_proxy_multi_schema = {
properties = {
algorithm = {
type = "string",
- enum = { "chash", "roundrobin" },
+ enum = { "chash", "roundrobin", "semantic" },
+ description = "Load-balancing algorithm. Note: 'semantic' "
+ .. "picks an instance by prompt similarity and does
not "
+ .. "participate in health checks or fallback_strategy
/ "
+ .. "retry — an upstream failure on the chosen instance
is "
+ .. "returned to the client. It only falls back (to the
"
+ .. "catchall, else the first instance) when no
instance "
+ .. "clears its threshold or embedding fails.",
+ },
+ threshold = {
+ type = "number",
+ description = "Global minimum cosine similarity for the "
+ .. "semantic algorithm. When no instance clears its "
+ .. "threshold the request falls back to the catchall "
+ .. "instance, or the first instance if none is set.",
+ },
+ aggregation = {
+ type = "string",
+ enum = { "avg", "max", "min" },
Review Comment:
Would it make sense to drop `avg`/`min` here and just keep `max` (ideally as
the fixed behaviour, not a knob)?
`max` is what the closest prior art does — LiteLLM's auto-routing hardcodes
MAX aggregation for exactly this reason: "a strong match on one keyword in a
tier is not diluted by that tier's other utterances." `avg` reintroduces that
dilution: an instance with one dead-on example plus a couple of loosely-related
ones scores *lower* than an instance whose examples are all
mediocre-but-consistent, which is usually the opposite of what you want for
intent routing.
And since `avg` is the default, the out-of-the-box behaviour is the diluted
one — a route that works great with two tight examples can start mis-routing
the moment someone adds a third, broader example.
If there's a concrete use case for `avg`/`min` I'm happy to be wrong, but
otherwise a single `max` (no enum, no default to reason about) is simpler and
matches what people expect from semantic routing. What do you think?
##########
apisix/plugins/ai-providers/azure-openai.lua:
##########
@@ -32,6 +32,7 @@ return require("apisix.plugins.ai-providers.base").new(
path = "/completions",
rewrite_request_body = rewrite_chat_request_body,
},
+ ["openai-embeddings"] = { path = "/embeddings" },
Review Comment:
Heads up that this `/embeddings` path probably won't take effect for
`azure-openai`. In `ai-providers/base.lua`, `build_request` resolves the path
as `parsed_url.path or opts.target_path` — so once the endpoint URL carries a
path (which it must for azure, e.g. `.../openai/deployments/{deployment}`),
`parsed_url.path` wins and this capability path is dropped. Azure also needs
the `?api-version=` query param, which nothing on this path adds.
The doc example for `embeddings.endpoint` stops at
`/openai/deployments/{deployment}` (no `/embeddings`, no `api-version`), so
following the docs a semantic route with `provider: azure-openai` would POST to
the deployment root and likely 404. All the routing tests use `provider:
openai`, so this branch isn't exercised.
Could you either (a) require the azure `embeddings.endpoint` to include the
full `/embeddings?api-version=...` and document that, or (b) construct the
azure embeddings URL the same way the chat path does — and add one azure
embeddings test so it doesn't silently regress? openai works today, but azure
looks unverified.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
