fix: prevent error when model outputs buffer is larger than the RunMo…

…delParams internal buffer (#525)

Fixes #524

packages/runtime/src/runnable-model/buffered-run-model-params.spec.ts

@@ -259,7 +259,7 @@ describe('BufferedRunModelParams', () => {
     )
   })
 
-  it('should store output values from the model run', () => {
+  it('should store output values from the model run (when model outputs buffer length is same as internal buffer length)', () => {
     const inputs = [1, 2, 3]
     const outputs = new Outputs(['_x', '_y'], 2000, 2002, 1)
 
@@ -289,6 +289,47 @@ describe('BufferedRunModelParams', () => {
     expect(outputs.runTimeInMillis).toBe(42)
   })
 
+  it('should store output values from the model run (when model outputs buffer is longer than internal buffer)', () => {
+    const inputs = [1, 2, 3]
+    const outputs = new Outputs(['_x', '_y'], 2000, 2002, 1)
+
+    const runnerParams = new BufferedRunModelParams()
+    const workerParams = new BufferedRunModelParams()
+
+    // Run once
+    runnerParams.updateFromParams(inputs, outputs)
+    workerParams.updateFromEncodedBuffer(runnerParams.getEncodedBuffer())
+
+    // Pretend that the model writes the following values to its outputs buffer (which is
+    // longer than necessary and contains unused/extra values at the end) then calls the
+    // `store` methods.  This simulates the case where the wasm model allocates an outputs
+    // buffer of length N on one model run, and then another model run is done with a
+    // smaller set of save points.  In this case, the wasm model will use the already
+    // allocated buffer of length N, but the internal `outputs` view in the
+    // `BufferedRunModelParams` instance will have length less than N.  We verify that
+    // the `storeOutputs` method only copies a subset of the outputs buffer.
+    const outputsArray = new Float64Array([
+      // actual outputs
+      1, 2, 3, 4, 5, 6,
+      // extra values (should be ignored)
+      9, 9, 9
+    ])
+    workerParams.storeElapsedTime(42)
+    workerParams.storeOutputs(outputsArray)
+
+    // Verify that the elapsed time can be accessed in the runner params
+    expect(runnerParams.getElapsedTime()).toBe(42)
+
+    // Verify that the outputs buffer in the runner params contains the correct values
+    expect(runnerParams.getOutputs()).toEqual(new Float64Array([1, 2, 3, 4, 5, 6]))
+
+    // Copy the outputs buffer to the `Outputs` instance and verify the values
+    runnerParams.finalizeOutputs(outputs)
+    expect(outputs.getSeriesForVar('_x').points).toEqual([p(2000, 1), p(2001, 2), p(2002, 3)])
+    expect(outputs.getSeriesForVar('_y').points).toEqual([p(2000, 4), p(2001, 5), p(2002, 6)])
+    expect(outputs.runTimeInMillis).toBe(42)
+  })
+
   it('should copy lookups', () => {
     const listing = new ModelListing(JSON.parse(listingJson))
 

packages/runtime/src/runnable-model/buffered-run-model-params.ts

@@ -183,8 +183,21 @@ export class BufferedRunModelParams implements RunModelParams {
 
   // from RunModelParams interface
   storeOutputs(array: Float64Array): void {
-    // Copy from the given array to the internal buffer
-    this.outputs.view?.set(array)
+    if (this.outputs.view === undefined) {
+      return
+    }
+
+    // Copy from the given array to the internal buffer.  Note that the given array
+    // can be longer than the internal buffer.  This can happen in the case where the
+    // model has an outputs buffer already allocated that was sized to accommodate a
+    // certain amount of outputs, and then a later model run used a smaller amount of
+    // outputs.  In this case, the model may choose to keep the reuse the buffer
+    // rather than reallocate/shrink the buffer, so we need to copy a subset here.
+    if (array.length > this.outputs.view.length) {
+      this.outputs.view.set(array.subarray(0, this.outputs.view.length))
+    } else {
+      this.outputs.view.set(array)
+    }
   }
 
   // from RunModelParams interface

packages/runtime/src/runnable-model/referenced-run-model-params.spec.ts

@@ -173,7 +173,7 @@ describe('ReferencedRunModelParams', () => {
     )
   })
 
-  it('should store output values from the model run', () => {
+  it('should store output values from the model run (when model outputs buffer length is same as outputs instance length)', () => {
     const inputs = [1, 2, 3]
     const outputs = new Outputs(['_x', '_y'], 2000, 2002, 1)
 
@@ -195,6 +195,33 @@ describe('ReferencedRunModelParams', () => {
     expect(outputs.getSeriesForVar('_y').points).toEqual([p(2000, 4), p(2001, 5), p(2002, 6)])
   })
 
+  it('should store output values from the model run (when model outputs buffer is longer than outputs instance)', () => {
+    const inputs = [1, 2, 3]
+    const outputs = new Outputs(['_x', '_y'], 2000, 2002, 1)
+
+    const params = new ReferencedRunModelParams()
+    params.updateFromParams(inputs, outputs)
+
+    // Pretend that the model writes the following values to its buffer then
+    // calls the `store` methods
+    const outputsArray = new Float64Array([
+      // actual outputs
+      1, 2, 3, 4, 5, 6,
+      // extra values (should be ignored)
+      9, 9, 9
+    ])
+    params.storeElapsedTime(42)
+    params.storeOutputs(outputsArray)
+
+    // Verify that the elapsed time can be accessed
+    expect(params.getElapsedTime()).toBe(42)
+
+    // Verify that the `Outputs` instance is updated with the correct values
+    expect(outputs.varIds).toEqual(['_x', '_y'])
+    expect(outputs.getSeriesForVar('_x').points).toEqual([p(2000, 1), p(2001, 2), p(2002, 3)])
+    expect(outputs.getSeriesForVar('_y').points).toEqual([p(2000, 4), p(2001, 5), p(2002, 6)])
+  })
+
   it('should copy lookups', () => {
     const listing = new ModelListing(JSON.parse(listingJson))
 

tests/integration/impl-var-access/run-tests.js

@@ -35,6 +35,7 @@ function verifyDeclaredOutputs(runnerKind, outputs, inputX) {
   // X = 0 [-10,10]
   // Y = X * 3
   // Z = T + Y
+  verify(runnerKind, outputs, inputX, '_y', () => inputX * 3)
   verify(runnerKind, outputs, inputX, '_z', (time, inputX) => time + inputX * 3)
 
   // A[DimA] = 1, 2
@@ -48,7 +49,7 @@ function verifyDeclaredOutputs(runnerKind, outputs, inputX) {
   verify(runnerKind, outputs, inputX, '_e[_a2,_b1]', () => 2 + 100)
 }
 
-function verifyImplOutputs(runnerKind, outputs, inputX) {
+function verifyImplOutputs(runnerKind, outputs, inputX, subset) {
   // Y = X * 3
   verify(runnerKind, outputs, inputX, '_y', () => inputX * 3)
 
@@ -57,6 +58,11 @@ function verifyImplOutputs(runnerKind, outputs, inputX) {
   // C[DimA, DimB] = A[DimA] + B[DimB]
   verify(runnerKind, outputs, inputX, '_c[_a2,_b2]', () => 2 + 200)
   verify(runnerKind, outputs, inputX, '_c[_a2,_b3]', () => 2 + 300)
+
+  if (subset > 1) {
+    verify(runnerKind, outputs, inputX, '_a[_a1]', () => 1)
+    verify(runnerKind, outputs, inputX, '_a[_a2]', () => 2)
+  }
 }
 
 async function runTests(runnerKind, modelRunner) {
@@ -75,21 +81,35 @@ async function runTests(runnerKind, modelRunner) {
   outputs = await modelRunner.runModel(inputs, outputs)
 
   // Verify declared output variables
-  verifyDeclaredOutputs(runnerKind, outputs, 0)
+  verifyDeclaredOutputs(runnerKind, outputs, /*inputX=*/ 0)
 
   // Run the model with input at 1
   inputX.set(1)
   outputs = await modelRunner.runModel(inputs, outputs)
 
   // Verify declared output variables
-  verifyDeclaredOutputs(runnerKind, outputs, 1)
+  verifyDeclaredOutputs(runnerKind, outputs, /*inputX=*/ 1)
 
-  // Run the model again and capture impl variables
+  // Run the model again and capture impl variables.  This first subset includes
+  // fewer variables (3) than the declared outputs (4).  This tests the case where
+  // the model allocates an internal outputs buffer of a certain length, and then
+  // is run again reusing that same buffer, which is longer than necessary.
   let outputsWithImpls1 = listing.deriveOutputs(outputs, ['_y', '_c[_a2,_b2]', '_c[_a2,_b3]'])
   outputsWithImpls1 = await modelRunner.runModel(inputs, outputsWithImpls1)
 
-  // Verify impl variables
-  verifyImplOutputs(runnerKind, outputsWithImpls1, 1)
+  // Verify impl variables (first subset)
+  verifyImplOutputs(runnerKind, outputsWithImpls1, /*inputX=*/ 1, /*subset=*/ 1)
+
+  // Run the model again and capture impl variables.  This second subset includes
+  // more variables (5) than the declared outputs (4).  This tests the case where
+  // the model allocates an internal outputs buffer of a certain length, and then
+  // is run again and needs to allocate a larger internal buffer to hold the new
+  // amount of outputs.
+  let outputsWithImpls2 = listing.deriveOutputs(outputs, ['_y', '_c[_a2,_b2]', '_c[_a2,_b3]', '_a[_a1]', '_a[_a2]'])
+  outputsWithImpls2 = await modelRunner.runModel(inputs, outputsWithImpls2)
+
+  // Verify impl variables (second subset)
+  verifyImplOutputs(runnerKind, outputsWithImpls2, /*inputX=*/ 1, /*subset=*/ 2)
 
   // Terminate the model runner
   await modelRunner.terminate()
@@ -105,7 +125,7 @@ async function createSynchronousRunner() {
   // Load the generated model and verify that it exposes `outputVarIds`
   const generatedModel = await loadGeneratedModel()
   const actualVarIds = generatedModel.outputVarIds || []
-  const expectedVarIds = ['_z', '_d[_a1]', '_e[_a2,_b1]']
+  const expectedVarIds = ['_y', '_z', '_d[_a1]', '_e[_a2,_b1]']
   if (actualVarIds.length !== expectedVarIds.length || !actualVarIds.every((v, i) => v === expectedVarIds[i])) {
     throw new Error(
       `Test failed: outputVarIds [${actualVarIds}] in generated model don't match expected values [${expectedVarIds}]`

tests/integration/impl-var-access/sde.config.js

@@ -11,7 +11,7 @@ export async function config() {
     modelSpec: async () => {
       return {
         inputs: ['X'],
-        outputs: ['Z', 'D[A1]', 'E[A2,B1]'],
+        outputs: ['Y', 'Z', 'D[A1]', 'E[A2,B1]'],
         customOutputs: true
       }
     },