Support simulators in SolaraViz

[quaquel]

This adds support for Simulator to SolaraViz. This closes #2428.

While working on it, I discovered a few subtle bugs in experimental.devs and the wolf-sheep benchmark model. These changes have been merged via #2478. This PR only contains the updates to solara_viz.The modular nature of SolaraViz works great and makes it easy to simply add a SimulatorController. However, as part of this, there are two key behavioral changes introduced:

1. force_update moved into the do_step method, rathen than being called via monkey patching model.step. This is a required change to make it work with simulator. ABMSimulator schedules model.step when simulator.setup(model) is called. This happens before the monkey patching, so the event calls the non-monkey patched step method, resulting in no force_update being called.
2. Previously, any change in the sliders would effectively reset the model by creating a new model instance. Now a new instance is only created when reset is hit. This means you can move the slider around while the model is running without interrupting the ongoing run. Again, this change is needed to cleanly seperate the Controller and the Creator. We might even consider renaming the ModelCreator....

I also updated wolf-sheep and rewrote it to use event scheduling.

Given some of the SemVer discussions: the changes in solara_viz are fully backward compatible.

[github-actions]

Performance benchmarks:

Model	Size	Init time [95% CI]	Run time [95% CI]
BoltzmannWealth	small	🟢 -4.7% [-5.8%, -3.6%]	🔵 -0.2% [-0.4%, +0.1%]
BoltzmannWealth	large	🔵 -1.1% [-1.6%, -0.6%]	🔵 -3.8% [-5.8%, -1.9%]
Schelling	small	🔵 -0.9% [-1.3%, -0.5%]	🔵 -0.3% [-0.8%, +0.1%]
Schelling	large	🔵 -0.0% [-1.0%, +0.8%]	🔵 -3.6% [-4.9%, -2.2%]
WolfSheep	small	🔵 +0.6% [+0.3%, +0.8%]	🔴 +147.1% [+138.7%, +155.0%]
WolfSheep	large	🟢 -3.8% [-4.3%, -3.2%]	🔴 +67.1% [+61.4%, +72.7%]
BoidFlockers	small	🔵 -0.8% [-1.5%, -0.0%]	🔵 -0.6% [-1.4%, +0.2%]
BoidFlockers	large	🔵 +0.7% [-0.3%, +1.9%]	🔵 -0.8% [-1.3%, -0.4%]

[quaquel]

Just a quick remark: the issue of the solara_viz wrapping of step is a nice example that with #2291 merged (I know I still need to write the tests) would become very simple to solve. One could declare mesa.steps, so the clock, Observable. SolaraViz could then simply subscribe to signals by doing model.observe("steps", "on_change", force_update). No further on the fly monkey patching as currently done would be required.

[EwoutH]

Thanks for working on this. Do you think it will be a user-API breaking change?

[quaquel]

No, this won't be API breaking. For solara_viz, it adds an optional keyword argument. For DEVS, it slightly changes the behavior of some methods but does not change the public API.

[EwoutH]

Great, let me know when it's ready for review.

[Corvince]

Just a quick remark: the issue of the solara_viz wrapping of step is a nice example that with #2291 merged (I know I still need to write the tests) would become very simple to solve. One could declare mesa.steps, so the clock, Observable. SolaraViz could then simply subscribe to signals by doing model.observe("steps", "on_change", force_update). No further on the fly monkey patching as currently done would be required.

True.

But for the issue at hand, I think we don't actually need to monkey patch step at all. It should be enough to call force_update() at do_step(). Monkey patching was I think mainly to allow triggering updates from the outside when stepping through the reactive model. But this should be handled separately from how solara viz works internally. Could you try if that works?

[quaquel]

Could you try if that works?

If I only add force_update to do_step in the new SimulatorController, we get very interesting but weird behavior. The event scheduling side works normal, but model.step is never called. If I next remove connect_to_model from SolaraViz, everything seems to work as intended. @Corvince, I assume you wanted me to try the combination of both changes because it effectivley removes the existing monkey patch of model.step.

[Corvince]

I did not anticipate the weird behavior, but yes, I think the monkey patching is actually not needed.

[quaquel]

I did not anticipate the weird behavior, but yes, I think the monkey patching is actually not needed.

The weird behavior comes from ABMSimulator and how this interacts with model.step. I'll remove the monkey patching, clean up the PR, run some more tests with several examples, and then report back here when its ready for a last round of review.

[EwoutH]

I will let the SolaraViz part to Corvince, a few comments on the DEVS side

[EwoutH]

Since there are some substantial changes to DEVS, it might be nice to make it two PRs: First one with the DEVS changes and then one with the SolaraViz ones.

[quaquel]

Since there are some substantial changes to DEVS, it might be nice to make it two PRs: First one with the DEVS changes and then one with the SolaraViz ones.

I was afraid that you would suggest that, and I agree (but don't want to). I'll do it tomorrow.

[EwoutH]

Yes, splitting off the DEVS fixes was the right call. This is now a beautifully clean changeset I will gracefully hand over to @Corvince.

[Corvince]

4 Things i noticed on a first look

Previously, any change in the sliders would effectively reset the model by creating a new model instance. Now a new instance is only created when reset is hit. This means you can move the slider around while the model is running without interrupting the ongoing run. Again, this change is needed to cleanly seperate the Controller and the Creator. We might even consider renaming the ModelCreator....

1. From looking at the code I don't see this change. I'm definitely not against this. I think this should be the way to go, especially since I also want to add at some stage the possibility to change some parameters of a running model. But I don't see any code changes that would do this now.

2. Right now this actually is a breaking change, since the third positional argument is changed. But I'd consider this a bug, since everything after model and components should actually be keyword only parameters (separated by a *). I thought this was already the case, specifically to make such changes as here non-breaking.

3. Following from 2 is easy that existing models might end up with Simulator controller because the logic only checks if the simulator is not None and not if it's actually a simulator instance

4. There seems to be a lot of code duplication between ModelController and SimulatorController. Maybe just subclass ModelController and overwrite step and reset? Not sure

That's it for now, should be easy to change/clarify and then we are good to go 👍

[quaquel]

Thanks! I addressed points 1-3.

1. In the cleanup, rebase and split, this one line change dropped out. I added it back in, so it's clear what I changed. For the longer-term vision, it sounds great. I actually think that I can rig that up with observables quite quickly....; imagine having a mouse over on agents, getting a table with the attribute values, and being able to go in and change any of them. I really like the idea.
2. Added the *,
3. Changed it from is None to not isinstance

I agree on point 4. I'll take a closer look hopefully later today and see if I can reduce the duplication.

[quaquel]

I had a closer look at point 4. Indeed, the only difference between ModelController and SimulatorController are the do_step and do_reset inner functions. So do avoid code duplication, it would make sene to factor those out. However, this is not entirely trivia. First, despite their camelcase naming, ModelController and SimulatorController are functions, not classes, so subclassing is not possible. Second, both do_step and do_reset are inner functions (i.e., closures) and thus rely on the variables available in the outer function, specifically model. If we refactor do_step and do_reset into stand-alone functions, you would need to use functools.partial to pass in model, or define them in SolaraViz. Neither is a really clean solution. In short, I am happy to take a step at the refactoring to avoid code duplication, but I am also fine with leaving that for now.

[Corvince]

True, let's leave it for now!