widgetti / solara

A Pure Python, React-style Framework for Scaling Your Jupyter and Web Apps
https://solara.dev
MIT License
1.75k stars 131 forks source link

Copying objects is not enough to trigger rerender #680

Open jsparger opened 1 month ago

jsparger commented 1 month ago

I am experiencing what I believe is a bug in the state management, specifically solara.use_state when it comes to objects. I understand that if I try to update the state using the same object wihout making a copy, then rendering may not be triggered. However, in order to get the page to rerender after updating the object I must also:

  1. Store the copied object in a temporary variable before calling set_value
  2. Perform copy.deepcopy if the objects are nested e.g. a list of dicts.

It could be that (1) is a weirdness of python itself, I'm not sure. I hope someone can comment on that. But (2) seems like a bug to me. The state comparison for objects should have to do with the object I pass in, not its contents. So it seems like, as long as I have copied the list itself, I should get a rerender, even if the objects contained within the list are not copied.

Below is some code illustrating this behavior.

import solara
import json
import copy

@solara.component 
def test_component():
    count, set_count = solara.use_state(0)
    dictionary, set_dictionary = solara.use_state({"count": count})
    list_of_dicts, set_list_of_dicts = solara.use_state([{"count": count}])

    def update_dictionary():
        dictionary["count"] = count
        # set_dictionary(dictionary) # no rerender (as expected, same object)
        # set_dictionary(dictionary.copy()) # no rerender -- why not? object is different
        set_dictionary(copy.deepcopy(dictionary)) # still no rerender

    def update_dictionary_with_tmp():
        # tmp = dictionary # no rerender (as expected, same object)
        tmp = dictionary.copy() # it works! but why is storing the copy in another variable important?
        # tmp = copy.deepcopy(dictionary) # it also works
        tmp["count"] = count
        set_dictionary(tmp)

    def update_list_of_dicts():
        list_of_dicts[0]["count"] = count
        # set_list_of_dicts(list_of_dicts) # no rerender (as expected, same object)
        # set_list_of_dicts(list_of_dicts.copy()) # no rerender -- why not? object is different
        set_list_of_dicts(copy.deepcopy(list_of_dicts)) # still no rerender

    def update_list_of_dicts_with_tmp():
        # tmp = list_of_dicts # no rerender (as expected, same object)
        # tmp = list_of_dicts.copy() # no rerender! This is good enough for a dict, but not list of dicts! Shouldn't the state be based on the list object being different, not its children? 
        tmp = copy.deepcopy(list_of_dicts) # it works -- but why is deepcopy AND tmp variable required?
        tmp[0]["count"] = count
        set_list_of_dicts(tmp)

    solara.Markdown("# Test")
    solara.Preformatted(json.dumps(count, indent=2))
    solara.Button("count += 1", on_click=lambda: set_count(count+1))
    solara.Preformatted(json.dumps(dictionary, indent=2))
    with solara.Row():
        solara.Button("update dict", on_click=update_dictionary)
        solara.Button("update dict with tmp", on_click=update_dictionary_with_tmp)
    solara.Preformatted(json.dumps(list_of_dicts, indent=2))
    with solara.Row():
        solara.Button("update list of dicts", on_click=update_list_of_dicts)
        solara.Button("update list of dicts with tmp", on_click=update_list_of_dicts_with_tmp)

my python and solara versions are:

$ python --version
Python 3.11.9

$ pip freeze | grep solara
solara==1.32.2
solara-server==1.32.2
solara-ui==1.32.2

I hope someone can help me with this. Thanks.

jsparger commented 1 month ago

It seems the need to copy to a temporary object (1 in my list above) is related to the fact that solara.use_state is returning the actual state object instead of copy of it. This means we can directly modify the state object. It doesn't trigger a rerender but probably screws up whatever comparisons solara is doing internally to decide if a render is needed

@solara.component 
def test_component():
    count, set_count = solara.use_state(0)
    dictionary, set_dictionary = solara.use_state({"count": 0})

    def update_dictionary_directly():
        dictionary["count"] += 1 # I don't use set_dictionary, but dictionary is updated.
        set_count(count + 1) # just to trigger a rerender

    solara.Markdown("# Why can I set the value of dictionary directly?")
    solara.Markdown("## Count")
    solara.Preformatted(json.dumps(count, indent=2))
    solara.Markdown("## dictionary")
    solara.Preformatted(json.dumps(dictionary, indent=2))
    solara.Button("count += 1", update_dictionary_directly)

I had assumed use_state would return a copy of the state instead of returning it directly. I guess this would impose a lot of restrictions on what can be used as state though. Still, this is a footgun. Sorry if I missed it in the docs.

This KIND OF explains (1) from my question above. It was the order of operations between copy and update that mattered, not copying to a temporary variable. But it still does not explain why a rerender is not triggered when I pass in a copied dictionary. It is still a different object after all!

Given this behavior in (1) and the need for deepcopy in (2), it seems like solara is doing some kind of non-trivial comparison of the contents of objects. That seems kind of fishy because that's hard to do in a general way. So I guess the question is, what are the internal comparisons solara is doing to decide whether the state should update? Why is deepcopy needed for a list of dicts?