Skip to content

Introduce bindable dataclass fields#3987

Merged
falkoschindler merged 11 commits intozauberzeug:mainfrom
balex89:bindable-dataclass
Jan 23, 2025
Merged

Introduce bindable dataclass fields#3987
falkoschindler merged 11 commits intozauberzeug:mainfrom
balex89:bindable-dataclass

Conversation

@balex89
Copy link
Copy Markdown
Contributor

@balex89 balex89 commented Nov 13, 2024

This PR follows up an idea #3957 to introduce bindable dataclasses.dataclass fields.

The main challenge is to marry bindableProperty with dataclasses.field, and by that preserve native dataclass features.
The proposed idea is to use a wrapper around dataclasses.field that updates passed metadata, adding nicegui-specific options (for now just a "bindable" flag). Then use it to add bindableProperties retroactively on dataclass type postprocessing (much like it is done in dataclasses_json.config)

Usage example

@bindable_dataclass
@dataclass
class MyClass:
    x: float = dataclass_bindable_field(default=1.0)

Known tradeoffs

  • Access to default field value through class attribute (e.g. MyClass.x) is lost.

@falkoschindler falkoschindler self-requested a review November 15, 2024 12:24
@falkoschindler falkoschindler added the feature Type/scope: New or intentionally changed behavior label Nov 15, 2024
Copy link
Copy Markdown
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the pull request, @balex89!

@rodja and I thought about the API quite a bit. Requiring the @bindable_dataclass decorator and the field wrapper feels a bit verbose. In #3957 you wrote "One might want to be able to pick specific attributes only". But let's question this requirement. Do we really need to handle this use case? I know, the bindable_field function was kind of my idea. But if 99% of our users want to mark all fields as bindable anyway, your initial approach of converting all fields would be much simpler and nicer to use. What do you think?

Apart from that we think we should merge @dataclass and @bindable_dataclass so that the user doesn't have to write both decorators and remember the right order. The new decorator would simply forward all keyword arguments to the dataclass decorator.

@balex89
Copy link
Copy Markdown
Contributor Author

balex89 commented Nov 26, 2024

But if 99% of our users want to mark all fields as bindable anyway, your initial approach of converting all fields would be much simpler and nicer to use

@falkoschindler, I do agree on making that a default behavior. And yet, shouldn't we leave an option to specify fields explicitly for those 1%? For instance:

@bindable_dataclass
class A:
    x: int  # bindable
    y: int  # bindable
    z: int  # bindable

@bindable_dataclass(bindable_fields=['x', 'y'])
class B:
    x: int  # bindable
    y: int  # bindable
    z: int  # not bindable

That doesn't seem to complicate implementation or interface much while providing full control on attribute picking, if ever needed.

As was said above, we forward additional kwargs to dataclass().

@falkoschindler
Copy link
Copy Markdown
Contributor

@balex89 You're right. Even though we could add such an additional attribute later on demand, we can as well add it right now. 👍

@balex89
Copy link
Copy Markdown
Contributor Author

balex89 commented Nov 27, 2024

API is simplified according to comments above.

@falkoschindler
Copy link
Copy Markdown
Contributor

Thanks for the update, @balex89!

I just tested with the following code:

@binding.bindable_dataclass  # version 1
# @binding.bindable_dataclass(bindable_fields=['x'])  # version 2
# @binding.bindable_dataclass(slots=True)  # version 3
class A:
    x: float
    y: float

a = A(x=1, y=2)  # a should be of type A
x = a.x  # x should be of type float

with ui.row():
    ui.number('X').bind_value(a, 'x')
    ui.slider(min=0, max=10).bind_value(a, 'x')

with ui.row():
    ui.number('Y').bind_value(a, 'y')
    ui.slider(min=0, max=10).bind_value(a, 'y')

It mostly works as expected, but there are still some subtle issues:

  1. While version 1 preserves the correct type for a and a.x, with version 2 both type hints are Any. This is suboptimal.
  2. Version 3 complains about a missing attribute ___x. The reason might be that dataclasses work differently with slots=True. Maybe we can catch this case and raise an informative exception.
  3. Even if I set ui.run(binding_refresh_interval=1.0, ...) to throttle updates for "active links", both bindings update instantly. Maybe my test is flawed and there's a simple explanation. But I wonder why "y" behaves like the bindable property "x".

@balex89
Copy link
Copy Markdown
Contributor Author

balex89 commented Dec 3, 2024

@falkoschindler, I pushed some improvements. Futher down your list:

  1. Type hints optimized.
  2. Using slots=True now forbidden and raises a ValueError. Also i tested every other option and found frozen=True also complains about attribute setting. So now both this options are handled.
  3. As far as i understand, your test does not perform as you might expect because of recursive value update propagation. When you, say, use a slider, this event calls _propagate on slider value, which calls it on dataclass field value, witch eventually immediately updates number value.
    I added this code lines to your test to make difference observable:
    ui.timer(0.2, lambda: list(map(
        lambda attr, value: setattr(a, attr, value),
        ['x', 'y'], [(a.x + 1) % 11] * 2
    )))
    
    ui.run(binding_refresh_interval=2.0)
    Here i automatically update both fields rapidly. Setting a huge binding_refresh_interval will show the difference.

@falkoschindler
Copy link
Copy Markdown
Contributor

falkoschindler commented Jan 17, 2025

Hi @balex89, I finally had another look into this PR.

Regarding point 3: You're totally right. Updating a bindable property triggers immediate propagation, even if active links are involved. To reproduce the delay we need to update the non-bindable property directly.

The pull request is almost ready to merge. But I'm still seeing two mypy errors which would break the build chain when merged into main:

$ mypy nicegui/binding.py
nicegui/binding.py:197: error: "bindable_fields" argument must be a True or False literal  [literal-required]
nicegui/binding.py:223: error: Need type annotation for "dataclass"  [var-annotated]

I don't understand why bindable_fields should be True or False. And I'm struggling to find the right type annotation for dataclass. Would you mind having another look? Thanks!

@balex89
Copy link
Copy Markdown
Contributor Author

balex89 commented Jan 20, 2025

Hi @falkoschindler!
Issues with mypy seem to be fixed now.

Turns out I initially misused dataclass_transform arbitrary param option. It has nothing to do with dataclass decorator params extension.

@falkoschindler falkoschindler added this to the 2.11 milestone Jan 23, 2025
Copy link
Copy Markdown
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks again, @balex89!
All linters and type checkers seem happy now. Let's merge!

@falkoschindler falkoschindler merged commit 1fb2d12 into zauberzeug:main Jan 23, 2025
@1081
Copy link
Copy Markdown

1081 commented Jan 26, 2025

I am very interested in this feature. Since I need a simpl way to create models with many parameters I can bind without creating "active links".

Not sure if my solution is robust. I changed the init method of the binding.BindableProperty() descriptor to accept an optinal name. With this i can create a bindable model like this model = create_model(["a", "b", "c"]).

from typing import Any, Callable, Optional

from nicegui import binding, ui

def __init__mod(self, on_change: Optional[Callable[..., Any]] = None, name="") -> None:
    self._change_handler = on_change
    self.name = name
    print("--> __init__", self.name)


binding.BindableProperty.__init__ = __init__mod

def create_model(attr_names):
    class Model:
        pass

    for name in attr_names:
        setattr(Model, name, binding.BindableProperty(name=name))

    model = Model()

    for name in attr_names:
        setattr(model, name, None)

    return model


model = create_model(["a", "b", "c"])

print(model.a, model.b, model.c)

ui.run(reload=False)

@MateoSaez
Copy link
Copy Markdown

  1. While version 1 preserves the correct type for a and a.x, with version 2 both type hints are Any. This is suboptimal.

I think the hints still show Any

@falkoschindler
Copy link
Copy Markdown
Contributor

@MateoSaez It works for me. Maybe your IDE or Pylance extension is not up to date?

Screenshot 2025-04-19 at 15 34 29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature Type/scope: New or intentionally changed behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants