From e298dbb3ff698fe3a9670a1ed1a118d2fbf0c144 Mon Sep 17 00:00:00 2001 From: Robert McIntosh <261477+rwmcintosh@users.noreply.github.com> Date: Wed, 28 Sep 2022 10:33:11 -0400 Subject: [PATCH 1/4] Fixes #1672 Errors are not propagated on failed simulation run --- src/OSPSuite.Assets/UIConstants.cs | 2 + .../Domain/Services/ConcurrencyManager.cs | 4 +- .../Domain/Services/SimModelBatch.cs | 11 +++- src/OSPSuite.R/Domain/SimulationBatch.cs | 10 ++- .../ConcurrentSimulationRunnerSpecs.cs | 61 +++++++++++++++++++ 5 files changed, 82 insertions(+), 6 deletions(-) diff --git a/src/OSPSuite.Assets/UIConstants.cs b/src/OSPSuite.Assets/UIConstants.cs index e287a438e..da6f584b5 100644 --- a/src/OSPSuite.Assets/UIConstants.cs +++ b/src/OSPSuite.Assets/UIConstants.cs @@ -1644,6 +1644,8 @@ public static string SimulationPKAnalysesFileDoesNotHaveTheExpectedFormat } } + public static string SimulationDidNotProduceResults = "Simulation did not produce results"; + public static string DuplicatedIndividualResultsForId(int individualId) => $"Individual results for individual with id '{individualId}' were defined more than once!"; public static string DuplicatedPKParameterSensitivityFor(string id) => $"PKParameter sensitivity results for '{id}' were defined more than once!"; diff --git a/src/OSPSuite.Core/Domain/Services/ConcurrencyManager.cs b/src/OSPSuite.Core/Domain/Services/ConcurrencyManager.cs index 3483e0eb4..1d1e90978 100644 --- a/src/OSPSuite.Core/Domain/Services/ConcurrencyManager.cs +++ b/src/OSPSuite.Core/Domain/Services/ConcurrencyManager.cs @@ -39,7 +39,7 @@ public interface IConcurrencyManager /// Data produced by the worker function /// List of data to consume by the workers /// - /// A function running on each worker on each piece of data. + /// A function running on each worker on each piece of data. Needs to throw exceptions if the is not successful /// /// Cancellation token to cancel the threads /// Number of cores to use. Use null to take all cores @@ -56,7 +56,7 @@ int numberOfCoresToUse /// Data type to consume by the worker function /// List of data to consume by the workers /// - /// An action running on each worker on each piece of data. + /// An action running on each worker on each piece of data. Needs to throw exceptions if the is not successful /// /// Cancellation token to cancel the threads /// Number of cores to use. Use null to take all cores diff --git a/src/OSPSuite.Core/Domain/Services/SimModelBatch.cs b/src/OSPSuite.Core/Domain/Services/SimModelBatch.cs index 198c80e17..5cd91c395 100644 --- a/src/OSPSuite.Core/Domain/Services/SimModelBatch.cs +++ b/src/OSPSuite.Core/Domain/Services/SimModelBatch.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Linq; +using OSPSuite.Assets; using OSPSuite.Core.Domain.Data; using OSPSuite.Core.Serialization.SimModel.Services; using OSPSuite.SimModel; @@ -79,7 +80,10 @@ public SimulationRunResults RunSimulation() { updateSimulationValues(); _simModelSimulation.RunSimulation(); - return new SimulationRunResults(success: true, warnings: WarningsFrom(_simModelSimulation), results: getResults()); + var hasResults = simulationHasResults(_simModelSimulation); + + return new SimulationRunResults(success: hasResults, warnings: WarningsFrom(_simModelSimulation), results: getResults(), + error: hasResults ? null : Error.SimulationDidNotProduceResults); } catch (Exception e) { @@ -93,6 +97,11 @@ public SimulationRunResults RunSimulation() } } + private bool simulationHasResults(Simulation simModelSimulation) + { + return simModelSimulation.AllValues.Any(); + } + public void UpdateParameterValue(string parameterPath, double value) => _parameterValueCache[parameterPath] = value; public void UpdateParameterValues(IReadOnlyList parameterValues) diff --git a/src/OSPSuite.R/Domain/SimulationBatch.cs b/src/OSPSuite.R/Domain/SimulationBatch.cs index fd7402256..99bd601b7 100644 --- a/src/OSPSuite.R/Domain/SimulationBatch.cs +++ b/src/OSPSuite.R/Domain/SimulationBatch.cs @@ -7,6 +7,7 @@ using OSPSuite.Core.Domain.Services; using OSPSuite.R.Extensions; using OSPSuite.SimModel; +using OSPSuite.Utility.Exceptions; using OSPSuite.Utility.Extensions; namespace OSPSuite.R.Domain @@ -148,12 +149,15 @@ public SimulationResults Run(SimulationBatchRunValues simulationBatchRunValues) { _simModelBatch.UpdateParameterValues(simulationBatchRunValues.Values); _simModelBatch.UpdateInitialValues(simulationBatchRunValues.MoleculeValues); - var simulationResults = _simModelBatch.RunSimulation(); + var simulationRunResults = _simModelBatch.RunSimulation(); + + if (!simulationRunResults.Success) + throw new OSPSuiteException(simulationRunResults.Error); if (!_simulationBatchOptions.CalculateSensitivity) - return _simulationResultsCreator.CreateResultsFrom(simulationResults.Results); + return _simulationResultsCreator.CreateResultsFrom(simulationRunResults.Results); - return _simulationResultsCreator.CreateResultsWithSensitivitiesFrom(simulationResults.Results, _simModelBatch, _simulationBatchOptions.Parameters); + return _simulationResultsCreator.CreateResultsWithSensitivitiesFrom(simulationRunResults.Results, _simModelBatch, _simulationBatchOptions.Parameters); } protected virtual void Cleanup() diff --git a/tests/OSPSuite.R.Tests/Services/ConcurrentSimulationRunnerSpecs.cs b/tests/OSPSuite.R.Tests/Services/ConcurrentSimulationRunnerSpecs.cs index 23bbe3b15..c7a2ccb2c 100644 --- a/tests/OSPSuite.R.Tests/Services/ConcurrentSimulationRunnerSpecs.cs +++ b/tests/OSPSuite.R.Tests/Services/ConcurrentSimulationRunnerSpecs.cs @@ -8,6 +8,7 @@ using OSPSuite.Core.Domain.Services; using OSPSuite.Core.Extensions; using OSPSuite.R.Domain; +using OSPSuite.Utility.Exceptions; namespace OSPSuite.R.Services { @@ -87,6 +88,66 @@ public void should_complete_without_error() } } + public class When_running_a_batch_simulation_with_exception : concern_for_ConcurrentSimulationRunner + { + private ConcurrentRunSimulationBatch _concurrentRunSimulationBatch; + private ConcurrencyManagerResult[] _results; + private IModelCoreSimulation _simulation; + private readonly List _ids = new List(); + private readonly List _simulationBatchRunValues = new List(); + + public override void GlobalContext() + { + base.GlobalContext(); + _simulation = _simulationPersister.LoadSimulation(HelperForSpecs.DataFile("S1.pkml")); + + _concurrentRunSimulationBatch = new ConcurrentRunSimulationBatch + ( + _simulation, + new SimulationBatchOptions + { + VariableMolecules = new[] + { + new[] { "Organism", "Kidney", "Intracellular", "Caffeine" }.ToPathString() + }, + + VariableParameters = new[] + { + new[] { "Organism", "Liver", "Volume" }.ToPathString(), + new[] { "Organism", "Hematocrit" }.ToPathString(), + } + } + ); + + sut.AddSimulationBatch(_concurrentRunSimulationBatch); + } + + protected override void Because() + { + _simulation.SimulationSettings.Solver.MxStep = 3; + _simulationBatchRunValues.Add(new SimulationBatchRunValues + { + InitialValues = new[] { 10.0 }, + ParameterValues = new[] { 3.5, 0.53 } + }); + + _concurrentRunSimulationBatch.AddSimulationBatchRunValues(_simulationBatchRunValues[0]); + + _ids.AddRange(_concurrentRunSimulationBatch.SimulationBatchRunValues.Select(x => x.Id)); + _results = sut.RunConcurrently(); + } + + [Observation] + public void should_throw_exception_when_the_simulation_run_fails() + { + var id = _ids.First(); + var simulationBatch = Api.GetSimulationBatchFactory().Create(_simulation, _concurrentRunSimulationBatch.SimulationBatchOptions); + + The.Action(() => simulationBatch.Run(_simulationBatchRunValues.FirstOrDefault(v => v.Id == id))) + .ShouldThrowAn(); + } + } + public class When_running_a_batch_simulation_run_concurrently : concern_for_ConcurrentSimulationRunner { private ConcurrentRunSimulationBatch _concurrentRunSimulationBatch; From 2fa3df2c4499b50f49ab9c3dd3b1b6a2844b4e7b Mon Sep 17 00:00:00 2001 From: Robert McIntosh <261477+rwmcintosh@users.noreply.github.com> Date: Wed, 28 Sep 2022 11:33:17 -0400 Subject: [PATCH 2/4] reorganize tests and add helper method for creating run results --- .../Domain/Services/SimModelBatch.cs | 20 +++++++-- .../Domain/SimulationBatchSpecs.cs | 45 +++++++++++++++++++ .../ConcurrentSimulationRunnerSpecs.cs | 12 +++-- 3 files changed, 66 insertions(+), 11 deletions(-) diff --git a/src/OSPSuite.Core/Domain/Services/SimModelBatch.cs b/src/OSPSuite.Core/Domain/Services/SimModelBatch.cs index 5cd91c395..edf0835d1 100644 --- a/src/OSPSuite.Core/Domain/Services/SimModelBatch.cs +++ b/src/OSPSuite.Core/Domain/Services/SimModelBatch.cs @@ -82,13 +82,11 @@ public SimulationRunResults RunSimulation() _simModelSimulation.RunSimulation(); var hasResults = simulationHasResults(_simModelSimulation); - return new SimulationRunResults(success: hasResults, warnings: WarningsFrom(_simModelSimulation), results: getResults(), - error: hasResults ? null : Error.SimulationDidNotProduceResults); + return createSimulationResults(); } catch (Exception e) { - return new SimulationRunResults(success: false, warnings: WarningsFrom(_simModelSimulation), results: new DataRepository(), - error: e.FullMessage()); + return createSimulationResults(e.FullMessage()); } finally { @@ -97,6 +95,20 @@ public SimulationRunResults RunSimulation() } } + SimulationRunResults createSimulationResults(string errorFromException = null) + { + var hasResults = simulationHasResults(_simModelSimulation); + var warnings = WarningsFrom(_simModelSimulation); + var error = errorFromException ?? (hasResults ? null : Error.SimulationDidNotProduceResults); + if (error == null) + { + return new SimulationRunResults(success: true, warnings, results: getResults()); + } + + //error + return new SimulationRunResults(false, warnings, new DataRepository(), error); + } + private bool simulationHasResults(Simulation simModelSimulation) { return simModelSimulation.AllValues.Any(); diff --git a/tests/OSPSuite.R.Tests/Domain/SimulationBatchSpecs.cs b/tests/OSPSuite.R.Tests/Domain/SimulationBatchSpecs.cs index 91af94a5f..2b570218d 100644 --- a/tests/OSPSuite.R.Tests/Domain/SimulationBatchSpecs.cs +++ b/tests/OSPSuite.R.Tests/Domain/SimulationBatchSpecs.cs @@ -7,6 +7,7 @@ using OSPSuite.Core.Extensions; using OSPSuite.R.Services; using OSPSuite.SimModel; +using OSPSuite.Utility.Exceptions; using OSPSuite.Utility.Extensions; namespace OSPSuite.R.Domain @@ -29,6 +30,50 @@ public override void GlobalContext() } } + public class When_running_a_batch_simulation_with_an_error : concern_for_SimulationBatch + { + private SimulationBatchOptions _simulationBatchOptions; + private SimulationResults _results; + private SimulationBatchRunValues _simulationBatchRunValues; + + public override void GlobalContext() + { + base.GlobalContext(); + + // Force an error during simulation run + _simulation.SimulationSettings.Solver.MxStep = 3; + _simulationBatchOptions = new SimulationBatchOptions + { + VariableMolecules = new[] + { + new[] {"Organism", "Kidney", "Intracellular", "Caffeine"}.ToPathString() + }, + + VariableParameters = new[] + { + new[] {"Organism", "Liver", "Volume"}.ToPathString(), + new[] {"Organism", "Hematocrit"}.ToPathString(), + } + }; + sut = _simulationBatchFactory.Create(_simulation, _simulationBatchOptions); + } + + protected override void Because() + { + _simulationBatchRunValues = new SimulationBatchRunValues + { + InitialValues = new[] { 10.0 }, + ParameterValues = new[] { 3.0, 0.53 } + }; + } + + [Observation] + public void should_throw_an_exception_during_run() + { + The.Action(() => sut.Run(_simulationBatchRunValues)).ShouldThrowAn(); + } + } + public class When_running_a_batch_simulation_run : concern_for_SimulationBatch { private SimulationBatchOptions _simulationBatchOptions; diff --git a/tests/OSPSuite.R.Tests/Services/ConcurrentSimulationRunnerSpecs.cs b/tests/OSPSuite.R.Tests/Services/ConcurrentSimulationRunnerSpecs.cs index c7a2ccb2c..138310af7 100644 --- a/tests/OSPSuite.R.Tests/Services/ConcurrentSimulationRunnerSpecs.cs +++ b/tests/OSPSuite.R.Tests/Services/ConcurrentSimulationRunnerSpecs.cs @@ -101,6 +101,8 @@ public override void GlobalContext() base.GlobalContext(); _simulation = _simulationPersister.LoadSimulation(HelperForSpecs.DataFile("S1.pkml")); + // Force an error during simulation run + _simulation.SimulationSettings.Solver.MxStep = 3; _concurrentRunSimulationBatch = new ConcurrentRunSimulationBatch ( _simulation, @@ -124,7 +126,7 @@ public override void GlobalContext() protected override void Because() { - _simulation.SimulationSettings.Solver.MxStep = 3; + _simulationBatchRunValues.Add(new SimulationBatchRunValues { InitialValues = new[] { 10.0 }, @@ -138,13 +140,9 @@ protected override void Because() } [Observation] - public void should_throw_exception_when_the_simulation_run_fails() + public void should_return_a_result_with_success_set_to_false() { - var id = _ids.First(); - var simulationBatch = Api.GetSimulationBatchFactory().Create(_simulation, _concurrentRunSimulationBatch.SimulationBatchOptions); - - The.Action(() => simulationBatch.Run(_simulationBatchRunValues.FirstOrDefault(v => v.Id == id))) - .ShouldThrowAn(); + _results.All(x => x.Succeeded).ShouldBeFalse(); } } From e7ebb7d7266f212a28fcb629553a9694a320e67f Mon Sep 17 00:00:00 2001 From: Robert McIntosh <261477+rwmcintosh@users.noreply.github.com> Date: Wed, 28 Sep 2022 11:34:13 -0400 Subject: [PATCH 3/4] Change method name --- src/OSPSuite.Core/Domain/Services/SimModelBatch.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/OSPSuite.Core/Domain/Services/SimModelBatch.cs b/src/OSPSuite.Core/Domain/Services/SimModelBatch.cs index edf0835d1..357499aa9 100644 --- a/src/OSPSuite.Core/Domain/Services/SimModelBatch.cs +++ b/src/OSPSuite.Core/Domain/Services/SimModelBatch.cs @@ -82,11 +82,11 @@ public SimulationRunResults RunSimulation() _simModelSimulation.RunSimulation(); var hasResults = simulationHasResults(_simModelSimulation); - return createSimulationResults(); + return createSimulationRunResults(); } catch (Exception e) { - return createSimulationResults(e.FullMessage()); + return createSimulationRunResults(e.FullMessage()); } finally { @@ -95,7 +95,7 @@ public SimulationRunResults RunSimulation() } } - SimulationRunResults createSimulationResults(string errorFromException = null) + SimulationRunResults createSimulationRunResults(string errorFromException = null) { var hasResults = simulationHasResults(_simModelSimulation); var warnings = WarningsFrom(_simModelSimulation); From 760b5d403291fdf603675cf99c3143ef0483e702 Mon Sep 17 00:00:00 2001 From: Robert McIntosh <261477+rwmcintosh@users.noreply.github.com> Date: Wed, 28 Sep 2022 14:40:46 -0400 Subject: [PATCH 4/4] Add a Success = true condition for test to make sure a failure does not cause the whole batch to fail --- src/OSPSuite.Core/Domain/Services/SimModelBatch.cs | 12 ++---------- .../Services/ConcurrentSimulationRunnerSpecs.cs | 13 ++++++++++--- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src/OSPSuite.Core/Domain/Services/SimModelBatch.cs b/src/OSPSuite.Core/Domain/Services/SimModelBatch.cs index 357499aa9..1ccc151da 100644 --- a/src/OSPSuite.Core/Domain/Services/SimModelBatch.cs +++ b/src/OSPSuite.Core/Domain/Services/SimModelBatch.cs @@ -100,19 +100,11 @@ SimulationRunResults createSimulationRunResults(string errorFromException = null var hasResults = simulationHasResults(_simModelSimulation); var warnings = WarningsFrom(_simModelSimulation); var error = errorFromException ?? (hasResults ? null : Error.SimulationDidNotProduceResults); - if (error == null) - { - return new SimulationRunResults(success: true, warnings, results: getResults()); - } - //error - return new SimulationRunResults(false, warnings, new DataRepository(), error); + return new SimulationRunResults(success: error == null, warnings, getResults(), error); } - private bool simulationHasResults(Simulation simModelSimulation) - { - return simModelSimulation.AllValues.Any(); - } + private bool simulationHasResults(Simulation simModelSimulation) => simModelSimulation.AllValues.Any(); public void UpdateParameterValue(string parameterPath, double value) => _parameterValueCache[parameterPath] = value; diff --git a/tests/OSPSuite.R.Tests/Services/ConcurrentSimulationRunnerSpecs.cs b/tests/OSPSuite.R.Tests/Services/ConcurrentSimulationRunnerSpecs.cs index 138310af7..f0706e4ab 100644 --- a/tests/OSPSuite.R.Tests/Services/ConcurrentSimulationRunnerSpecs.cs +++ b/tests/OSPSuite.R.Tests/Services/ConcurrentSimulationRunnerSpecs.cs @@ -101,8 +101,6 @@ public override void GlobalContext() base.GlobalContext(); _simulation = _simulationPersister.LoadSimulation(HelperForSpecs.DataFile("S1.pkml")); - // Force an error during simulation run - _simulation.SimulationSettings.Solver.MxStep = 3; _concurrentRunSimulationBatch = new ConcurrentRunSimulationBatch ( _simulation, @@ -133,7 +131,15 @@ protected override void Because() ParameterValues = new[] { 3.5, 0.53 } }); + // Force an error during simulation run because ParameterValues vector does not have enough values + _simulationBatchRunValues.Add(new SimulationBatchRunValues + { + InitialValues = new[] { 10.0 }, + ParameterValues = new[] { 3.5 } + }); + _concurrentRunSimulationBatch.AddSimulationBatchRunValues(_simulationBatchRunValues[0]); + _concurrentRunSimulationBatch.AddSimulationBatchRunValues(_simulationBatchRunValues[1]); _ids.AddRange(_concurrentRunSimulationBatch.SimulationBatchRunValues.Select(x => x.Id)); _results = sut.RunConcurrently(); @@ -142,7 +148,8 @@ protected override void Because() [Observation] public void should_return_a_result_with_success_set_to_false() { - _results.All(x => x.Succeeded).ShouldBeFalse(); + _results.Count(x => x.Succeeded).ShouldBeEqualTo(1); + _results.Count(x => !x.Succeeded).ShouldBeEqualTo(1); } }