a sometimes failing concurrency test #1712

rwmcintosh · 2022-09-29T17:00:06Z

@Yuri05 @msevestre @PavelBal

When I run this test it will fail similarly often to the R code that inspired it.

The SequenceEqual test is checking that all the sequences are identical besides any from failing runs.

msevestre · 2022-09-29T17:07:30Z

This is good. We removed R altogether from this issue.
@Yuri05 finally has an issue worth looking at;-)

Yuri05 · 2022-09-29T17:56:42Z

Nice 🙂

Yuri05 · 2022-10-05T11:12:12Z

fun to debug :)
I temporarily extended the code in SimModelBatch.CreateAndFinalizeSimulation with this:

This writes the xmls which are passed to SimModel for calculation.
In the simulation, exactly 1 output variable is set as persistable:

Now when some simulations do not retrieve any results: this output variable is set as non-persistable!

Here the xmls of a failing run: simulation 27 is the one with persistable = 0
xmls.zip

So what happens: SimModel sometimes becomes an input with 0 required outputs, calculates and returns empty set of outputs, because none of the outputs is marked as to be returned.
SimModel should surely check for this and either produce a warning or stop with the error (not sure what's better) - but the initial issue is apparently the SimModel-XML creation in core.
Something is not thread safe there...

msevestre · 2022-10-05T12:33:50Z

SO GOOD @Yuri05
debug master did it again.
Ok so we need to make SimModel generation thread safe!. WTF. THe service needs to be instantiated on each Manager instead of recup once and cached. This is probably an easy fix

rwmcintosh · 2022-10-05T12:58:07Z

once again, bug reports go in to SimModel and then bounce straight back out. Thanks @Yuri05

msevestre · 2022-10-06T01:52:34Z

So we are already creating a new instance of the simulation exporter

   private SimulationExport createSimulationExport(IModelCoreSimulation simulation, SimModelExportMode simModelExportMode, IReadOnlyList<string> variableMoleculePaths)
      {
         var simulationExportCreator = _simulationExportCreatorFactory.Create();
         var simulationExport = simulationExportCreator.CreateExportFor(simulation.Model, simModelExportMode, variableMoleculePaths);
         simulationExport.AddSimulationConfiguration(simulation.SimulationSettings);
         return simulationExport;
      }

So the quick fix that I was hoping is not happening

msevestre · 2022-10-06T02:35:12Z

Ok I think I have it....

rwmcintosh · 2022-10-06T08:48:17Z

src/OSPSuite.R/Services/SimulationTask.cs

+      /// Return a clone of the simulation that can be used during batch run.
+      /// This simulation will contain the minimal information required to run. Configuration settings such as some building blocks will not be copied over
+      /// </summary>
+      /// <param name="simulation"></param>


That can be removed. You really just wanted the summary here I guess.

Yuri05 · 2022-10-06T08:53:32Z

src/OSPSuite.R/Services/SimulationTask.cs

+            // Initialize a BuildConfiguration with only SimulationSettings because some of the properties to complete the initialization are required
+            BuildConfiguration = new BuildConfiguration
+            {
+               SimulationSettings = _cloneManagerForModel.Clone(simulationToClone.SimulationSettings)
+            }
         };

-         // Initialize a BuildConfiguration with only SimulationSettings because some of the properties to complete the initialization are required
-         // None of the other properties are required to complete the simulation
-         var simulationBuildConfiguration = new BuildConfiguration
-         {
-            SimulationSettings = _cloneManagerForModel.Clone(simulationToClone.SimulationSettings)
-         };
-         simulation.BuildConfiguration = simulationBuildConfiguration;
+
+         //Once we prepare a simulation for a batch, we are assuming that it won't be changed by the caller.
+         //We update the persistable at this stage to avoid any possible thread problems
+         _simulationPersistableUpdater.UpdateSimulationPersistable(simulation);


so the problem was that cloning did not properly copy the simulation settings and thus the curve was not set as persistable?
But why did it happen only sometimes and not every time?

No that's not it. I put it there because it's an implementation details that needs to happen as we are about to start the simulation run.

Befote, it was done in the initialize of the batch, which was using the clone already. However it was the same instance shared amongst all run of the batch. as this was happening in //, there was potentially a moment where one batch would cleaer the output to set them again, but the other one was done and was generating XML..

Anyways this line was added to fix another bug and I remembered not being too happy about doing it that way but I didn't find anything better. Now that we have a place to clone the simulation, this felt to me like the right location

ok, I see. cool, cool :)

msevestre · 2022-10-06T02:36:00Z

src/OSPSuite.R/Domain/ConcurrentRunSimulationBatch.cs

@@ -25,7 +25,7 @@ public class ConcurrentRunSimulationBatch : IDisposable, IWithId

      public ConcurrentRunSimulationBatch(IModelCoreSimulation simulation, SimulationBatchOptions simulationBatchOptions)
      {
-         Simulation = Api.GetSimulationTask().Clone(simulation);
+         Simulation = Api.GetSimulationTask().CloneForBatchRun(simulation);


@rwmcintosh @Yuri05 I have renamed this guy because we are not creating a real clone
Just one that is enough to calculate

msevestre · 2022-10-06T02:36:30Z

src/OSPSuite.R/Domain/SimulationBatch.cs

      }

      public void Initialize(IModelCoreSimulation simulation, SimulationBatchOptions simulationBatchOptions)
      {
-         _simulationPersistableUpdater.UpdateSimulationPersistable(simulation);


This was the culprit. It is resetting the simulations output and then setting them again. This call is NOT thread safe and was called in mult thread environment

msevestre · 2022-10-06T02:37:18Z

src/OSPSuite.R/Services/SimulationTask.cs

+
+         //Once we prepare a simulation for a batch, we are assuming that it won't be changed by the caller.
+         //We update the persistable at this stage to avoid any possible thread problems
+         _simulationPersistableUpdater.UpdateSimulationPersistable(simulation);


I am doing it here. As soon as we cloen the simulation (for batch), I am making it also up to date as far as output selection is concerned. It is similar to what we do in normal run where we update before we reun

msevestre · 2022-10-06T02:37:58Z

tests/OSPSuite.R.Tests/Services/ConcurrentSimulationRunnerSpecs.cs

@@ -165,6 +228,7 @@ public override void GlobalContext()
      {
         base.GlobalContext();
         _simulation = _simulationPersister.LoadSimulation(HelperForSpecs.DataFile("S1.pkml"));
+         _simulationPersistableUpdater.UpdateSimulationPersistable(_simulation);


I do not know why this was required . The test is bypassing the runner and calling directly the underlying stuff so that's probably why

msevestre · 2022-10-06T11:51:06Z

src/OSPSuite.R/Services/SimulationTask.cs

+            // Initialize a BuildConfiguration with only SimulationSettings because some of the properties to complete the initialization are required
+            BuildConfiguration = new BuildConfiguration
+            {
+               SimulationSettings = _cloneManagerForModel.Clone(simulationToClone.SimulationSettings)
+            }
         };

-         // Initialize a BuildConfiguration with only SimulationSettings because some of the properties to complete the initialization are required
-         // None of the other properties are required to complete the simulation
-         var simulationBuildConfiguration = new BuildConfiguration
-         {
-            SimulationSettings = _cloneManagerForModel.Clone(simulationToClone.SimulationSettings)
-         };
-         simulation.BuildConfiguration = simulationBuildConfiguration;
+
+         //Once we prepare a simulation for a batch, we are assuming that it won't be changed by the caller.
+         //We update the persistable at this stage to avoid any possible thread problems
+         _simulationPersistableUpdater.UpdateSimulationPersistable(simulation);


No that's not it. I put it there because it's an implementation details that needs to happen as we are about to start the simulation run.

Befote, it was done in the initialize of the batch, which was using the clone already. However it was the same instance shared amongst all run of the batch. as this was happening in //, there was potentially a moment where one batch would cleaer the output to set them again, but the other one was done and was generating XML..

Anyways this line was added to fix another bug and I remembered not being too happy about doing it that way but I didn't find anything better. Now that we have a place to clone the simulation, this felt to me like the right location

msevestre · 2022-10-06T18:41:06Z

Damm I had written all those comments yesterday in the PR and forgot to submit review. I HATE THIS FEATURE IN GITHUB

msevestre · 2022-10-06T18:41:33Z

going to merge now. I think this will solve all our issues
FYI @PavelBal

PavelBal · 2022-10-07T08:14:55Z

Awesome. Once PK-Sim is updated, we should also update Core in R and should be able to use simulation batches again.

a sometimes failing concurrency test

a7b46d4

Fixes concurrenty issue

f3eee2a

msevestre marked this pull request as ready for review October 6, 2022 02:35

msevestre requested a review from Yuri05 October 6, 2022 02:35

rwmcintosh commented Oct 6, 2022

View reviewed changes

Yuri05 reviewed Oct 6, 2022

View reviewed changes

Fixes concurrency test

5cc6ff3

msevestre reviewed Oct 6, 2022

View reviewed changes

msevestre merged commit ccb2b9f into develop Oct 6, 2022

msevestre deleted the concurrency-test branch October 6, 2022 18:41

Yuri05 mentioned this pull request Nov 9, 2022

Non-deterministic simulation runs from SimModel Open-Systems-Pharmacology/OSPSuite.SimModel#151

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

a sometimes failing concurrency test #1712

a sometimes failing concurrency test #1712

rwmcintosh commented Sep 29, 2022

msevestre commented Sep 29, 2022

Yuri05 commented Sep 29, 2022

Yuri05 commented Oct 5, 2022

msevestre commented Oct 5, 2022

rwmcintosh commented Oct 5, 2022 •

edited

Loading

msevestre commented Oct 6, 2022

msevestre commented Oct 6, 2022

rwmcintosh Oct 6, 2022

Yuri05 Oct 6, 2022

msevestre Oct 6, 2022

Yuri05 Oct 6, 2022

msevestre Oct 6, 2022

msevestre Oct 6, 2022

msevestre Oct 6, 2022

msevestre Oct 6, 2022

msevestre Oct 6, 2022

msevestre commented Oct 6, 2022

msevestre commented Oct 6, 2022

PavelBal commented Oct 7, 2022

a sometimes failing concurrency test #1712

a sometimes failing concurrency test #1712

Conversation

rwmcintosh commented Sep 29, 2022

msevestre commented Sep 29, 2022

Yuri05 commented Sep 29, 2022

Yuri05 commented Oct 5, 2022

msevestre commented Oct 5, 2022

rwmcintosh commented Oct 5, 2022 • edited Loading

msevestre commented Oct 6, 2022

msevestre commented Oct 6, 2022

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

msevestre commented Oct 6, 2022

msevestre commented Oct 6, 2022

PavelBal commented Oct 7, 2022

rwmcintosh commented Oct 5, 2022 •

edited

Loading