Optimize model execution to stop immediately when required

[HMNS19]

This PR updates the do_step function to exit early when running.value is False. Previously, it would continue executing n steps even after running was set to stop. Now, it stops immediately when running.value becomes False.

This applies to models running in both ModelController and SimulatorController.

[github-actions]

Performance benchmarks:

Model	Size	Init time [95% CI]	Run time [95% CI]
BoltzmannWealth	small	🔵 +0.6% [+0.1%, +1.1%]	🔵 -0.1% [-0.3%, +0.2%]
BoltzmannWealth	large	🔵 +0.2% [-0.4%, +0.8%]	🔵 -1.6% [-2.4%, -0.8%]
Schelling	small	🔵 -0.4% [-0.6%, -0.2%]	🔵 -0.5% [-0.7%, -0.4%]
Schelling	large	🔵 -0.6% [-0.8%, -0.3%]	🔵 -0.5% [-1.9%, +0.7%]
WolfSheep	small	🔵 -1.2% [-1.4%, -1.0%]	🔵 -1.0% [-1.1%, -0.8%]
WolfSheep	large	🔵 -1.0% [-1.5%, -0.5%]	🔵 -0.8% [-1.1%, -0.5%]
BoidFlockers	small	🔵 +1.2% [+0.3%, +2.1%]	🔵 +3.2% [+3.0%, +3.4%]
BoidFlockers	large	🔵 +0.9% [+0.5%, +1.2%]	🔵 +2.4% [+2.1%, +2.6%]

[HMNS19]

I noticed that the checks didn’t pass in my PR due to an AttributeError in the test. The issue seems to be that model.running isn’t set in the mock object for simulator models. Since I added a feature to stop the model when running is False, the test fails because it tries to access model.running, which doesn’t exist in the mock.

Would it make sense to update the test setup to include model.running = True, or should we handle this differently?

[EwoutH]

Thanks for the PR! Could you explain clearer what the old behavior is, why it was wrong, what the new behavior is, and why that's right?

[HMNS19]

Right now, in the Schelling model, if I set the step interval to 100 (with default values for every other parameter), it runs all 100 steps even if it should stop earlier. For example, if running.value turns False at step 47, it still keeps going till 100, which isn’t how it should work. This PR fixes that by checking running.value before every step and stopping the simulation when it’s False.

• before

• after
  

This works fine for the example models but the PR checks fail for test models running in SimulatorController because model.running is missing in the test setup.

For SimulatorController, model.running is needed since simulator.py checks it to determine if the model should stop. However, in the test models model.running isn’t initialized in the mock object running on SimulatorContoller, causing an AttributeError. Since this PR references model.running in simulator.py, the test fails because it tries to access a missing attribute which caused the earlier check failures.

An alternative would be to copy how ModelController handles this into SimulatorController by directly checking running.value in solaraviz.py itself, instead of model.running . This would solve the issue without failing the checks, but it would also require using the for loop for the step interval just like ModelController does.

[quaquel]

I am trying to understand this PR and what motivates it. In my understanding, the overarching issue is that model.running is not used by Solara to stop a model when model.running is set to False. Is this correct?

Next, when trying to fix this, it has knock-on consequences for event scheduling as well. Again, is this understanding correct?

[HMNS19]

My bad for not being clear. This PR ensures the model stops exactly when it's supposed to, instead of running extra steps. This is essentially implementing #2332 in #2596.

The overarching issue is that model.running is not used by Solara to stop a model when model.running is set to False.

Right. We do have a variable running.value in solara_viz.py that is responsible for stopping the frontend, but it's only updated after running n steps (as specified by step interval).

Now, ModelController only required minor changes, which could be handled with running.value itself. These changes are already included in this PR.

For SimulatorController, initially, I considered the following approach:

def do_step():
    for _ in range(render_interval.value):
        if not running.value:
                break
        simulator.run_for(1)
        running.value = model.value.running
    force_update()

This would ensure that the model stops at the correct step, and it would also pass the build checks.

But since run_for runs the simulator from the current time to current time + time_delta, I thought it would be better to refrain from using a for loop and instead keep the current approach:

    def do_step():
        simulator.run_for(render_interval.value)
        running.value = model.value.running
        force_update()

while handling self.model.running in another suitable place, since this approach doesn't allow checking running.value before every step. This is why I opted to use self.model.running in simulator.py which caused the build checks to fail.

Wanted to get your input on which of these approaches seems more suitable.

[quaquel]

1. Can you ensure this PR is up to date with main?
2. I am inclined to separate this into 2 PRs. One is just solving the use of model.running bug for normal ABM models. The other would focus on adding support for model.running to event scheduling.
3. Beyond this PR, I find it confusing that we have two places where a running variable is declared: once in the model and once in ModelController for SolaraViz purposes. Ideally, I would like to find a way of reducing this back to a single variable in just Model. However, I am not up to speed with how all this interacts with the threading stuff that has been added recently. @Corvince or @EwoutH: any thoughts?

[EwoutH]

@HMNS19 are you still (planning to) working on this?

[HMNS19]

Hey @quaquel @EwoutH, thanks for checking in. I was on a short break, but I’m back now and will get this updated

[EwoutH]

Awesome! Let us know if you need anything.