fix: fail fast on incompatible ONNX Runtime - #731
Conversation
|
Thanks — fail-fast preflight + the CPU-EP fix are exactly the right immediate scope, and the debug-tracing breadcrumbs around 1. Clippy (also fails the Embed SDK job, which compiles the lib with 2. Format: Both resolved in one go by something like: fn validate_ort_dylib_version(path: &Path) -> anyhow::Result<()> {
// SAFETY: the path was resolved by resolve_ort_dylib; loading a shared
// library executes its initializers, which is the accepted risk of any
// dlopen-based ORT discovery (same trust boundary as ort::init_from).
let lib = unsafe { libloading::Library::new(path) }
.map_err(|e| anyhow::anyhow!("failed to load {}: {e}", path.display()))?;
// SAFETY: OrtGetApiBase is the stable C entry point every ONNX Runtime
// exports; the signature matches the ORT C API declaration.
let get_api_base: libloading::Symbol<OrtGetApiBase> = unsafe { lib.get(b"OrtGetApiBase") }
.map_err(|_| anyhow::anyhow!("{} does not export OrtGetApiBase", path.display()))?;
// SAFETY: the symbol was just resolved from the loaded library and takes
// no arguments; it returns a pointer we null-check before use.
let base = unsafe { get_api_base() };
anyhow::ensure!(
!base.is_null(),
"OrtGetApiBase returned null for {}",
path.display()
);
// SAFETY: base is non-null (checked above) and points to the static
// OrtApiBase; GetVersionString takes no arguments.
let version = unsafe { ((*base).get_version_string)() };
// SAFETY: GetVersionString returns a static NUL-terminated C string owned
// by the runtime for the lifetime of the library.
let version = unsafe { CStr::from_ptr(version) }.to_string_lossy();
let minor = version
.split('.')
.nth(1)
.and_then(|part| part.parse::<u32>().ok())
.unwrap_or(0);
...(then One substantive review note: the EP change makes deterministic mode register all compiled-in providers, not just CPU — on let eps = if deterministic {
vec![ort::ep::CPU::default().build()]
} else {
crate::core::ort_execution_providers::gpu_execution_providers()
};Default/official builds behave identically either way (no GPU features compiled in), so this only matters for feature builds — but it keeps the deterministic contract honest. With the SAFETY comments, fmt, and that one-liner this is good to merge from my side. |
- add SAFETY comments to unsafe blocks in validate_ort_dylib_version (clippy::undocumented_unsafe_blocks) - run cargo fmt on ort_environment.rs - keep deterministic embedding mode CPU-only instead of registering all compiled-in execution providers, preserving the determinism guarantee
yvgude
left a comment
There was a problem hiding this comment.
Reviewed the preflight path in detail — this is exactly the right shape. ✅
validate_ort_dylib_versionchecksOrtGetApiBase→GetVersionStringbeforeort::init_from, so an ABI-incompatible runtime turns from a hang/crash into a clear, actionable error (the message names both versions andORT_DYLIB_PATH). Theminor >= ort::MINOR_VERSIONcomparison matches ORT's compatibility rule (MINOR_VERSIONisORT_API_VERSION, runtimes are backwards-compatible).- The double
dlopen(libloading probe, thenort::init_from) is fine — the loader refcounts, and the probe shares the exact trust boundary the subsequent init has anyway. SAFETY comments are accurate. shared_engine_result()preserving the load error (instead of a bareOption) finally makesctx doctor-style diagnostics possible frombuild_or_update; theOptionwrapper keeps every existing caller untouched.- Explicit CPU EP for deterministic inference is a good hardening move (identical behavior, clearer intent).
CI is fully green across all three OS + the no-ORT build. Merging — thanks for the thorough fix, this closes a nasty failure mode for everyone on distro-packaged onnxruntime.
Summary
Fixes #730.
This PR hardens the embeddings initialization path when
ortis used withload-dynamic.It adds an eager ONNX Runtime version preflight before calling
ort::init_from, preserves embedding engine load errors so the CLI can show useful diagnostics, and ensures deterministic embeddings still register a CPU execution provider.Changes:
libloadingto theembeddingsandneuralfeature dependency sets so the runtime.socan be inspected before handing it toort.libonnxruntimeincore::ort_environment:OrtGetApiBase;GetVersionString;ort::MINOR_VERSION;ort::init_fromand ONNX Runtime environment commit.shared_engine_result()so callers that can surface diagnostics do not lose the original engine-load error.Why
lean-ctxcurrently relies on a dynamically loaded ONNX Runtime shared library. If the discovered library is missing or too old, the user needs an immediate, actionable error. In practice, an ONNX Runtime1.22.0library with a build compiled forapi-24causedort::init_fromto hang beforecommit(), which made semantic indexing appear stuck.Deterministic mode also passed zero execution providers. Once the API mismatch was removed in a probe build, session creation could crash inside ONNX Runtime because the execution-provider vector was empty. CPU should always be present as the fallback provider.
Test plan
cd rust && cargo check -q --lib --features embeddingscd rust && cargo build -q --bin lean-ctx --features 'tree-sitter embeddings http-server team-server gateway-server shape-xlat secure-update jemalloc qdrant pgvector'1.22.0fails fast with a clear version error instead of hanging.1.27.0succeeds end-to-end and persists semantic embeddings.cd rust && cargo testcd rust && cargo clippy --all-targets --all-features -- -D warningscd rust && cargo fmt --checkIncompatible runtime validation with ONNX Runtime
1.22.0:Result: no hang; reports that ONNX Runtime
1.22.0is too old for a build requiring>= 1.24.x.Compatible runtime validation with ONNX Runtime
1.27.0:Result:
Notes for reviewers
libonnxruntime, runtime version parsing, deterministic embedding mode, and CPU execution-provider fallback.dlopen; the Python wheel is only a convenient distribution source forlibonnxruntime.so, not a Python runtime dependency.Contributor License Agreement
First-time contributors: a bot will ask you to sign our one-time CLA. Reply to the PR with:
I have read the CLA Document and I hereby sign the CLA.