Add UserInputs component by Corvince · Pull Request #1788 · mesa/mesa

Corvince · 2023-09-03T18:31:12Z

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 · 2023-09-03T19:52:53Z

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

mesa/experimental/jupyter_viz.py

codecov · 2023-09-04T07:56:11Z

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 · 2023-09-04T07:56:15Z

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

rht · 2023-09-04T09:49:43Z

mesa/experimental/jupyter_viz.py

+        if input_type == "SliderInt":
+            solara.SliderInt(
+                label,
+                value=options.get("value"),


Why shouldn't the value be a use_state hook?

I meant, use_reactive.

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?

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.

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.

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)?

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 · 2023-09-07T07:42:33Z

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 · 2023-09-07T08:28:23Z

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 · 2023-09-07T11:39:27Z

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 · 2023-09-07T12:41:55Z

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 · 2023-09-07T12:47:33Z

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 · 2023-09-07T13:52:14Z

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 · 2023-09-07T14:46:09Z

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 · 2023-09-07T14:49:38Z

@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 · 2023-09-07T17:29:13Z

@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 · 2023-09-09T11:54:28Z

@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

rht reviewed Sep 4, 2023

View reviewed changes

mesa/experimental/jupyter_viz.py Outdated Show resolved Hide resolved

Corvince force-pushed the solara-user-inputs branch from 3912beb to 541442c Compare September 4, 2023 07:54

rht reviewed Sep 4, 2023

View reviewed changes

Corvince added 4 commits September 7, 2023 09:38

Add UserInputs component

bfdcc5f

update tests

30341cb

remove global state dependency from UserInputs

90e1243

Simplify UserInputs change handler

8d6c673

Corvince force-pushed the solara-user-inputs branch from e9fe7f3 to 8d6c673 Compare September 7, 2023 07:41

rht approved these changes Sep 7, 2023

View reviewed changes

rht merged commit d451ef0 into main Sep 7, 2023

rht deleted the solara-user-inputs branch September 7, 2023 12:52

Corvince mentioned this pull request Sep 7, 2023

Fix: Remove dict merge operator, python 3.8 compat #1793

Merged

tpike3 added this to the Release 2.1.2 milestone Sep 19, 2023

rht mentioned this pull request Sep 30, 2023

Solara: Let users rewind sessions #1805

Closed

Corvince mentioned this pull request Feb 19, 2024

docs: Convert api/*.rst -> api/*.md via rst2myst #2035

Merged

Uh oh!

Conversation

Corvince commented Sep 3, 2023

Uh oh!

Corvince commented Sep 3, 2023

Uh oh!

Uh oh!

codecov bot commented Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Corvince commented Sep 4, 2023

Uh oh!

rht Sep 4, 2023

Choose a reason for hiding this comment

Uh oh!

rht Sep 4, 2023

Choose a reason for hiding this comment

Uh oh!

Corvince Sep 4, 2023

Choose a reason for hiding this comment

Uh oh!

rht Sep 4, 2023

Choose a reason for hiding this comment

Uh oh!

Corvince Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rht Sep 5, 2023

Choose a reason for hiding this comment

Uh oh!

Corvince Sep 5, 2023

Choose a reason for hiding this comment

Uh oh!

Corvince commented Sep 7, 2023

Uh oh!

rht commented Sep 7, 2023

Uh oh!

Corvince commented Sep 7, 2023

Uh oh!

rht commented Sep 7, 2023

Uh oh!

rht commented Sep 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rht commented Sep 7, 2023

Uh oh!

Corvince commented Sep 7, 2023

Uh oh!

rht commented Sep 7, 2023

Uh oh!

Corvince commented Sep 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tpike3 commented Sep 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Sep 4, 2023 •

edited

Loading

Corvince Sep 5, 2023 •

edited

Loading

rht commented Sep 7, 2023 •

edited

Loading

Corvince commented Sep 7, 2023 •

edited

Loading