Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add bind function and associated reactive API #460

Merged
merged 30 commits into from
Sep 23, 2023
Merged

Add bind function and associated reactive API #460

merged 30 commits into from
Sep 23, 2023

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Feb 23, 2021

Adds param.bind function which acts like functools.partial but evaluates the current value of a parameter.

  • Add tests
  • Add docs
  • Decide whether to offer hooks for param.depends and param.bind so that they can accept other objects and interpret them as parameters (making them behave like pn.depends and pn.bind).

reactive API

reactive acts as a proxy for the object it is wrapping, therefore the API of reactive itself should be minimal to avoid overriding the behavior of the wrapped object. Here is the current reactive public API. When reviewing please consider the naming and consider whether it is likely to clash with objects we are wrapping.

  • resolve: Evaluates and returns the current value of the reactive expression
  • set_input: If the input to the expression was a scalar value (i.e. not a reference such as Parameter or bound function) then this allows updating the input to the expression
  • watch: Watches the expression

Additionally we introduce a few additional methods to support certain operations that cannot easily be replicated using standard Python operations:

  • bool_: Casts the object to bool (equivalent to .pipe(bool)), needed because __bool__ must return a boolean value, not just something that will evaluate to a boolean value.
  • is_: Allows performing identity comparison on the underlying object, e.g. obj is None
  • len: Computes the len of the object, since __len__ cannot return anything except an int.
  • pipe: Allows chaining an arbitrary function on the reactive expression (modeled on pd.DataFrame.pipe)
  • when: Construct a new reactive object that only emits new events when the provided reference updates (required because performing actions based only on some external trigger, e.g. clicking a button, is hard to express otherwise).
  • where: Construct that allows returning either value A or value B (required because A if ... else B is hard to express otherwise).

@philippjfr
Copy link
Member Author

philippjfr commented Feb 23, 2021

@jlstevens suggested the hooks could take the form of:

{class_to_check_isinstance:['param1', 'param2',...], ...}

However that doesn’t seem sufficient, e.g. in panel I actually create synthetic parameters around ipywidgets:

    from .pane.ipywidget import IPyWidget
    if IPyWidget.applies(arg) and hasattr(arg, 'value'):
        name = type(arg).__name__
        if name in ipywidget_classes:
            ipy_param = ipywidget_classes[name]
        else:
            ipy_param = param.parameterized_class(name, {'value': param.Parameter()})
        ipywidget_classes[name] = ipy_param
        ipy_inst = ipy_param(value=arg.value)
        arg.observe(lambda event: ipy_inst.param.set_param(value=event['new']), 'value')
        return ipy_inst.param.value

@jlstevens
Copy link
Contributor

I think a dictionary is always appropriate as you have a match condition then a way to get the parameters. So maybe a bunch of things could be supported:

{class1_to_check_isinstance:['param1', 'param2',...], 
class2_to_check_isinstance:function_returning_parameter_list_given_instance, 
predicate_function:function_returning_parameter_list_given_instance
... }

Then you can use as complicated a format as necessary.

@jbednar
Copy link
Member

jbednar commented Feb 23, 2021

Looks good to me but probably needs watch support as suggested in holoviz/panel#1999.

@jlstevens
Copy link
Contributor

I think a watch argument is definitely required now panel supports it: I don't think having the panel and param versions of bind diverge would be a good thing.

@jbednar jbednar added this to the v1.10.2 milestone May 10, 2021
@jbednar jbednar modified the milestones: v1.11.0, v1.11.1 Jul 2, 2021
@MridulS MridulS modified the milestones: v1.11.2, v1.12.1 Feb 7, 2022
@droumis droumis modified the milestones: v1.12.x, 2.0 Jan 16, 2023
@maximlt
Copy link
Member

maximlt commented Apr 5, 2023

@philippjfr do you need this in 2.0?

@jbednar
Copy link
Member

jbednar commented May 12, 2023

@philippjfr , up to you to declare this ready for review.

@philippjfr
Copy link
Member Author

I've decided simply to support a list of hooks instead of the dictionary format as I don't much like the idea of mixing types and predicate functions in the dictionary suggestion, because it makes the ordering ambiguous, while the list of hooks simply iterates in order until it is either exhausted or returns a Parameter or function with dependencies.

param/parameterized.py Outdated Show resolved Hide resolved
@maximlt
Copy link
Member

maximlt commented Jun 23, 2023

I really like pn.bind in Panel, specially that it's similar to functools.partial. I think that makes it really easy for users to leverage functions from other libraries (imagine str.upper being replaced by some model returning an object Panel supports):

image
def bind(function, *args, watch=False, **kwargs):

I haven't been super convinced by using pn.bind with watch=True though:

  • it adds a parameter to the signature of bind which weakens the functools.partial equivalence:
  • it's used for registering callbacks with side-effects only but when I see a stray line like pn.bind(func, ..., watch=True) in an example, possibly quite far from the definition of func and the objects it's being bound to, I find that hard to spot and the intention pretty unclear. Panel users could already register them decorating a function with @pn.depends(..., watch=True) which at least had the advantage of making it clear that this was a side-effect callback (ignoring the general confusions users have with @pn/param.depends(...) and watch=True). Panel has been more and more recommending pn.bind instead of @pn.depends, in the case of side-effects callbacks I question whether this is an improvement. Even if the introduction of the reactive API and soon pn.interactive makes it less likely to have to write such side-effects callbacks, it won't cover all the cases though and so I believe there's a need for a better API.

I think what I'd prefer to have would roughly be something like:

watchers = param.watch?(func, panel_widget, param_obj, bound_func, ...)
# or
watchers = param.bind(func, panel_widget, param_obj, bound_func, ...).watch?()

Returning an object that can be used to unregister the watchers.

@philippjfr
Copy link
Member Author

philippjfr commented Jun 23, 2023

I fully agree, and strongly regret adding watch to bind. The main problem we face though, is that deprecating it is quite difficult and adding a diverging version of bind to param is confusing.

@MarcSkovMadsen
Copy link
Collaborator

MarcSkovMadsen commented Jun 23, 2023

Please also consider supporting param.bind/param.depends to a Parameterized class will bind to its .value parameter if it exists. And maybe even to its .object or .objects parameter if it exists.

This will solve many issues

  • I experience users being very confused by seeing a mix of binding directly to a widget and then to the .param.value of a Parameterized class.
  • users cannot easily make widgets in Panel today because widgets are not simply Parameterized classes with a .value Parameter. They have to inherit from Widget.
  • why cant you use a Pane (or even Layout) as input to pn.bind/ pn.depends in the same way as a widget?

@philippjfr
Copy link
Member Author

This new implementation allows providing hooks that convert arguments to dependencies so you could do something like this:

def transform_pane_obj_dep(obj):
    return obj.param.object if isinstance(obj, PaneBase) else obj

param.parameterized.register_depends_transform(transform_pane_obj_dep)

It definitely isn't param's job to register something like this though, so please file an issue on Panel instead.

@MarcSkovMadsen
Copy link
Collaborator

Hooks are an implementation detail.

I really Think that there should be only one .bind function. Having param.bind and pn.bind with different behaviour is just so confusing to users in practice.

And if .bind could be used as an annotation and .depends could be ditched or vice versa it would again make Panel and HoloViz easier to explain.

@philippjfr
Copy link
Member Author

That's the whole point of this PR, to have one version of depends and bind with the same behavior. I'm just saying that while param defines the ability to define hooks, the actual implementation will have to be in Panel so I'm asking you to open an issue there.

@MarcSkovMadsen
Copy link
Collaborator

MarcSkovMadsen commented Jun 24, 2023

That is not what I'm asking 😉. Im asking to just have param.bind to replace param.depends as well as pn.bind and pn.depends

@philippjfr
Copy link
Member Author

Got you, I was referring to this part of your post:

why cant you use a Pane (or even Layout) as input to pn.bind/ pn.depends in the same way as a widget?

I will see what it would look like to allow binding string parameters to methods like depends.

@philippjfr
Copy link
Member Author

The naming of this seems backward, from the description. If I have an expression X that "observes" some Y, I expect X to respond to Y, whereas here it seems like Y is responding to X. Maybe .rx.observer()?

Don't really get this, .watch has exactly the same semantics and isn't called .watcher.

Hmm. I still have the same concerns with the name "set", coming back to it a month later.

Not sure I like .update much, I'd be okay with something like .set_input I suppose.

I.e., partial should be able to be applied repeatedly, one argument at a time ("currying"), each time returning a "more bound" function until all arguments are fully bound.

Fixed

I think we discussed and must have rejected this possibility during the design sessions, but coming to it fresh I would again suggest supporting rx.call either instead of or as an alias for resolve. It's just a natural counterpart to how we work with reactive bound functions. () for both!

I put a poll up to decide between these and people didn't vote for __call__, I agree with them.

It looks like or behaves appropriately I.e., the result of or is a reactive expression with the appropriate value.

This isn't true, reactive is always truthy, your example simply happened to assume truthiness.

think people are going to be confused about the difference between .bind and .reactive, since both create "reactive" objects. Can't we add .rx to .bind and support call on both so that people don't have to care about the difference? I think I've asked this before and you explained the reasons but I must have forgotten them.

I'm quite wary of adding API that make it appear like two objects are the same when they really aren't. Either param.bind should just go ahead and return a reactive expression OR we should continue making the distinction, superficially papering over the distinction is just going to confuse more people.

@jbednar
Copy link
Member

jbednar commented Sep 18, 2023

The naming of this seems backward, from the description. If I have an expression X that "observes" some Y, I expect X to respond to Y, whereas here it seems like Y is responding to X. Maybe .rx.observer()?

Don't really get this, .watch has exactly the same semantics and isn't called .watcher.

I don't use .watch, but I guess I have the same complaint about that too, then! Sounds like the semantics of "watch" is thus "register_watcher", i.e., the opposite of "watch". A verb method on an object normally means an action invoked on that object, not an action another object is doing on this one, but sounds like it's too late to change in any case.

reactive is always truthy

So that means none of the logical operators work? Does that need to be documented specially?

think people are going to be confused about the difference between .bind and .reactive, since both create "reactive" objects. Can't we add .rx to .bind and support call on both so that people don't have to care about the difference? I think I've asked this before and you explained the reasons but I must have forgotten them.

I'm quite wary of adding API that make it appear like two objects are the same when they really aren't. Either param.bind should just go ahead and return a reactive expression OR we should continue making the distinction, superficially papering over the distinction is just going to confuse more people.

I think they share enough similarities that this is a problem already, which is what I'm concerned about. Is making them be the same problematic?

@philippjfr
Copy link
Member Author

A verb method on an object normally means an action invoked on that object

Don't see how watch and observe don't meet that requirement, the action is to watch or observe changes on the object, the thing you are registering is the watcher. This naming is quite common across libraries that allow registering callbacks. I also really can't see how register_watcher is the opposite of watch, registering the watcher just seems like a technical detail required to watch for changes.

So that means none of the logical operators work? Does that need to be documented specially?

Yes, that seems like a good idea. Reactive doesn't support operator keywords (i.e. and, or, not, in and is), control flow keywords (i.e. if, elif, else), ternary conditional expressions (i.e. a if condition else b), or iteration keywords (i.e. for, while, break, continue, else). That last one isn't quite true because it does implement an iterator interface, but that has the limitation of not allowing variable length unpacking.

I think they share enough similarities that this is a problem already, which is what I'm concerned about. Is making them be the same problematic?

Yes, unfortunately, we can't really resolve a reactive expression on __call__ because that would override the __call__ of the underlying object (if it exists).

@philippjfr
Copy link
Member Author

I guess your point about watch is really about who is the subject of the verb, for watch you might expect that the object is doing the watching. I think the fact that this is a common convention is enough to justify it, but if you want some silly sophistry to justify it then read the signature of a method which is watch(self, ...) which reads fine :)

@philippjfr
Copy link
Member Author

I'd like to merge this so we can start testing it as part of a beta release. There's a few outstanding items that I promise to address:

  • Add a clear explanation of the "Why"
  • Document the keywords and expressions that don't work directly with reactive
  • Cross-link Reactive docs

@philippjfr
Copy link
Member Author

philippjfr commented Sep 22, 2023

Final changes we decided on:

  • Renaming .rx.set -> .rx.set_input
  • Renaming .rx.observe -> .rx.watch

We have also decided that to make the parallels between reactive, bind and Parameter explicit by adding the .rx namespace to all of them. Additionally .rx() can be used to turn bind and Parameter types into a reactive expression (for reactive objects this is a no-op).

@philippjfr
Copy link
Member Author

Thanks for all the review comments. I think everyone's input really helped tighten up the API and come up with a consistent story for bind, reactive and parameters.

@philippjfr philippjfr merged commit b3c6508 into main Sep 23, 2023
10 checks passed
@philippjfr philippjfr deleted the bind branch September 23, 2023 12:13
@jbednar
Copy link
Member

jbednar commented Sep 23, 2023

Yep! I'm really happy with how it has all come together. Great job, everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants