refactor: simplify synchronous inference session - #57
Conversation
SummaryThe following content is AI-generated and provides a summary of the pull request: refactor: Simplify Synchronous Inference SessionThis PR removes the two-layer Changes
MotivationSQLite user-defined functions must return synchronously, while the public 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 refactor cleanly collapses the unnecessary two-layer abstraction into a single SynchronousInferenceSession class with a well-encapsulated private binding — a clear improvement in readability and ownership. Two small robustness gaps were flagged: the public name arrays are writable/mutable and could silently corrupt run() state, and the native binding is not guarded against double-dispose or use-after-dispose, which can crash the process. Please address those before merging.
PR Bot Information
Version: 1.29.49
- Correlation ID:
1b195f20-a171-11f1-8ad8-04ebc3770916 - LLM:
anthropic--claude-4.6-sonnet - File Content Strategy: Full file content
- Event Trigger:
pull_request.opened
| this.inputNames = this.#session.inputNames; | ||
| this.outputNames = this.#session.outputNames; |
There was a problem hiding this comment.
Bug: Public inputNames and outputNames properties are mutable and reflect the internal state
this.inputNames and this.outputNames are assigned directly from this.#session.inputNames (likely an array). Because they are plain public properties, any caller can accidentally or maliciously overwrite or mutate them (e.g. session.inputNames = [] or session.inputNames.push('x')), which would break subsequent run() calls that iterate over them without touching the private binding. Freezing the arrays when storing them prevents both mutation and reassignment confusion.
Consider making them read-only:
this.inputNames = Object.freeze([...this.#session.inputNames]);
this.outputNames = Object.freeze([...this.#session.outputNames]);Or define them as non-writable, non-configurable own properties if the spread/freeze pattern is too heavy.
| this.inputNames = this.#session.inputNames; | |
| this.outputNames = this.#session.outputNames; | |
| this.inputNames = Object.freeze([...this.#session.inputNames]); | |
| this.outputNames = Object.freeze([...this.#session.outputNames]); |
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
There was a problem hiding this comment.
Sounds reasonable to me; Could also just use the getters as was done in #51; We do need to expose these for createFeeds in embeddings.js, although I am not sure if createFeeds really makes a lot of sense ... Looking at that implementation, instead of creating feeds for the model, we could just create feeds hand them to the inference session and stop caring about input and output names on the outside ... I think that might also let us get rid of the Tensor re-export.
| dispose() { | ||
| this.#session.dispose(); | ||
| } |
There was a problem hiding this comment.
Bug: dispose() does not guard against double-dispose
After this.#session.dispose() is called the native binding object is freed. A second call to dispose() (or a run() after dispose) will dereference the already-freed native object and likely crash the process or produce undefined behaviour. The old SynchronousSessionHandler had the same gap, but since this PR is consolidating the class it is a good moment to fix it.
Consider nulling out the private field after disposal and guarding both run and dispose:
dispose() {
this.#session?.dispose();
this.#session = null;
}And in run, add an early guard: if (!this.#session) throw new Error('Session has been disposed.');
| dispose() { | |
| this.#session.dispose(); | |
| } | |
| dispose() { | |
| this.#session?.dispose(); | |
| this.#session = null; | |
| } |
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 session = await InferenceSession.create(modelPath); | ||
| const session = new SynchronousInferenceSession(modelPath); |
There was a problem hiding this comment.
I think create may have been an intentional mirroring of the ONNX API, but I am not sure if that's something we want to maintain.
PDT42
left a comment
PDT42
left a comment
There was a problem hiding this comment.
I think we should update createFeeds and decouple even more.
|
20585f2 to
13af14d
Compare
|
Rebased onto merged #51 and addressed the review feedback in 13af14d:
Focused vector/provisioning/knowledge-graph tests pass (61 tests), as do ESLint, Prettier, syntax checks, and |
2 participants
Summary
InferenceSession/SynchronousSessionHandlerwrapper with oneSynchronousInferenceSessiononnxruntime-nodeand nativeTensorcreation behind the dynamically imported session boundaryonnxruntime-node@1.20.1guard required by the private native APISQLite user-defined functions must return synchronously, while the public
onnxruntime-nodesession API returns promises. The dedicated adapter therefore remains necessary, but the extra handler abstraction is not.Validation
git diff --checkpassStack
Targets
AISQLiteServiceso merging this PR updates #53 directly.