feat: configure and provision local embedding models - #51
Conversation
e6cf3dd to
447f21c
Compare
dae05b3 to
394af11
Compare
sjvans
changed the title
|
Summary of the update:
Verification on commit
|
PDT42
left a comment
PDT42
left a comment
There was a problem hiding this comment.
IMO this should replace: #53
- The model configuration should be updated to require only,
model, local dir where to store the model,normalization,pooling; could consider keeping the others for consistency checks - I'd like more readable test titles, please 😅; Currently the titles don't tell me what the test expects to happen and doesn't allow me to agree or disagree with that expectation
|
After some confusion about the See DetailsEdited the ai node module with:dbc.function('COSINE_SIMILARITY', deterministic, (a, b) => cosineSimilarity(toFloatArray(a), toFloatArray(b)))
...
function toFloatArray(vector) {
if (vector == null) return null
if (vector instanceof Float32Array) return Array.from(vector)
if (Buffer.isBuffer(vector)) return JSON.parse(vector.toString('utf8'))
if (vector instanceof Uint8Array) return JSON.parse(Buffer.from(vector).toString('utf8'))
if (typeof vector === 'string') return JSON.parse(vector)
if (Array.isArray(vector)) return vector
throw new Error(`Unsupported vector type: ${typeof vector}`)
}
function cosineSimilarity(a, b) {
if (a == null || b == null) return null
let dot = 0, normA = 0, normB = 0
for (let i = 0; i < a.length; i++) {
dot += a[i] * b[i]
normA += a[i] * a[i]
normB += b[i] * b[i]
}
const denom = Math.sqrt(normA) * Math.sqrt(normB)
return denom === 0 ? 0 : dot / denom
}I was able to make this work in https://github.tools.sap/cap/know-cap/pull/20 👌 |
44dafe9 to
55ddfdb
Compare
* feat: add explicit embedding model provisioning * feat: support lazy embedding model provisioning * docs: explain embedding model provisioning * fix: require explicit embedding model * refactor: require provisioned embedding models * feat: provision embedding models by name
55ddfdb to
2693601
Compare
sjvans
changed the title
SummaryThe following content is AI-generated and provides a summary of the pull request: feat: Configure and provision local embedding modelsThis PR overhauls the 🔑 Key ChangesRuntime configuration is now limited to {
"cds": {
"requires": {
"db": {
"kind": "ai-sqlite",
"embedding": { "model": "foo/bar" }
}
}
}
}Model provisioning supports two modes:
🏗️ Architecture Changes
✅ Compatibility
Have you...
PR Bot InformationVersion:
|
![hyperspace-pr-bot[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR introduces a well-structured model provisioning system with strong integrity guarantees, but there are a few substantive issues: the module-level singleton in createSession/embedding is incompatible with the new per-service runtime design and will silently misbehave in multi-service scenarios; ensureDirectory will throw ENOENT when the target directory doesn't yet exist; and the installOptions.directory forwarding is fragile due to double-resolution of the path. Please also address the existing reviewer comments about validateModelDescriptor being too strict for user-facing runtime config, and the @huggingface/tokenizers optional peer dependency noted in the package.json thread.
PR Bot Information
Version: 1.29.54
- Event Trigger:
pull_request.ready_for_review - LLM:
anthropic--claude-4.6-sonnet - Correlation ID:
5f91bc50-a217-11f1-875d-3b7d143453d0 - File Content Strategy: Full file content
| let sessionRuntime; | ||
| let sessionInitialization; | ||
|
|
||
| async function createSession() { | ||
| const modelDir = getModelDir(); | ||
| await downloadModelIfNeeded(modelDir, MODEL); | ||
| ({ session, tokenizer } = await loadModelAndTokenizer(modelDir)); | ||
| async function createSession(configuration, options) { | ||
| sessionInitialization ??= createEmbeddingRuntime(configuration, options) | ||
| .then((runtime) => (sessionRuntime = runtime)) | ||
| .catch((error) => { | ||
| sessionInitialization = undefined; | ||
| throw error; | ||
| }); | ||
| return sessionInitialization; | ||
| } |
There was a problem hiding this comment.
Bug: Module-level sessionRuntime and sessionInitialization state is shared across all callers of createSession
AISQLiteService now creates a per-service _embeddingRuntime via createEmbeddingRuntime, but createSession / embedding (used by the index.js compatibility path) still writes to module-level singletons. Calling createSession twice with different configurations (e.g., two services with different models in tests or multi-service scenarios) silently ignores the second configuration because sessionInitialization ??= … is already set. More critically, dispose() on the first runtime also sets sessionRuntime = undefined, leaving any subsequent call to embedding() throwing "session not initialized" even though a second runtime may still exist.
Consider removing the module-level singleton entirely from createSession/embedding and routing the index.js path through the per-service _embeddingRuntime held by AISQLiteService, or at minimum document that only a single global session is supported and guard against re-initialization with a different configuration.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| async function ensureDirectory(directory) { | ||
| await ensureParentDirectories(directory); | ||
| await assertModelDirectory(directory); | ||
| await fs.chmod(directory, PROVISIONED_DIRECTORY_MODE); | ||
| } |
There was a problem hiding this comment.
Bug: ensureDirectory calls assertModelDirectory after ensureParentDirectories, but ensureParentDirectories only creates the parent directories, not the directory itself. If directory does not yet exist, assertModelDirectory returns false (directory not found) and then fs.chmod is called on a path that does not exist, throwing ENOENT.
Should use fs.mkdir(directory, { recursive: true }) or call fs.mkdir for the leaf directory before assertModelDirectory.
| async function ensureDirectory(directory) { | |
| await ensureParentDirectories(directory); | |
| await assertModelDirectory(directory); | |
| await fs.chmod(directory, PROVISIONED_DIRECTORY_MODE); | |
| } | |
| async function ensureDirectory(directory) { | |
| await ensureParentDirectories(directory); | |
| await fs.mkdir(directory, { mode: PROVISIONED_DIRECTORY_MODE }).catch((error) => { | |
| if (error.code !== 'EEXIST') throw error; | |
| }); | |
| await assertModelDirectory(directory); | |
| await fs.chmod(directory, PROVISIONED_DIRECTORY_MODE); | |
| } |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| let owner; | ||
| try { | ||
| owner = JSON.parse(contents); | ||
| } catch { | ||
| if (Date.now() - stat.mtimeMs <= INSTALL_LOCK_STALE_MS) return false; | ||
| } | ||
| if (!isStaleInstallLock(owner, stat)) return false; |
There was a problem hiding this comment.
Bug: recoverStaleInstallLock falls through to isStaleInstallLock(owner, stat) even when JSON parsing failed — owner is undefined after a caught parse error. The if (!isStaleInstallLock(owner, stat)) return false; guard does handle undefined owner correctly (falls back to mtime check), but only when Date.now() - stat.mtimeMs > INSTALL_LOCK_STALE_MS. If the lock file has invalid JSON and was written recently, isStaleInstallLock returns false (not stale) and the function returns false, permanently blocking provisioning. Consider throwing or treating a corrupt recent lock as non-recoverable rather than silently stalling.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| const installOptions = { | ||
| root, | ||
| directory: modelRoot, | ||
| home: options.home, | ||
| fetchImpl, | ||
| discover, | ||
| validate: validate ?? validateEmbeddingModel, | ||
| timeoutMs: options.provisionTimeoutMs, | ||
| retryMs: options.provisionRetryMs | ||
| }; |
There was a problem hiding this comment.
Logic Error: resolveEmbeddingModel passes modelRoot as directory in installOptions, but installModel re-derives modelRoot = getModelRoot(options.directory, options.root, options.home) from it. Because modelRoot is already a fully-resolved absolute path (not the original user-supplied directory string), the ~/ and relative-path resolution branches in getModelRoot are bypassed and the paths remain consistent — but only by coincidence. If the user supplies directory: "~/.cds/models", the resolved modelRoot is /home/user/.cds/models; passing that as options.directory to installModel hits the path.resolve(root, directory) branch, which is correct only because absolute paths are returned unchanged. This is fragile. Consider passing the raw directory option through, or passing the pre-resolved modelDir directly to provisionModel.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
|
|
||
| Options: | ||
| --directory <path> Use this model-cache root instead of .cds/models | ||
| --help Show this help |
There was a problem hiding this comment.
Typo: The --help option in HELP text is indented with extra spaces ( --help Show this help) making the alignment inconsistent with --directory. This may confuse users reading the help output.
Consider aligning both options consistently.
| --help Show this help | |
| --help Show this help |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
3 participants
Summary
@huggingface/tokenizers@0.1.3@huggingface/tokenizersas an optional peer dependency and fail with an installation hint whenai-sqliteuses it without the package installedembedding: { model, directory? }<cds.root>/.cds/models/<model>after logging a warningnpx @cap-js/ai install-model <model> [--directory <root>]embedding.lock.jsoncontaining the resolved revision, files, checksums, dimensions, token limit, pooling, and normalizationRuntime configuration
{ "cds": { "requires": { "db": { "kind": "ai-sqlite", "embedding": { "model": "foo/bar" } } } } }Without
directory, the effective installation is:For a shared pre-installed cache:
npx @cap-js/ai install-model foo/bar --directory ~/.cds/models{ "embedding": { "model": "foo/bar", "directory": "~/.cds/models" } }Configured directories are verified but remain offline/read-only during application startup.
Model compatibility
onnx/model.onnx, including external ONNX data filesValidation
ai-sqlite, and knowledge-graph tests: 68/68 passingnpx -y eslint@10 .npx -y prettier@3 --check .git diff --checkStack
AISQLiteService, including feat: triple store support for@cap-js/sqlite#49's triple-store integration and fix: truncate embeddings to one model window #58's single-window behavioronnxruntime-noderemains pinned to1.20.1; broader compatibility is tracked in Support newer onnxruntime-node versions in the ai-sqlite synchronous adapter #54Category: Feature / Refactor