Bugfix for Solara deepcopy bug

[quaquel]

This removes the deepcopy operation from SolaraViz. This changes the behavior. The current behavior is that when reset is hit, the model is reset to the original model passed when initializing SolaraViz. Getting rid of deepcopy implies that this behavior cannot be maintained. So, I replace it here by resetting the model to whatever the current values are for the model parameters (i.e., the user controllable settings, combined with the passed fixed values).

I also added (but this can be its own PR) an extra user input field for a TextInput. This allows for recreating the seed input that we had before #2454.

If we agree on this PR, we might revert #2378.

In passing, I fixed another bug: if model_params was not specified SolaraViz would error in _check_params.

closes #2427

[github-actions]

Performance benchmarks:

Model	Size	Init time [95% CI]	Run time [95% CI]
BoltzmannWealth	small	🔵 -1.5% [-1.9%, -1.0%]	🔵 +0.3% [-0.2%, +0.7%]
BoltzmannWealth	large	🔵 +1.2% [+0.4%, +2.1%]	🔵 -1.0% [-1.8%, -0.1%]
Schelling	small	🔵 -0.9% [-1.5%, -0.4%]	🔵 -2.2% [-3.1%, -1.2%]
Schelling	large	🔵 -0.4% [-1.8%, +1.1%]	🔵 -2.3% [-4.7%, -0.4%]
WolfSheep	small	🔵 -0.6% [-1.0%, -0.2%]	🔵 +0.1% [-0.5%, +0.6%]
WolfSheep	large	🔵 +0.3% [-0.7%, +1.2%]	🔵 -1.8% [-3.1%, -0.6%]
BoidFlockers	small	🔵 +0.2% [-0.4%, +0.7%]	🔵 +0.4% [-0.4%, +1.1%]
BoidFlockers	large	🔵 -0.0% [-0.9%, +0.7%]	🔵 +1.1% [+0.1%, +2.0%]

[Corvince]

Thanks, please let me review/comment before merging this (hopefully later tonight I'll have some time)

[EwoutH]

We need some policy if we want to include this in every example, or handle it differently.

[quaquel]

it simply should not be there and we need to find a cleaner way to run these examples from the command line. Not critical for 3.0 but a prioirty afterwards

[quaquel]

Thanks, please let me review/comment before merging this (hopefully later tonight I'll have some time)

Of course.

[Corvince]

Okay, so I am still a bit undecided on what to think about this. This is now pretty much how the experimental solara_viz approached this problem. But one of the reasons for the rewrite was to have more separated control, thus moving the reinstantiating logic into "ModelCreator". But there is also an argument to be made about "SolaraViz" being the smart component handling all the logic and the other components being dumb. But I lean towards not liking a monolithic approach that much. I don't find the removal of "ModelCreator" very bad, but passing model_params to the ModelController (which purpose is to mainly run the model)... I don't like that very much and I think it hurts reusability. Without this PR you could just pass it a model and control your model interactively (e.g. in a notebook). Now you also have to think about how to instantiate it.

Another reason to use deepcopy, which I unfortunately forgot to mention before, was that at some soon point (hopefully for 3.1) I wanted to introduce the "timeline control" from mesa-viz, which allows users to not just step forward in time, but also to get back to earlier steps. But this requires serializing the model some way or another and just deepcopying is a very fast and simple solution (and if you can't deepcopy an object its also very hard to serialize it another way). Which is a convoluted way of saying: SolaraViz not working with non-deepcopyable objects is maybe not a bug, but just a limitation. And I don't think its a big one. For example #2427 mentions binding and starting a thread in a models __init__ function. I don't think thats a good idea in the first place and very likely solvable in another way.

[Corvince]

Maybe I shouldn't write comments late at night. Here is a more productive comment:

What about keeping ModelCreator, but just declaring model_parameters as a solara.reactive variable inside SolaraViz. Then we can keep the logic inside ModelCreator, but SolaraViz also has the current state. And my comment about not wanting to pass model_params to ModelController was a bit nonsensical. It can be an optional parameter and if it is not provided just fall back to an empty dict. That's enough to restart the model if it has default values.

And we really want to reset the model by recreating it from scratch. Otherwise we keep the bug where the model just resets to the initial state, which might not be step 0.

With these changes I would be in approval with this PR

[quaquel]

Thanks for both comments @Corvince.

But one of the reasons for the rewrite was to have more separated control, thus moving the reinstantiating logic into "ModelCreator".

I was indeed wondering about that, but I did not see the point of ModelCreator after having moved over all the code preceding the creation of UserParam.

What about keeping ModelCreator, but just declaring model_parameters as a solara.reactive variable inside SolaraViz.

This is indeed the cleaner solution and I'll implement that here.

But I lean towards not liking a monolithic approach that much. I don't find the removal of "ModelCreator" very bad, but passing model_params to the ModelController (which purpose is to mainly run the model)... I don't like that very much and I think it hurts reusability.

I agree. This is, in part, why I felt uncomfortable with removing ModelCreator.

Which is a convoluted way of saying: SolaraViz not working with non-deepcopyable objects is maybe not a bug, but just a limitation. And I don't think its a big one.

If we can avoid making that decision now, that would be my preference. Many other things can leave an object in an unpickleable state. For example, generators, nested functions, lambda's, various descriptors, etc. If we need a way of serializing the model at some future point, we might look into using dill instead of pickle, although personally, I would prefer finding a cleaner solution than either one. This issue is directly related to caching a mesa run, and I believe that the observable/signal stuff might be a cleaner route for this. We should discuss this separately.

A separate reason for changing the behavior of SolaraViz is that, from a UI point of view, it makes more sense to me that reset is in line with the user-settable parameters rather than whatever state the model was in when starting the UI. For example, you start with a 20 x 20 grid, but the model_params uses 10 x 10. Everytime you hit reset, you now would go back to 20 x 20, but there is no way to interact with that version of the model because the moment you change any slider or so, it reverts to 10 x 10. This seems strange behavior to me.

[Corvince]

Okay, than I think we are in line with how to update this PR! Great

A separate reason for changing the behavior of SolaraViz is that, from a UI point of view, it makes more sense to me that reset is in line with the user-settable parameters rather than whatever state the model was in when starting the UI.

Exactly, this is what I was refering to with

And we really want to reset the model by recreating it from scratch. Otherwise we keep the bug where the model just resets to the initial state, which might not be step 0.

So, yeah, independent of the deepcopy/pickle subject this should be changed in line with how this PR does it (just making model_params an optional argument). (On the discussion itself, yes, should be discussed somewhere else, but note that deepcopy and pickle are different - pickle is much more restrictive in what can be pickled)

[quaquel]

I think I figured it out, but @Corvince please check the changes in solara_vis to make sure I don't do anything stupid. I have tested it with Boltzman and it works as intended. I'll fix the rest including docs later.

In short:

1. ModelCreator is back.
2. I create a reactive variable based on model_parameters. This is done in SolaraViz and then shared between ModelCreator and ModelController.
3. However, this does require moving the parameter check (i.e., _check_model_params) and splitting (i.e., split_model_params) to SolaraViz. It might be possible to keep these in ModelCreator but that would make creating the reactive variable in SolaraViz a bit more complicated. @Corvince let me know your thoughts.
4. ModelCreator takes the reactive version of model_parameters and the user settable parameters.

ps. check_param_is_fixed might be a source for bugs. Imagine a model that takes a dict as a keyword argument. It is currently not possible to pass a default fixed value for this dict to SolaraViz because check_param_is_fixed assumes that all dicts are specifications for user interaction items. I think the longer-term objective would be to move away from purely dict-based specifications of interaction elements (and agent_portrayal, property_layer portrayal, etc.).

[Corvince]

Looking good, 2 more comments. Also like the additional small fixes you added (cleanup of seed,)

[quaquel]

I made the requested changes. I also added some additional type hinting and changed a call to a deprecated function (from the API reorganization) with the updated correct function call. I again tested various examples and they seem to be working correctly.

[Corvince]

Awesome work! Maybe more of a clarification question: I previously suggested adding a * here to have keyword-only arguments. Maybe I am wrong here and thats why you ignored it, but my understanding was that this is how it works

def foo(bar, baz=2):
    ...
# all of this works:
foo(1)
foo(3, baz=4)
foo(5, 6)
# Compared to
def foo2(bar, *, baz):
    pass

foo2(1) # works
foo2(3, baz=4) # works
foo2(3, 4) # doesnt work

My thinking behind this was to have API stability. If we later change the signature to

def ModelController(model, awesome_new_arg, model_parameters=None)

we don't have a breaking change. right now it would be breaking if model_parameters is only provided as a positional argument.

Does that make sense? Is that how it works?

[quaquel]

I was wondering what you ment, but this makes good sense. I'll update it accordingly.

[Corvince]

Great, feel free to merge afterwards