Concurrent simulation runner #946

abdelr · 2021-04-08T08:15:43Z

No description provided.

…tches

… share the same simulation object

Yuri05 · 2021-04-08T10:50:27Z

@abdelr @PavelBal Can you give an example R code how this "concurrent" simulation run will be used in R?

PavelBal · 2021-04-08T11:26:57Z

@Yuri05 here just an example how it could look like (this code is for the older version and the signatures of the methods are different, but the idea from @examples should be clear)

#' @title  Runs a set of simulations (only individual simulations) and returns a list of \code{SimulationResults}
#'
#' @param simulations A list of \code{Simulation} objects to simulate.
#' @param simulationRunOptions Optional instance of a \code{SimulationRunOptions} used during the simulation run
#'
#' @return A list of \code{SimulationResults} objects in the order corresponding to the order of simulations.
#'
#' @examples
#' simPath <- system.file("extdata", "simple.pkml", package = "ospsuite")
#' sim <- loadSimulation(simPath)
#' sim2 <- loadSimulation(simPath)
#' sim3 <- loadSimulation(simPath)
#' results <- runSimulationsConcurrently(list(sim, sim2, sim3))
#' @export
runSimulationsConcurrently <- function(simulations, simulationRunOptions = NULL) {
  validateIsOfType(simulations, Simulation)
  validateIsOfType(simulationRunOptions, SimulationRunOptions, nullAllowed = TRUE)
  simulationRunner <- getNetTask("SimulationRunner")

  simulations <- c(simulations)

  #Create SimulationRunnerConcurrentOptions and add all simulations
  runConcurrentOptions <- SimulationRunnerConcurrentOptions$new()
  for (i in seq_along(simulations)){
    runConcurrentOptions$addSimulation(simulations[[i]])
  }

  results <- rClr::clrCall(simulationRunner, "RunConcurrently", runConcurrentOptions$ref)

  simResultsList <- vector("list", length(simulations))
  for (i in seq_along(simulations)){
    simResultsList[[i]] <- SimulationResults$new(results[[i]], simulations[[i]])
  }
  return(simResultsList)
}

Yuri05 · 2021-04-08T16:59:05Z

And should all simulations be executed only once or multiple times with different parameter sets?
Because in the latter case the current implementation would be very inefficient.

msevestre · 2021-04-08T17:06:14Z

src/OSPSuite.Core/Domain/Services/SimModelBatch.cs

      }

      public IReadOnlyList<string> VariableParameterPaths { get; private set; }

      public IReadOnlyList<string> VariableMoleculePaths { get; private set; }

+      public object Clone()
+      {
+         var xml = _simModelExporter.ExportSimModelXml(_modelCoreSimulation, new SimulationRunOptions().SimModelExportMode);


wow.. what's going on here?😳
What are you trying to achieve? If feels like you are not working at the right abstraction level here

… for running simulations concurrently

src/OSPSuite.Core/Domain/Services/ConcurrencyManager.cs

msevestre

I think this is looking very good. A few questions

msevestre · 2021-04-16T18:12:48Z

src/OSPSuite.Core/Domain/Services/ConcurrencyManager.cs

+               while (data.TryDequeue(out var datum))
+               {
+                  //Invoke the action on it and store the result
+                  var result = await action.Invoke(coreIndex, cancellationToken, datum);


the cancellationToken needs to be checked here I believe. It is the responsibility of the caller to throw if the action has been cancelled

You are right... adding the fix now

msevestre · 2021-04-16T18:13:25Z

src/OSPSuite.Core/Domain/Services/ConcurrencyManager.cs

+         await Task.WhenAll(tasks);
+         //all tasks are completed. Can return results
+
+         return results.ToDictionary(kvp => kvp.Key, kvp => kvp.Value);


why not return results? Just curious

So we return a Task you could await. Again, from R you cannot await so the ConcurrentSimulationRunner does not use this feature but from other parts of the code you could have async/await

This is not what I mean.results is already a dictionary. Why are your returning another dictionary based of this? The async/await is created for you automatically because you are using await /async in your code.

To the best of my knowledge, ConcurrentDictionary and Dictionary are different classes. They do not share a common interface or anything like that. We need a ConcurrentDictionary to avoid concurrent access issues but beyond ConcurrencyManager class, we do not need it anymore so we should turn it into a Dictionary. Similarly, we use a ConcurrentQueue. I have just updated the signature so it accepts a IEnumerable instead of a ConcurrentQueue since this data structure is only needed as a robust implementation but should not be affecting the signature of the method.

To the best of my knowledge, ConcurrentDictionary and Dictionary are different classes. They do not share a common interface or anything like that.

oh yeah they share IDictionary or IReadOnlyDictionary.

IReadOnlyDictionary is what we should return in that case anyways so that the caller does not mutate (at least not without explicit casting) the results

src/OSPSuite.Core/Domain/Services/ISimModelBatch.cs

src/OSPSuite.Core/Domain/Services/SimModelBatch.cs

msevestre · 2021-04-16T18:15:14Z

src/OSPSuite.R/Services/ConcurrentSimulationRunner.cs

+
+      public SimulationResults[] RunConcurrently()
+      {
+         var results = _concurrentManager.RunAsync<IModelCoreSimulation, SimulationResults>(


should not we create the instance of ConcurrentMAnager here? why is it injected?

Why creating a new object every time. Also, with injection you decouple the implementations. Whoever is using the ConcurrentSimulationRunner could decide on the implementation of the ConcurrencyManager to use by just injecting it. Why should we create the object ourselves in the code and break the inversion of control?

I did not say NEW. I say ensuring that we create a. new instance here so that there is no risk of memory leaks. Typically using a factory method to retrieve a new instance in a Dispose construct. That way, if something goes wrong, we clear after ourselves.

So we do not create the object ourselves, we do not break inversion of control. We just ensure that we deal with memory leaks intrinsically by not holding references to objects

I agree. Sorry, I did not understood before. I will add the changes for this.

msevestre · 2021-04-16T18:15:41Z

src/OSPSuite.R/Services/ConcurrentSimulationRunner.cs

+      {
+         var results = _concurrentManager.RunAsync<IModelCoreSimulation, SimulationResults>(
+            NumberOfCores,
+            new System.Threading.CancellationToken(false),


what does the false do?

The cconcurrentSimulationRunner is never canceling the run since it will be used from R. I am not sure if this should be a feature (canceling the call). I am not sure either on how to do it from R in case it is possible. If this is something we should include, please tell me and I will include it. Still, the ConcurrencyManager does support canceling since it could be used from somewhere else.

My question is: What does the false do? :)

Initializes the state of the token as not cancelled.

src/OSPSuite.R/Services/ConcurrentSimulationRunner.cs

msevestre · 2021-04-19T12:50:40Z

src/OSPSuite.R/Services/ConcurrentSimulationRunner.cs

+         if (_simulationBatches.Count > 0)
+            return null;
+
+         return Enumerable.Empty<SimulationResults>().ToArray();


Array.Empty :)

msevestre · 2021-04-19T12:50:59Z

src/OSPSuite.R/Services/ConcurrentSimulationRunner.cs

+
+      private async Task<SimulationResults> runSimulation(int coreIndex, CancellationToken cancellationToken, IModelCoreSimulation simulation)
+      {
+         return await Api.GetSimulationRunner().RunAsync(new SimulationRunArgs() { Simulation = simulation, SimulationRunOptions = SimulationRunOptions });


np need to Async away. Just return the Task

src/OSPSuite.R/Services/ConcurrentSimulationRunner.cs

msevestre · 2021-04-19T12:52:33Z

src/OSPSuite.R/Services/ConcurrentSimulationRunner.cs

+         //Currently we only allow for running simulations or simulation batches, but not both
+         if (_simulationBatches.Count > 0 && _simulations.Count > 0) 
+            //Temporal Exception. We should allow for mixing both use cases but we need to discuss first
+            throw new Exception("You already have Simulation and SimulationBatch objects and should not mix, please invoke Clear to start adding objects from a fresh start");


Never throw Exception when coming from us. Use OSPSuiteException (unless you intend to see the whole call stack etc... Here this is clearly more for the user to see

Also error in Error files?

msevestre · 2021-04-19T12:53:41Z

src/OSPSuite.R/Services/ConcurrentSimulationRunner.cs

+      SimulationResults[] RunConcurrently();
+   }
+
+   public class ConcurrentSimulationRunner : IConcurrentSimulationRunner


I would make this guy umplement IDisposable and ensure that Dispose called Clear if not done already

This is the snippet we use

private bool _disposed; public void Dispose() { if (_disposed) return; Cleanup(); GC.SuppressFinalize(this); _disposed = true; } ~ConcurrentSimulationRunner () { Cleanup(); } protected virtual void Cleanup() { }

msevestre · 2021-04-19T12:54:08Z

src/OSPSuite.R/Services/ConcurrentSimulationRunner.cs

+            var results = _concurrentManager.RunAsync(
+               NumberOfCores,
+               new CancellationToken(false),
+               new List<IModelCoreSimulation>(_simulations),


Why do you pass a New List?

abdelr and others added 14 commits April 8, 2021 10:06

Adding support for parallel execution of Simulations and simulationBa…

78c4569

…tches

Renaming to RunConcurrently

2a52a3d

SimulationRunner can also run populations

4a0910b

Update SimulationRunner.cs

542b3e5

turn collections into object[] for compatibility with rclr

a016b3d

Update SimulationBatchSpecs.cs

6f1d416

Update SimulationRunner.cs

ce7482a

Defining a settings class for running simulations concurrently

b969f2f

Update SimulationRunner.cs

ed6faca

Renamed settings to options

3641618

Adding failing tests to recreate Pavel's problem

df9662d

Remove Population

0c5b0f7

Fix: each thread runs with a different SimModelManager so they do not…

3c6beda

… share the same simulation object

Fixing rebase

804de61

abdelr requested a review from msevestre April 8, 2021 08:15

abdelr self-assigned this Apr 8, 2021

abdelr linked an issue Apr 8, 2021 that may be closed by this pull request

It should be possible to run simulations concurrently from R #945

Closed

SimulationBatch also accepts an unique argument

acd19e4

abdelr added 2 commits April 8, 2021 14:05

Make inner list readonly and add Add method for populating

2193ff1

typo fix

1b5306a

msevestre reviewed Apr 8, 2021

View reviewed changes

Adding ConcurrencyManager and ConcurrentSimulationRunner as a wrapper…

39c3de1

… for running simulations concurrently

Yuri05 reviewed Apr 16, 2021

View reviewed changes

src/OSPSuite.Core/Domain/Services/ConcurrencyManager.cs Outdated Show resolved Hide resolved

abdelr requested review from georgeDaskalakis and PavelBal April 16, 2021 11:58

Quick fixes

a1c5480

msevestre requested changes Apr 16, 2021

View reviewed changes

abdelr added 3 commits April 17, 2021 08:05

checking cancelation token before starting a new simulation

236bad2

Removing clones and some misc fixes

fbddd1f

Some missing cleaning up

2213af2

msevestre reviewed Apr 19, 2021

View reviewed changes

msevestre and others added 4 commits April 19, 2021 10:19

Some updates based on Code REview with @abdelr

c7d663b

Proper use of cancellation token

71bc753

Remove task duplication

02bc8fa

Error text from constant

03d1cf9

msevestre merged commit 7bbe115 into develop Apr 19, 2021

msevestre deleted the Concurrent_Simulation_Runner branch April 19, 2021 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Concurrent simulation runner #946

Concurrent simulation runner #946

abdelr commented Apr 8, 2021

Yuri05 commented Apr 8, 2021

PavelBal commented Apr 8, 2021

Yuri05 commented Apr 8, 2021

msevestre Apr 8, 2021

msevestre left a comment

msevestre Apr 16, 2021

abdelr Apr 17, 2021

msevestre Apr 16, 2021

abdelr Apr 17, 2021

msevestre Apr 17, 2021

abdelr Apr 19, 2021

msevestre Apr 19, 2021

msevestre Apr 16, 2021

abdelr Apr 17, 2021

msevestre Apr 17, 2021

abdelr Apr 19, 2021

msevestre Apr 16, 2021

abdelr Apr 17, 2021

msevestre Apr 17, 2021

abdelr Apr 19, 2021

msevestre Apr 19, 2021

msevestre Apr 19, 2021

msevestre Apr 19, 2021

msevestre Apr 19, 2021

msevestre Apr 19, 2021

Concurrent simulation runner #946

Concurrent simulation runner #946

Conversation

abdelr commented Apr 8, 2021

Yuri05 commented Apr 8, 2021

PavelBal commented Apr 8, 2021

Yuri05 commented Apr 8, 2021

Choose a reason for hiding this comment

msevestre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment