feat: introduce native model runtime contract - #2530
Conversation
✅ Deploy Preview for vllm-semantic-router ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
Bot
assigned
AayushSaini101,
drivebyer,
FAUST-BENCHOU,
ramkrishs,
shraderdm and
WUKUNTAI-0211
👥 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 |
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>
595306e to
023ef3d
Compare
Signed-off-by: Param <param15.veer.singh@gmail.com>
Xunzhuo
left a comment
Xunzhuo
left a comment
There was a problem hiding this comment.
Thanks for splitting the original refactor. Keeping Phase 1 additive is the right boundary, and go test -race ./pkg/modelruntime/native passes on this head. I am requesting changes because the foundational contract still cannot support a backend-neutral request path, and downstream phases already expose the gaps.
Please fix this phase before rebasing the later phases:
- Use a module-prefixed title such as
[Router] .... - Replace
Fixes #2396withPart of #2396; Phase 1 does not satisfy the roadmap issue and must not close it when merged. - Make #2532 base on this branch, #2533 on #2532, and #2534 on #2533 (or land/rebase them sequentially). Today all four target
mainand contain independently recreated Phase 1 commits, so their diffs overlap and cannot be merged as a clean stack.
The inline comments cover the contract, normalized model-info shape, registry semantics, and tests. Once those are addressed, this additive first slice can be reviewed independently of the backend migrations.
| type BackendAdapter interface { | ||
| Name() Backend | ||
| Capabilities() CapabilitySet | ||
| LoadModel(ctx context.Context, req LoadRequest) (ModelHandle, error) |
There was a problem hiding this comment.
[P1] The neutral contract has no inference operation. InferenceRequest is declared, but neither BackendAdapter nor ModelHandle can execute it and there is no response type. A migrated caller would still have to import a concrete binding after LoadModel, defeating the issue's core goal that request paths ask for a capability/model ref rather than a backend package. Please put inference on the handle or adapter (with a handle argument), and use typed capability-specific request/response shapes instead of an unconsumed map[string]interface{} bag.
| IsLoaded bool | ||
| MaxSequenceLength int | ||
| DefaultDimension int | ||
| } |
There was a problem hiding this comment.
[P1] This is not yet the normalized model-info contract required by #2396 or consumed by Phase 4. It omits artifact format, modality, dimensions/layers as requested-vs-runtime metadata, provider/device, version, runtime feature flags/unsupported reasons, and registry metadata; it also allows only one capability. If this foundational type lands now, the API migration will either lose existing information or require an immediate breaking expansion. Please define the complete normalized discovery shape in Phase 1 and add round-trip/discovery tests before call sites depend on it.
| r.mu.RLock() | ||
| defer r.mu.RUnlock() | ||
| list := make([]BackendAdapter, 0, len(r.adapters)) | ||
| for _, adapter := range r.adapters { |
There was a problem hiding this comment.
[P1] Iterating a Go map makes List() nondeterministic. Phase 4 emits /models in exactly this order, so identical processes can return different response ordering and order-sensitive tests/clients will flake. Return adapters in a stable backend order (or sort the collected keys) and assert it with multiple registrations.
| func (r *AdapterRegistry) Register(adapter BackendAdapter) { | ||
| r.mu.Lock() | ||
| defer r.mu.Unlock() | ||
| r.adapters[adapter.Name()] = adapter |
There was a problem hiding this comment.
[P2] Registration has no invalid/duplicate contract: a typed-nil adapter can panic at Name(), and a second adapter silently replaces the first backend. Since registration becomes the composition boundary, please return an error for nil/empty names and duplicate backends (or document and test an intentional replacement API separately). The current concurrency smoke test deliberately avoids the nil case but does not validate registry composition semantics.
| "testing" | ||
| ) | ||
|
|
||
| // In a real scenario, this would import C and use unsafe.Sizeof to verify structs against C headers. |
There was a problem hiding this comment.
[P2] This file is explicitly a placeholder and does not test FFI layout or discriminants, so it cannot be presented as the ABI drift coverage requested by #2396. Either add real Go/Rust/C boundary checks (for example generated discriminants plus unsafe.Sizeof/offset assertions against the exported C structs) or rename this to a registry/taxonomy unit test and leave ABI coverage to the Rust-binding phase. A test that only compares a Go string constant to a literal will not catch binding drift.
674ed9a to
ad61a35
Compare
|
Thanks for the detailed review! I've updated the PR title and description as requested. I've also pushed a new commit that addresses all your points:
The base contracts are fully updated and tested. I'll make sure to sequentially rebase the other phases |
Xunzhuo
left a comment
Xunzhuo
left a comment
There was a problem hiding this comment.
Thanks for the update. The latest head addresses most of the original feedback: inference is now part of the adapter contract, ModelInfo is expanded, list ordering is deterministic, registration rejects ordinary nil/empty/duplicate inputs, and the placeholder ABI test is gone.
Changes are still required before this contract becomes the base of the remaining phases:
- EmbeddingRequest must carry the per-inference dimension/layer options supported by current Candle APIs; load-time fields are not equivalent.
- Info() must return an error so backend discovery failures do not silently become an empty model list.
- Register must reject typed-nil adapters, with a regression test.
- Fix the failing gofumpt check.
Please also use a [Router] title, change Fixes #2396 to Part of #2396, remove the obsolete ffi_layout_test.go description, and make the Phase 2–4 PR bases reflect the actual stack before further review.
Overview This is Phase 1 of splitting PR vllm-project#2458 into smaller, staged, and behavior-preserving pull requests. Part of vllm-project#2396 Signed-off-by: Param <param15.veer.singh@gmail.com>
ffebe06 to
16d9530
Compare
Xunzhuo
added
the
pr/blocked
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2530 +/- ##
=======================================
Coverage ? 33.93%
=======================================
Files ? 20
Lines ? 2959
Branches ? 0
=======================================
Hits ? 1004
Misses ? 1849
Partials ? 106
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
github-actions
Bot
added
the
wg/router-models-runtime-ecosystem
9 participants
Overview
This is Phase 1 of splitting PR #2458 into smaller, staged, and behavior-preserving pull requests to address Issue #2396.
As requested in previous reviews, this PR focuses strictly on introducing the backend-neutral native runtime contracts and taxonomy. It does not touch any existing call sites or Rust logic, ensuring that the legacy
candle-bindingsingletons and existing runtime behaviors are 100% preserved.Key Additions
taxonomy.go): Established strictBackend,Capability,Family,ArtifactFormat, andModalityconstants to eliminate string duplication and prevent future taxonomy drift.adapter.go,request.go): Defined theBackendAdapterinterface that future backend adapters (Candle, ONNX, OpenVINO) will implement. IntroducedCapabilitySet,LoadRequest, andInferenceRequestto formalize hardware and feature support.registry.go): Introduced the centralnative.Registrycomponent to track loaded model bindings securely.ffi_layout_test.go): Added stub test files for catching layout/taxonomy drifts at the CGO boundary.This PR only adds the new foundational types to
src/semantic-router/pkg/modelruntime/native. Because no existinginit()side effects, Rust code, or CGO calls were modified, this guarantees that we bypass the RustLazyLockand Go NUL-byte CI failures encountered previously.Fixes #2396 (part of 2396)