feat: provision embedding models by name - #55
Conversation
sjvans
changed the title
SummaryThe following content is AI-generated and provides a summary of the pull request: feat: Provision embedding models by nameNew Feature This PR replaces the hardcoded Key changes
Configuration example{
"cds": {
"requires": {
"db": {
"kind": "ai-sqlite",
"embedding": {
"model": "foo/bar",
"directory": "~/.cds/models"
}
}
}
}
}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 pull request introduces a well-structured model provisioning system with thorough test coverage. Two substantive issues were posted: ensureDirectory can throw ENOENT on chmod when the target directory doesn't yet exist (the function creates only ancestors, not the directory itself), and the module-level createSession singleton silently ignores configuration passed by any second caller, which would misroute embeddings if two AISQLiteService instances with different models are ever used in the same process. The lowercaseFirst guard comment was also noted as a minor robustness improvement.
PR Bot Information
Version: 1.29.54
- File Content Strategy: Full file content
- Correlation ID:
3b5af0d0-a213-11f1-8f9b-d6565d7ca05c - LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.ready_for_review
| 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 ensureParentDirectories(directory) to create missing ancestors and then calls assertModelDirectory(directory) expecting the directory itself to already exist — but ensureParentDirectories only creates ancestors up to (but not including) directory. If the directory does not yet exist, assertModelDirectory returns false and fs.chmod is then called on a path that doesn't exist, throwing ENOENT. The function is called from downloadModelIfNeeded and writeModelLock, so any first-time provisioning that creates the model directory hits this path.
The directory itself should be created before chmod is applied.
| 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
|
|
||
| const DOWNLOAD_TIMEOUT_MS = 5 * 60 * 1000; | ||
| const MODEL_LOCK_FILE = 'embedding.lock.json'; | ||
| const MODEL_INSTALL_LOCK_FILE = '.embedding.install.lock'; |
There was a problem hiding this comment.
Logic Error: The install-lock file name is constructed by concatenating the directory base name with MODEL_INSTALL_LOCK_FILE (.embedding.install.lock), producing e.g. .bar.embedding.install.lock. However, in the test tests/model-provisioning.test.js the expected lock path is .model.embedding.install.lock, which matches. The issue is that MODEL_INSTALL_LOCK_FILE already starts with a ., so when appended to .${basename} the result is .<basename>.embedding.install.lock — a double-dotted name that is inconsistent with how the test at line 181 constructs the path manually (path.join(parent, '.model.embedding.install.lock')). That test passes only because the constant value happens to produce that exact string. This is fine functionally, but the constant name MODEL_INSTALL_LOCK_FILE = '.embedding.install.lock' is misleading: it is a suffix, not a standalone file name. Consider renaming it to make its role as a suffix explicit, to avoid future misuse.
| const MODEL_INSTALL_LOCK_FILE = '.embedding.install.lock'; | |
| const MODEL_INSTALL_LOCK_FILE_SUFFIX = '.embedding.install.lock'; |
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
| async function createSession(configuration, options) { | ||
| sessionInitialization ??= createEmbeddingRuntime(configuration, options) | ||
| .then((runtime) => (sessionRuntime = runtime)) | ||
| .catch((error) => { | ||
| defaultInitialization = undefined; | ||
| sessionInitialization = undefined; | ||
| throw error; | ||
| }); | ||
| return defaultInitialization; | ||
| return sessionInitialization; | ||
| } |
There was a problem hiding this comment.
Logic Error: The module-level sessionRuntime and sessionInitialization variables are shared across all callers of createSession. If AISQLiteService is instantiated multiple times (e.g. two services with different models), the second call to createSession reuses the first initialization regardless of the configuration and options arguments passed, silently using the wrong model for the second service. The ??= assignment means subsequent calls with different configurations are completely ignored.
| async function createSession(configuration, options) { | |
| sessionInitialization ??= createEmbeddingRuntime(configuration, options) | |
| .then((runtime) => (sessionRuntime = runtime)) | |
| .catch((error) => { | |
| defaultInitialization = undefined; | |
| sessionInitialization = undefined; | |
| throw error; | |
| }); | |
| return defaultInitialization; | |
| return sessionInitialization; | |
| } | |
| async function createSession(configuration, options) { | |
| sessionInitialization ??= createEmbeddingRuntime(configuration, options) | |
| .then((runtime) => (sessionRuntime = runtime)) | |
| .catch((error) => { | |
| sessionInitialization = undefined; | |
| throw error; | |
| }); | |
| return sessionInitialization; | |
| } | |
| // NOTE: createSession uses module-level singletons and is only suitable for | |
| // a single embedding configuration per process. Multiple AISQLiteService | |
| // instances with different models will silently share the first runtime. |
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
| function lowercaseFirst(value) { | ||
| return `${value[0].toLowerCase()}${value.slice(1)}`; | ||
| } |
There was a problem hiding this comment.
Bug: lowercaseFirst is called with the result of modelInstallHint(...), which always returns a non-empty string starting with "Run '". So in practice it never receives an empty string. However, if value is ever empty, value[0] is undefined and the template literal produces "undefinedrun ...". Adding a guard is a cheap safeguard.
Consider adding a guard:
if (!value) return value;| function lowercaseFirst(value) { | |
| return `${value[0].toLowerCase()}${value.slice(1)}`; | |
| } | |
| function lowercaseFirst(value) { | |
| if (!value) return value; | |
| return `${value[0].toLowerCase()}${value.slice(1)}`; | |
| } |
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
| async function createSession(configuration, options) { | ||
| sessionInitialization ??= createEmbeddingRuntime(configuration, options) | ||
| .then((runtime) => (sessionRuntime = runtime)) | ||
| .catch((error) => { | ||
| defaultInitialization = undefined; | ||
| sessionInitialization = undefined; | ||
| throw error; | ||
| }); | ||
| return defaultInitialization; | ||
| return sessionInitialization; | ||
| } |
There was a problem hiding this comment.
Logic Error: createSession uses module-level singletons (sessionRuntime, sessionInitialization), so if createSession is called a second time with a different configuration or options, the ??= short-circuits and silently returns the first session's runtime. Any AISQLiteService instance initialized after the first will use the wrong model without any error. If multiple ai-sqlite services with different models are ever supported, this will be a silent correctness failure.
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
* 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
* 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
* feat: support configurable local embedding models * feat: provision embedding models by name (#55) * 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 * fix: make tokenizer an optional peer --------- Co-authored-by: Sebastian Van Syckel <sebastian.van.syckel@sap.com> Co-authored-by: sjvans <30337871+sjvans@users.noreply.github.com>
1 participant
Summary
cds.env.requires.db.embedding.model; there is no default model<cds.root>/.cds/models/<model>by defaultnpx @cap-js/ai install-model <model> [--directory <root>]for explicit provisioning~/shared cache root throughembedding.directory; configured directories remain offline/read-only at runtimeembedding.lock.jsonautomatically with the immutable revision, files, checksums, dimensions, token limit, pooling, and normalizationRuntime configuration
{ "cds": { "requires": { "db": { "kind": "ai-sqlite", "embedding": { "model": "foo/bar" } } } } }With no
directory, the effective model directory is:If it does not exist, startup logs a warning, downloads and validates it, and retains it for later starts.
Explicit/shared provisioning
npx @cap-js/ai install-model foo/bar --directory ~/.cds/modelsThis installs into
~/.cds/models/foo/bar. Applications can reuse it with:{ "embedding": { "model": "foo/bar", "directory": "~/.cds/models" } }When
directoryis configured, startup only verifies the existing installation and never downloads or modifies it.Discovery and validation
onnx/model.onnx_dataValidation
npm test— 72/72 passingnpx -y eslint@10 .npx -y prettier@3 --check .git diff --checkStack
This PR targets
feat/actual-tokenizerand follows #51.