Add UserInputs component

[Corvince]

Upgraded the make_user_input function into a solara component.
Updated the model parameters handling in general
Reordered the JupyterViz Component: Now all UI elements are rendered at the bottom of the function.

[Corvince]

oopsie thanks to @rebeccadavidsson we had tests for make_user_params! Updated the first test right now, leaving the others for tomorrow

[codecov]

Codecov Report

Patch coverage: 75.00% and project coverage change: +0.32% 🎉

Comparison is base (9572e65) 81.72% compared to head (8d6c673) 82.05%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1788      +/-   ##
==========================================
+ Coverage   81.72%   82.05%   +0.32%     
==========================================
  Files          15       15              
  Lines         881      886       +5     
  Branches      189      191       +2     
==========================================
+ Hits          720      727       +7     
+ Misses        136      135       -1     
+ Partials       25       24       -1     

Files Changed	Coverage Δ
mesa/experimental/jupyter_viz.py	35.00% <75.00%> (+3.88%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Corvince]

Added missing tests and removed the global state - thx @rht for the heads-up

[rht]

Why shouldn't the value be a use_state hook?

[rht]

I meant, use_reactive.

[Corvince]

No strict rule here, but I try to follow a one-directional data flow model here. JupyterViz "owns" the state here, i.e. we just pass the current parameters down to UserInputs. Any changes are brought back via the on_change method and the parent component decides what to do with the changes.

Contrast this with keeping the state inside UserParams. It wouldn't be clear how to extract the current value back to the parent component (JupyterViz) so we can act on a change. I don't know enough about how solara yet, so maybe this would also be possible. I am mainly following React best practices here - for a reference (and a great start to react in general) see https://react.dev/learn/thinking-in-react

Or maybe you meant something completely different?

[rht]

It's in the very simplest example of Solara that you pass in reactive variable to the user input value: https://github.com/widgetti/solara/blob/master/solara/website/pages/examples/basics/sine.py. I find the on_value construct too convoluted, not to mention its nested functions for achieving the same functionality.

[Corvince]

But the question is where should that state live? In the sine example global state is used. Which I did in the previous iteration, where you - correctly - blamed me for it being not reusable that way.

I think the current way is very simple. At the top level we have the model_parameters state variable that is used for the model creation. Its a simple variable-value dictionary. And we have the list of user-settable parameters. This list is passed independently of the model_parameters to UserParams. And any changes inside UserParams are handled at the top level by updating the model_parameters. This way we have maximal reusability of the UserParams component.

Now if we were to define the reactive variables at the JupyterViz component and pass them to UserParams we have a tight coupling between the user parameters and the model parameters and the logic resides both in JupyterViz and UserParams. UserParams can't be used without model_parameters although that dependency is not strictly required.

And if we define the reactive variables inside UserParams than we are back to my problem of not knowing how to get the updated value back to the parent component.

I fully agree that it appears a bit convoluted at first and especially the nested functions are not nice - in JS the function definition could simply be written as (value) => (name) => on_change(name, value). I don't know if it would be possible to have a similar concise syntax in Python. But the important part is that it is not the same functionality. Just defining value as a reactive value doesn't help us changing the model_parameters dictionary.

[rht]

The nested functions are just one of the complications.
It's that you have to do handle_* at the JupyterViz level, and do something along the line of subsequent on_change's inside the subcomponents. Simpler would be to have this at the parent component, and at the last leg of the child component.

I'm not sure what you meant by UserParams. Is it a hypothetical reactive variable, a Solara component, a class, or something else not yet implemented? If it is initialized inside JupyterViz, why can't JupyterViz be notified of its change when you can add do solara.use_reactive(..., on_change=do_something)?

[Corvince]

Sorry about the UserParams confusion. I meant the UserInputs component implemented in this PR.

It makes sense for the handle_ function to be at JupyterViz because it changes the model_parameters state - where else should that happen.

Frankly I don't understand the rest of your comment. Maybe it would be more productive if you could have some concrete advice on how to move forward. Is this blocking for this PR or could this be an improvement in a further PR? I am really not sure anymore what your critique revolves around and how to move on

[Corvince]

I updated and simplified the change_handler. Would this be sufficient @rht or do you need anything else for this PR to be mergeable?

[rht]

I almost agree with your change to make it one-way here. Also that handle_change_model_params is a thin wrapper of set_model_parameters, similar to the inverse data flow recommendation in https://react.dev/learn/thinking-in-react#step-5-add-inverse-data-flow.

If you can explain why we shouldn't use use_reactive and I am convinced, then I will merge:
Solara's use_reactive doesn't have an equivalent API in React. It seems to be a convenient API sugar specific to Solara. It allows child components to modify the parent state directly. The way for the parent component to be notified of the change event is to specify on_change during initialization. This works. And so what is the downside of this? The state is still defined in the parent scope, and the updater is passed down from the parent, just like set_model_parameters.

[Corvince]

Thanks for your swift reply and clarification.

And I feel like I might be missing something super obvious. But I have to ask you again, because I still don't get it.

For which variable should use_reactive be used?

[rht]

I meant the subset of the model parameters that are meant to be changed by UserInputs, as in the current main.

But according to Solara dev (https://discord.com/channels/1106593685241614489/1106593686223069309/1138104278792290355), use_reactive is not meant to be defined inside a for loop or conditional. So for this reason alone, your solution is more favorable, unless you know a simple way to use 1 use_reactive without a for loop (e.g. passing the whole dict as a state).

[rht]

To put it simply, in general

a, set_a = solara.use_state(...)
# later on
set_a(...)

a = solara.use_reactive(...)
# later on, not sure
a.value = ...
# or
a.something(...)

The 2 methods are 2 different API's of doing the same thing. The advantage of the latter:

This is useful for implementing component that accept either a reactive variable or a normal value along with an optional on_change callback.

The disadvantage of the latter is that for people coming from React, it's a different API.

[rht]

It turns out that there is a failed CI test on Python 3.8. I didn't realize it happening because I'm used to the coverage often giving "x". The reason is that | is not supported on 3.8.

[Corvince]

Good catch. Same for me with the single failing check.

Is #1756 going to be merged soon or should I submit a PR to fix this? I mean the PR should be simple enough ( I think the | is basically syntactic sugar). But I won't be able to do any laptop work until next week

[rht]

@jackiekazil @tpike3 there should be an option on GH to force require specific GH Actions check to pass before merging, that can be enabled.

#1756 is not going to be merged soon, because we are aiming for a quick 2.1.2 release.

[Corvince]

@jackiekazil @tpike3 there should be an option on GH to force require specific GH Actions check to pass before merging, that can be enabled.

#1756 is not going to be merged soon, because we are aiming for a quick 2.1.2 release.

That's a good idea. I enabled required passing checks for all the Linux builds (3.8 through 3.11). For future reference: to be found under branch protection rules

[tpike3]

@jackiekazil @tpike3 there should be an option on GH to force require specific GH Actions check to pass before merging, that can be enabled.
#1756 is not going to be merged soon, because we are aiming for a quick 2.1.2 release.

That's a good idea. I enabled required passing checks for all the Linux builds (3.8 through 3.11). For future reference: to be found under branch protection rules

Yep, this is pretty easy, in the dev meeting we can just add the rules we want