From 847da16ed5645de2b386022e28d7eda0368aecf2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Abdel=20Rodr=C3=ADguez?= Date: Tue, 27 Apr 2021 11:20:33 +0200 Subject: [PATCH 1/3] Support for Exception handling --- .../Domain/Services/ConcurrencyManager.cs | 76 ++++++++++++++++++- .../Services/ConcurrentSimulationRunner.cs | 49 +++++------- .../ConcurrentSimulationRunnerSpecs.cs | 12 +-- 3 files changed, 98 insertions(+), 39 deletions(-) diff --git a/src/OSPSuite.Core/Domain/Services/ConcurrencyManager.cs b/src/OSPSuite.Core/Domain/Services/ConcurrencyManager.cs index 1175d883b..3c54eb03c 100644 --- a/src/OSPSuite.Core/Domain/Services/ConcurrencyManager.cs +++ b/src/OSPSuite.Core/Domain/Services/ConcurrencyManager.cs @@ -7,6 +7,22 @@ namespace OSPSuite.Core.Domain.Services { + public class ConcurrencyManagerResult + { + public string Id { get; } + public bool Succeeded { get; } + public string ErrorMessage { get; } + public TResult Result { get; } + + public ConcurrencyManagerResult(string id, bool succeeded, TResult result, string error) + { + Id = id; + Succeeded = succeeded; + ErrorMessage = error; + Result = result; + } + } + public interface IConcurrencyManager { /// @@ -18,20 +34,34 @@ public interface IConcurrencyManager /// List of data to consume by the workers /// A function to run on each worker on each piece of data /// Dictionary binding a result for each input data after running the action on it - Task> RunAsync(int numberOfCoresToUse, CancellationToken cancellationToken, IReadOnlyList data, Func> action); + Task>> RunAsync + ( + int numberOfCoresToUse, + CancellationToken cancellationToken, + IReadOnlyList data, + Func id, + Func> action + ) where TResult : class; } public class ConcurrencyManager : IConcurrencyManager { private readonly int _maximumNumberOfCoresToUse = Math.Max(1, Environment.ProcessorCount - 1); - public async Task> RunAsync(int numberOfCoresToUse, CancellationToken cancellationToken, IReadOnlyList data, Func> action) + public async Task>> RunAsync + ( + int numberOfCoresToUse, + CancellationToken cancellationToken, + IReadOnlyList data, + Func id, + Func> action + ) where TResult : class { if (numberOfCoresToUse <= 0) numberOfCoresToUse = _maximumNumberOfCoresToUse; var concurrentData = new ConcurrentQueue(data); numberOfCoresToUse = Math.Min(numberOfCoresToUse, concurrentData.Count); - var results = new ConcurrentDictionary(); + var results = new ConcurrentDictionary>(); //Starts one task per core var tasks = Enumerable.Range(0, numberOfCoresToUse).Select(async coreIndex => { @@ -41,7 +71,13 @@ public async Task> RunAsync( cancellationToken.ThrowIfCancellationRequested(); //Invoke the action on it and store the result - var result = await action.Invoke(coreIndex, cancellationToken, datum); + var result = await returnWithExceptionHandling( + coreIndex, + cancellationToken, + action, + datum, + id + ); results.TryAdd(datum, result); } }).ToList(); @@ -52,5 +88,37 @@ public async Task> RunAsync( var tt = results.Values; return results; } + + private async Task> returnWithExceptionHandling + ( + int coreId, + CancellationToken cancellationToken, + Func> task, + TData args, Func id + ) where TResult : class + { + TResult simulationResult; + try + { + simulationResult = await task.Invoke(coreId, cancellationToken, args); + } + catch (Exception e) + { + return new ConcurrencyManagerResult + ( + id(args), + false, + null, + e.Message + ); + } + return new ConcurrencyManagerResult + ( + id(args), + true, + simulationResult, + "" + ); + } } } \ No newline at end of file diff --git a/src/OSPSuite.R/Services/ConcurrentSimulationRunner.cs b/src/OSPSuite.R/Services/ConcurrentSimulationRunner.cs index 5cb74d881..1c1d78e8e 100644 --- a/src/OSPSuite.R/Services/ConcurrentSimulationRunner.cs +++ b/src/OSPSuite.R/Services/ConcurrentSimulationRunner.cs @@ -21,9 +21,10 @@ public class SettingsForConcurrentRunSimulationBatch public SimulationBatchOptions SimulationBatchOptions { get; set; } private ConcurrentQueue _simulationBatches = new ConcurrentQueue(); internal int MissingBatchesCount { get => Math.Max(0, SimulationBatchRunValues.Count - _simulationBatches.Count); } - internal bool AddNewBatch() { - _simulationBatches.Enqueue(Api.GetSimulationBatchFactory().Create(Simulation, SimulationBatchOptions)); - return true; + internal SimulationBatch AddNewBatch() { + var batch = Api.GetSimulationBatchFactory().Create(Simulation, SimulationBatchOptions); + _simulationBatches.Enqueue(batch); + return batch; } public IEnumerable SimulationBatches { get => _simulationBatches; } public string AddSimulationBatchRunValues(SimulationBatchRunValues simulationBatchRunValues) @@ -43,17 +44,6 @@ class SimulationBatchRunOptions public SimulationBatchOptions SimulationBatchOptions { get; set; } } - public class ConcurrentSimulationResults - { - public ConcurrentSimulationResults(string id, SimulationResults results) - { - Id = id; - SimulationResults = results; - } - public string Id { get; } - public SimulationResults SimulationResults { get; } - } - public interface IConcurrentSimulationRunner : IDisposable { /// @@ -81,13 +71,13 @@ public interface IConcurrentSimulationRunner : IDisposable /// Runs all preset settings concurrently /// /// - ConcurrentSimulationResults[] RunConcurrently(); + ConcurrencyManagerResult[] RunConcurrently(); /// /// After initialization phase, run all simulations or simulationBatches Async /// /// - Task> RunConcurrentlyAsync(); + Task>> RunConcurrentlyAsync(); } public class ConcurrentSimulationRunner : IConcurrentSimulationRunner @@ -140,11 +130,12 @@ private Task initializeBatches() ( settings => Enumerable.Range(0, settings.MissingBatchesCount).Select(_ => settings) ).ToList(), + data => new Guid().ToString(), (core, ct, settings) => Task.FromResult(settings.AddNewBatch()) ); } - public async Task> RunConcurrentlyAsync() + public async Task>> RunConcurrentlyAsync() { //Currently we only allow for running simulations or simulation batches, but not both if (_listOfSettingsForConcurrentRunSimulationBatch.Count > 0 && _simulations.Count > 0) @@ -157,6 +148,7 @@ public async Task> RunConcurrentlyAsync numberOfCores(), _cancellationTokenSource.Token, _simulations, + simulation => simulation.Id, runSimulation ); return results.Values; @@ -176,6 +168,7 @@ public async Task> RunConcurrentlyAsync SimulationBatchOptions = sb.SimulationBatchOptions, SimulationBatchRunValues = rv })).ToList(), + sb => sb.SimulationBatchRunValues.Id, runSimulationBatch ); @@ -185,27 +178,23 @@ public async Task> RunConcurrentlyAsync return results.Values; } - return Enumerable.Empty(); + return Enumerable.Empty>(); } - public ConcurrentSimulationResults[] RunConcurrently() => RunConcurrentlyAsync().Result.ToArray(); - - private async Task runSimulation(int coreIndex, CancellationToken cancellationToken, IModelCoreSimulation simulation) + public ConcurrencyManagerResult[] RunConcurrently() => RunConcurrentlyAsync().Result.ToArray(); + + + + private async Task runSimulation(int coreIndex, CancellationToken cancellationToken, IModelCoreSimulation simulation) { //We want a new instance every time that's why we are not injecting SimulationRunner in constructor - return new ConcurrentSimulationResults( - simulation.Id, - await Api.GetSimulationRunner().RunAsync(new SimulationRunArgs { Simulation = simulation, SimulationRunOptions = SimulationRunOptions }) - ); + return await Api.GetSimulationRunner().RunAsync(new SimulationRunArgs { Simulation = simulation, SimulationRunOptions = SimulationRunOptions }); } - private async Task runSimulationBatch(int coreIndex, CancellationToken cancellationToken, SimulationBatchRunOptions simulationBatchWithOptions) + private async Task runSimulationBatch(int coreIndex, CancellationToken cancellationToken, SimulationBatchRunOptions simulationBatchWithOptions) { - return new ConcurrentSimulationResults( - simulationBatchWithOptions.SimulationBatchRunValues.Id, - await simulationBatchWithOptions.SimulationBatch.RunAsync(simulationBatchWithOptions.SimulationBatchRunValues) - ); + return await simulationBatchWithOptions.SimulationBatch.RunAsync(simulationBatchWithOptions.SimulationBatchRunValues); } #region Disposable properties diff --git a/tests/OSPSuite.R.Tests/Services/ConcurrentSimulationRunnerSpecs.cs b/tests/OSPSuite.R.Tests/Services/ConcurrentSimulationRunnerSpecs.cs index 63b9800d2..0191ae048 100644 --- a/tests/OSPSuite.R.Tests/Services/ConcurrentSimulationRunnerSpecs.cs +++ b/tests/OSPSuite.R.Tests/Services/ConcurrentSimulationRunnerSpecs.cs @@ -3,6 +3,8 @@ using OSPSuite.BDDHelper.Extensions; using OSPSuite.Core; using OSPSuite.Core.Domain; +using OSPSuite.Core.Domain.Data; +using OSPSuite.Core.Domain.Services; using OSPSuite.Core.Extensions; using OSPSuite.R.Domain; using System.Collections.Generic; @@ -20,7 +22,7 @@ class CoreUserSettings : ICoreUserSettings public class When_running_simulations_concurrently : ContextForIntegration { private ISimulationPersister _simulationPersister; - private ConcurrentSimulationResults[] _results; + private ConcurrencyManagerResult[] _results; protected override void Context() { @@ -43,7 +45,7 @@ protected override void Because() public void should_run_the_simulations() { Assert.IsNotNull(_results); - Assert.IsTrue(_results.All(r => r.SimulationResults.ElementAt(0).AllValues.SelectMany(v => v.Values).Count() > 0)); + Assert.IsTrue(_results.All(r => r.Result.ElementAt(0).AllValues.SelectMany(v => v.Values).Count() > 0)); } } @@ -51,7 +53,7 @@ public class When_running_a_batch_simulation_run_concurrently : ContextForIntegr { private SettingsForConcurrentRunSimulationBatch _simulationWithBatchOptions; private ISimulationPersister _simulationPersister; - private ConcurrentSimulationResults[] _results; + private ConcurrencyManagerResult[] _results; private IModelCoreSimulation _simulation; private List _ids = new List(); private List _simulationBatchRunValues = new List(); @@ -115,8 +117,8 @@ public void should_be_able_to_simulate_the_simulation_for_multiple_runes() { var result = Api.GetSimulationBatchFactory().Create(_simulation, _simulationWithBatchOptions.SimulationBatchOptions).Run(_simulationBatchRunValues.FirstOrDefault(v => v.Id == id)); var concurrentResult = _results.FirstOrDefault(r => r.Id == id); - result.Time.Values.ShouldBeEqualTo(concurrentResult.SimulationResults.Time.Values); - result.ResultsFor(0).ValuesAsArray().Select(qv => qv.Values).ShouldBeEqualTo(concurrentResult.SimulationResults.ResultsFor(0).ValuesAsArray().Select(qv => qv.Values)); + result.Time.Values.ShouldBeEqualTo(concurrentResult.Result.Time.Values); + result.ResultsFor(0).ValuesAsArray().Select(qv => qv.Values).ShouldBeEqualTo(concurrentResult.Result.ResultsFor(0).ValuesAsArray().Select(qv => qv.Values)); } } } From de4f9bb46e3a9043c6de27b49d7b43d8f06f0be5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Abdel=20Rodr=C3=ADguez?= Date: Tue, 27 Apr 2021 15:49:10 +0200 Subject: [PATCH 2/3] Simulation and SimulationBatchOptions are set through constructor on SettingsForConcurrentRunSimulationBatch --- .../Services/ConcurrentSimulationRunner.cs | 13 ++++++++-- .../ConcurrentSimulationRunnerSpecs.cs | 26 +++++++++---------- 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/src/OSPSuite.R/Services/ConcurrentSimulationRunner.cs b/src/OSPSuite.R/Services/ConcurrentSimulationRunner.cs index 1c1d78e8e..4a4a803f5 100644 --- a/src/OSPSuite.R/Services/ConcurrentSimulationRunner.cs +++ b/src/OSPSuite.R/Services/ConcurrentSimulationRunner.cs @@ -16,17 +16,26 @@ namespace OSPSuite.R.Services { public class SettingsForConcurrentRunSimulationBatch { - public IModelCoreSimulation Simulation { get; set; } + public IModelCoreSimulation Simulation { get; } public List SimulationBatchRunValues { get; } = new List(); - public SimulationBatchOptions SimulationBatchOptions { get; set; } + public SimulationBatchOptions SimulationBatchOptions { get; } private ConcurrentQueue _simulationBatches = new ConcurrentQueue(); internal int MissingBatchesCount { get => Math.Max(0, SimulationBatchRunValues.Count - _simulationBatches.Count); } + + public SettingsForConcurrentRunSimulationBatch(IModelCoreSimulation simulation, SimulationBatchOptions simulationBatchOptions) + { + Simulation = simulation; + SimulationBatchOptions = simulationBatchOptions; + } + internal SimulationBatch AddNewBatch() { var batch = Api.GetSimulationBatchFactory().Create(Simulation, SimulationBatchOptions); _simulationBatches.Enqueue(batch); return batch; } + public IEnumerable SimulationBatches { get => _simulationBatches; } + public string AddSimulationBatchRunValues(SimulationBatchRunValues simulationBatchRunValues) { var id = Guid.NewGuid().ToString(); diff --git a/tests/OSPSuite.R.Tests/Services/ConcurrentSimulationRunnerSpecs.cs b/tests/OSPSuite.R.Tests/Services/ConcurrentSimulationRunnerSpecs.cs index 0191ae048..62122c74a 100644 --- a/tests/OSPSuite.R.Tests/Services/ConcurrentSimulationRunnerSpecs.cs +++ b/tests/OSPSuite.R.Tests/Services/ConcurrentSimulationRunnerSpecs.cs @@ -64,23 +64,23 @@ public override void GlobalContext() _simulationPersister = Api.GetSimulationPersister(); _simulation = _simulationPersister.LoadSimulation(HelperForSpecs.DataFile("S1.pkml")); - _simulationWithBatchOptions = new SettingsForConcurrentRunSimulationBatch() - { - Simulation = _simulation, - SimulationBatchOptions = new SimulationBatchOptions - { + _simulationWithBatchOptions = new SettingsForConcurrentRunSimulationBatch + ( + _simulation, + new SimulationBatchOptions + { VariableMolecules = new[] - { - new[] {"Organism", "Kidney", "Intracellular", "Caffeine"}.ToPathString() - }, + { + new[] {"Organism", "Kidney", "Intracellular", "Caffeine"}.ToPathString() + }, VariableParameters = new[] - { - new[] {"Organism", "Liver", "Volume"}.ToPathString(), - new[] {"Organism", "Hematocrit"}.ToPathString(), - } + { + new[] {"Organism", "Liver", "Volume"}.ToPathString(), + new[] {"Organism", "Hematocrit"}.ToPathString(), + } } - }; + ); sut = Api.GetConcurrentSimulationRunner(); sut.AddSimulationBatchOption(_simulationWithBatchOptions); From c5cc5c236cbc5c9bf21a1e0496df266dfb9f63fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Abdel=20Rodr=C3=ADguez?= Date: Wed, 28 Apr 2021 09:02:11 +0200 Subject: [PATCH 3/3] Code improve based on Michael's comments --- .../Domain/Services/ConcurrencyManager.cs | 38 ++++++++++--------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/src/OSPSuite.Core/Domain/Services/ConcurrencyManager.cs b/src/OSPSuite.Core/Domain/Services/ConcurrencyManager.cs index 3c54eb03c..5e6163650 100644 --- a/src/OSPSuite.Core/Domain/Services/ConcurrencyManager.cs +++ b/src/OSPSuite.Core/Domain/Services/ConcurrencyManager.cs @@ -7,20 +7,28 @@ namespace OSPSuite.Core.Domain.Services { - public class ConcurrencyManagerResult + public class ConcurrencyManagerResult where TResult : class { public string Id { get; } public bool Succeeded { get; } public string ErrorMessage { get; } public TResult Result { get; } - public ConcurrencyManagerResult(string id, bool succeeded, TResult result, string error) + public ConcurrencyManagerResult(string id, TResult result) { Id = id; - Succeeded = succeeded; - ErrorMessage = error; + Succeeded = true; + ErrorMessage = ""; Result = result; } + + public ConcurrencyManagerResult(string id, string errorMessage) + { + Id = id; + Succeeded = false; + ErrorMessage = errorMessage; + Result = null; + } } public interface IConcurrencyManager @@ -94,31 +102,25 @@ private async Task> returnWithExceptionHandlin int coreId, CancellationToken cancellationToken, Func> task, - TData args, Func id + TData input, Func id ) where TResult : class { - TResult simulationResult; try { - simulationResult = await task.Invoke(coreId, cancellationToken, args); + return new ConcurrencyManagerResult + ( + id(input), + result: await task.Invoke(coreId, cancellationToken, input) + ); } catch (Exception e) { return new ConcurrencyManagerResult ( - id(args), - false, - null, - e.Message + id(input), + errorMessage: e.Message ); } - return new ConcurrencyManagerResult - ( - id(args), - true, - simulationResult, - "" - ); } } } \ No newline at end of file