From e04ee7d8291e6d7a834c59b6269055aa03e2cf9a Mon Sep 17 00:00:00 2001 From: Chris Campbell Date: Tue, 27 Aug 2024 14:00:15 -0700 Subject: [PATCH] fix: prevent error when model outputs buffer is larger than the RunModelParams internal buffer --- .../buffered-run-model-params.spec.ts | 43 ++++++++++++++++++- .../buffered-run-model-params.ts | 17 +++++++- .../referenced-run-model-params.spec.ts | 29 ++++++++++++- .../integration/impl-var-access/run-tests.js | 34 ++++++++++++--- .../integration/impl-var-access/sde.config.js | 2 +- 5 files changed, 113 insertions(+), 12 deletions(-) diff --git a/packages/runtime/src/runnable-model/buffered-run-model-params.spec.ts b/packages/runtime/src/runnable-model/buffered-run-model-params.spec.ts index b217fa65..2a71e57d 100644 --- a/packages/runtime/src/runnable-model/buffered-run-model-params.spec.ts +++ b/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)) diff --git a/packages/runtime/src/runnable-model/buffered-run-model-params.ts b/packages/runtime/src/runnable-model/buffered-run-model-params.ts index c5e28b8d..e0a5674b 100644 --- a/packages/runtime/src/runnable-model/buffered-run-model-params.ts +++ b/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 diff --git a/packages/runtime/src/runnable-model/referenced-run-model-params.spec.ts b/packages/runtime/src/runnable-model/referenced-run-model-params.spec.ts index d6439823..7abe1e56 100644 --- a/packages/runtime/src/runnable-model/referenced-run-model-params.spec.ts +++ b/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)) diff --git a/tests/integration/impl-var-access/run-tests.js b/tests/integration/impl-var-access/run-tests.js index 4ff828a4..355e4a32 100755 --- a/tests/integration/impl-var-access/run-tests.js +++ b/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}]` diff --git a/tests/integration/impl-var-access/sde.config.js b/tests/integration/impl-var-access/sde.config.js index a3884ea0..3bfa0b4d 100644 --- a/tests/integration/impl-var-access/sde.config.js +++ b/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 } },