feat: migrate Candle adapter and Rust registry (Phase 2)#2532
feat: migrate Candle adapter and Rust registry (Phase 2)#2532Paramveersingh-S wants to merge 10 commits into
Conversation
✅ Deploy Preview for vllm-semantic-router ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
👥 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: 📁
|
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>
✅ Supply Chain Security Report — All Clear
Scanned at |
a2be10f to
2cad810
Compare
Signed-off-by: Param <param15.veer.singh@gmail.com>
Xunzhuo
left a comment
There was a problem hiding this comment.
Phase 2 is not behavior-preserving yet. The new Go adapter reports successful lifecycle operations without touching Candle, and the Rust registry is only used by GlobalStateManager, while the production FFI init/classify paths still read their existing role-specific OnceLock globals. This recreates a second state system instead of migrating the real one.
Please first make this a real stack on #2530 and resolve the Phase 1 contract review. Also change Fixes #2396 to Part of #2396. For this phase specifically, wire a real Candle lifecycle with parity tests, or keep the adapter unregistered and return explicit unsupported errors until it is real; do not report success for no-ops.
The branch is not at a runnable boundary today: go test ./pkg/modelruntime/native/... fails on the unused fmt import, cargo fmt --all -- --check fails in all three touched Rust files, and there are no new Rust registry/state-manager tests or Candle adapter tests. Please run the reported test-binding-minimal/test-binding-lora gates after the lifecycle is wired.
|
|
||
| let _lock = self.initialization_lock.lock().map_err(|e| e.to_string())?; | ||
| *self.system_state.write().unwrap() = SystemState::Initializing; | ||
| get_registry().register("unified_classifier", classifier)?; |
There was a problem hiding this comment.
[P1] This does not migrate the state used by production FFI calls. ffi/init.rs, ffi/classify.rs, ffi/embedding.rs, the generative modules, and model architecture modules still own and read their existing OnceLock globals; repository search finds no production call from those paths into GlobalStateManager. Only the manager/tests use this new registry. As a result cleanup() can report Uninitialized after removing registry entries while the actual classifiers/embedders remain loaded in the old globals. Please route the real init/get/cleanup paths through one authoritative registry (with parity tests), rather than introducing a parallel state store.
|
|
||
| // E.g. candle_binding.InitModel(...) would happen here in a full migration. | ||
| // For now, this serves as the adapter entry point. | ||
| return &candleHandle{id: req.ModelRef}, nil |
There was a problem hiding this comment.
[P1] LoadModel returns a successful handle without validating the request or invoking any Candle initializer, and UnloadModel below is also a successful no-op. Once a caller uses this adapter it will believe a model is loaded even though no runtime state changed. This is the same placeholder behavior called out on #2458. Please delegate each supported capability to the existing binding and verify load/info/infer/unload parity; until then return an explicit unsupported/not-implemented error and do not advertise the capability as operational.
|
|
||
| func init() { | ||
| // Register the Candle adapter automatically | ||
| native.Registry.Register(NewAdapter()) |
There was a problem hiding this comment.
[P1] This init() never runs in the router: no production package imports pkg/modelruntime/native/candle, and importing only the parent native package does not initialize subpackages. Therefore native.Registry remains empty, which Phase 4 then treats as “no embedding models”. Prefer explicit adapter composition at the runtime assembly boundary (respecting build tags), and add a test against the real binary composition that Candle is registered exactly once; relying on an unimported side-effect package is not sufficient.
| parallel_lora_engine_initialized: self.get_parallel_lora_engine().is_some(), | ||
| lora_token_classifier_initialized: self.get_lora_token_classifier().is_some(), | ||
| bert_similarity_initialized: self.get_bert_similarity().is_some(), | ||
| legacy_classifiers_count: 0, |
There was a problem hiding this comment.
[P2] This permanently reports zero legacy classifiers even after init_legacy_bert_* registered one or more entries, regressing the existing stats contract. The centralized registry needs typed inventory/count support (or the manager must track the registered keys) so get_stats() remains truthful. Please cover 0/1/multiple legacy registrations and cleanup in tests.
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>
96824f5 to
c057e83
Compare
|
Thanks for the thorough review! I've updated the branch to completely resolve all of these points:
|

Overview
This is Phase 2 of the staged rollout for Issue #2396. This PR targets the Rust core to eliminate the CI failures seen in the original PR while laying the Go adapter groundwork for the default backend.
Key Changes
candle-binding/src/registry.rs): Replaced the failingLazyLock(which broke the Rust 1.7x CI pipelines) with a backwards-compatiblestd::sync::OnceLock. The registry now seamlessly downcastsArc<dyn Any>types securely.candle-binding/src/ffi/state_manager.rs): Ripped out the sprawling, role-specific singletons (e.g.,RwLock<Option<Arc<ParallelLoRAEngine>>>) and refactoredGlobalStateManagerto delegate all state-keeping to the new centralizedModelRegistry.native/candle/adapter.go): Created theBackendAdapterimplementation for the Candle engine. Explicitly wired up the capability manifest (CapabilitySet), declaring precisely what Candle can do (e.g., embeddings, LoRA, guard, batching) and what it cannot do. Hooked it intonative.Registryautomatically viainit().Note on Bisectability
To maintain bisectability and zero disruption, the legacy inference pipelines in Go have not been pointed to the registry yet (that happens in Phase 5). Existing tests will continue to pass untouched.
Fixes #2396 (solves part of it)