solara icon indicating copy to clipboard operation
solara copied to clipboard

Copying objects is not enough to trigger rerender

Open jsparger opened this issue 1 year ago • 2 comments

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 avatar Jun 11 '24 16:06 jsparger

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?

jsparger avatar Jun 11 '24 18:06 jsparger

Hi,

solara/reacton does not make a copy, and mutations are not something you should do. We recognize this footgun, and plan to detect this: https://github.com/widgetti/solara/pull/595 so it becomes clear to the user what to do.

But it still does not explain why a rerender is not triggered when I pass in a copied dictionary.

We do equality comparison, not object identify comparison, hope that explain it.

Regards,

Maarten

PS: sorry for answering this so late. Because you replied to yourself (I saw a reply) I thought I already answered this.

maartenbreddels avatar Aug 20 '24 08:08 maartenbreddels