feat: migrate discovery call sites to native registry (Phase 4)#2534
feat: migrate discovery call sites to native registry (Phase 4)#2534Paramveersingh-S wants to merge 15 commits into
Conversation
Signed-off-by: Param <param15.veer.singh@gmail.com>
Signed-off-by: Param <param15.veer.singh@gmail.com>
Signed-off-by: Param <param15.veer.singh@gmail.com>
✅ Deploy Preview for vllm-semantic-router ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Signed-off-by: Param <param15.veer.singh@gmail.com>
👥 vLLM Semantic Team NotificationThe following members have been identified for the changed files in this PR and have been automatically assigned when their GitHub accounts are assignable in this repository: 📁
|
✅ Supply Chain Security Report — All Clear
Scanned at |
Xunzhuo
left a comment
There was a problem hiding this comment.
Requesting changes because the Phase 4 migration currently breaks the existing /models embedding inventory. No production package imports native/candle, native/onnx, or native/openvino, so their init() functions never execute and the global registry is empty. This PR also does not compile (logging is now unused), adds no API parity/regression test, inherits the earlier phase blockers, is based on main rather than Phase 3, and prematurely says Fixes #2396. Please make this a real stacked PR, use Part of #2396, explicitly assemble the available adapters, and demonstrate parity with the legacy Candle inventory before removing the direct binding path.
| modelPath := normalizeEmbeddingModelPath(model.ModelPath, model.ModelName) | ||
| if modelPath == "" { | ||
| modelPath = strings.TrimSpace(model.ModelPath) | ||
| for _, adapter := range native.Registry.List() { |
There was a problem hiding this comment.
This iterates an empty registry in production: none of the adapter packages are imported, so their init() registration is unreachable. Replacing GetEmbeddingModelsInfo() here therefore makes the endpoint return no embedding models. Please assemble/register adapters explicitly at the runtime composition seam and add an API test that loads a Candle embedding model and verifies the migrated response is non-empty and parity-compatible.
| candle_binding "github.com/vllm-project/semantic-router/candle-binding" | ||
| "github.com/vllm-project/semantic-router/src/semantic-router/pkg/modelruntime/native" | ||
| routerconfig "github.com/vllm-project/semantic-router/src/semantic-router/pkg/config" | ||
| "github.com/vllm-project/semantic-router/src/semantic-router/pkg/observability/logging" |
There was a problem hiding this comment.
This import is left behind after removing the only warning path, so go test ./pkg/apiserver fails with logging imported and not used. More importantly, the old binding error is now silently represented as an empty inventory because Adapter.Info() cannot return an error. Please extend the contract to preserve discovery errors/status and retain observable warning behavior rather than treating failure as “zero models”.
| } | ||
|
|
||
| isMatryoshka := "false" | ||
| if adapter.Capabilities().Features["matryoshka_2d"] { |
There was a problem hiding this comment.
matryoshka_2d is being read as a backend-wide property, but one backend can host models with different dimensional capabilities. This metadata must come from the individual ModelInfo (or equivalent per-model metadata), otherwise mixed inventories will report incorrect support.
Signed-off-by: Param <param15.veer.singh@gmail.com>
Signed-off-by: Param <param15.veer.singh@gmail.com>
Signed-off-by: Param <param15.veer.singh@gmail.com>
Signed-off-by: Param <param15.veer.singh@gmail.com>
Signed-off-by: Param <param15.veer.singh@gmail.com>
Signed-off-by: Param <param15.veer.singh@gmail.com>
Signed-off-by: Param <param15.veer.singh@gmail.com>
Signed-off-by: Param <param15.veer.singh@gmail.com>
Signed-off-by: Param <param15.veer.singh@gmail.com>
Signed-off-by: Param <param15.veer.singh@gmail.com>
5202905 to
c34dc81
Compare
|

Overview
This is Phase 4 of the staged rollout for Issue #2396. This PR successfully decouples the API server's model discovery logic from the legacy CGO bindings.
Key Changes
apiserver/route_model_info_embeddings.go): Completely removed the hard-coded dependency oncandle_binding.GetEmbeddingModelsInfo(). The API endpoint now correctly iterates overnative.Registry.List()and queriesadapter.Info()to assemble its response. Any new backend added to the registry automatically appears in the/modelsendpoint response.Fixes #2396 (fixes part of it)