Coder Social home page Coder Social logo

Behavior without @depends about panel HOT 17 CLOSED

holoviz avatar holoviz commented on May 13, 2024
Behavior without @depends

from panel.

Comments (17)

philippjfr avatar philippjfr commented on May 13, 2024

I would have thought that the default behavior would have been to depend on all parameters already, though?

No, this will never be done because you could use parameterized methods without declaring depends and those shouldn't magically depend on all parameters. That's a recipe for tremendous confusion and backward compatibility issues. What I can imagine is having something like @param.depends(all=True).

However, if I comment out the @param.depends, I get a KeyError: 'dependencies', presumably indicating that there is code that requires @Depends. Is there a clear reason that @Depends is required here?

I'd have to look into this but I suspect that's something that needs to be fixed at the param level.

from panel.

jbednar avatar jbednar commented on May 13, 2024

No, this will never be done because you could use parameterized methods without declaring depends and those shouldn't magically depend on all parameters. That's a recipe for tremendous confusion and backward compatibility issues.

I'm not looking for magic; quite the opposite. I'm looking for logical consistency and having well-founded definitions with appropriate behavior in limiting cases, and I'm claiming that the current approach violates those. My arguments are as follows:

  1. Inherently, in the absence of any information to the contrary, isn't it true that the result of a Parameterized method may depend on any parameter of that object? Regardless of any change we've made to Param in 2018, surely you agree that the body of the method could, at any point, request the value of any parameter, and the return value of the method could thus in principle change, when any of those parameters are changed. Let's call this "principle 1", i.e., that any change to a parameter could affect a method, and in the absence of further information that method thus depends on every parameter.

  2. Because of principle 1, I see the addition of a @depends decorator as inherently limiting what that method depends on, not expanding what it depends on. By default, all methods must logically depend on all parameters, but a programmer may choose to be more explicit and provide additional more specific information that in fact this method only depends on some subset of the parameters. @depends can thus only ever reduce the set of parameters a method depends on, not expand it.

  3. But maybe you also agree with both principles 1 and 2, but still argue that principle 2 causes backwards compatibility problems and thus is worth violating despite it being the logically correct inference to draw from the absence of @depends. If so, then I'd have to hear more examples of precisely what sorts of backwards compatibility problems there would be. I would argue precisely the opposite, in fact: in the absence of @depends, the only safe approach is to assume that the method depends on all parameters. Only by making that assumption can we safely use code written before @depends, because in the absence of any more specific declaration, any change to any parameter could affect the results of the method (principle 1) .

There may be a hole in this reasoning, but if so, I can't see it. And sure, practical considerations could make it appropriate to create a logically ill-founded system solely to achieve backwards compatibility, if there are strong enough reasons. But I can't currently see any backwards compatibility issues with my proposed approach; it seems perfectly backwards compatible already, in that it treats the new decorator as only adding more specific information than we had before, and never contradicting what's already available in every Parameterized class.

from panel.

philippjfr avatar philippjfr commented on May 13, 2024

There are two places where the declared (or undeclared) dependencies will be used:

  1. A DynamicMap uses it to figure out which parameters the ParamStream that is created should watch.

  2. The ParamMethodPanel uses it to figure out when to trigger a full rerender of a panel

In any previous "dashboards" built using parambokeh the whole class would have had to be a Stream instance to connect things up and since streams always depend on all the parameters, anyone using the old approach would already have declared that they depend on all parameters by declaring the stream as part of their DynamicMap. This means that depending on all parameters was a very explicit declaration in the past and the inverse, where you have a method that depends on none of the parameters, would also have required the very explicit choice not to list the class as a stream.

By saying that a DynamicMap should listen to all parameters on a parameterized method by default you are therefore directly counteracting the very deliberate choice of a user not to list the stream and therefore ignore changes in parameters. You are breaking their code even though the old definition was entirely unambiguous about whether to listen to the parameters or not and I absolutely do not think that is a good idea.

The above issues apply entirely to case 1), but in case 2) where you list a parameterized method as a panel listening to all parameters by default seems sensible, e.g. you might do something like this:

explorer = SineExplorer()
Row(explorer, explorer.view)

If you don't want panel to listen for changes on any parameters you already have the choice of doing this:

explorer = SineExplorer()
Row(explorer, explorer.view())

So there is a clear distinction between 1) and 2). That said, I may be convinced that being consistent across 1) and 2) is desirable with one exception, to maintain backward compatibility a DynamicMap would not automatically watch parameters on a Stream class but would watch a simple Parameterized, i.e.:

# Watches all parameters on some_parameterized
hv.DynamicMap(some_parameterized.some_undecorated_method)

# Does not watch any parameters on some_stream
hv.DynamicMap(some_stream.some_undecorated_method)

Even then though, there are valid cases where you might want to use method on a parameterized and not depend on any of the parameters, would you force users to declare an empty @param.depends() in such cases?

from panel.

philippjfr avatar philippjfr commented on May 13, 2024

I've unassigned @ceball since any decision made here only affects holoviews and panel and will (probably) require no changes at the param level.

from panel.

ceball avatar ceball commented on May 13, 2024

edit 1: apologies to github users staticmethod, classmethod, and depends for pinging them!
edit 2: I wrote my comment before Philipp's previous two comments, but didn't submit (so haven't read his comments)

I'll have to spend some time thinking about it. I think I've missed out on some discussion elsewhere related to some of this and haven't caught up yet, so I don't quite know what I'm talking about.

Anyway, my initial thoughts are that I agree with 1, and I almost agree with 2. But...

I decided before to think of the "depends" decorator as signifying that the method is some kind of "special" method that param has some business with - whereas other, regular methods are not something param has anything to do with. Like parameters are special attributes supported by param, while regular python attributes are untouched/not something param has anything to do with. (In that sense, "depends" is probably not the best name.)

I realize that's not quite the right way to think about it, because depends doesn't cause any change - it just declares some things.

I've kind of thought about 2 in the past. Before implementing @depends, I vaguely wondered about having @param_method. Like an instance method gets first argument of the instance and so has access to all state, and @classmethod restricts to class state and the method gets passed the class (and @staticmethod restricts to no state), @param_method might have allowed controlled access to parameters in some way, passing in some special param object to the method to allow that. By default, all parameters could be read, but @param_method(a,b) would have restricted to a and b. And similarly support for saying things about writing. But I dismissed all that kind of thinking :)

@jbednar, for 2, what do think about parameters of subobjects? The story for subobjects isn't really clear in the current version, but in your proposal would methods also depend on all parameters of all subobjects by default?

However, if I comment out the @param.depends, I get a KeyError: 'dependencies', presumably indicating that there is code that requires @depends. Is there a clear reason that @depends is required here?

I'd have to look into this but I suspect that's something that needs to be fixed at the param level.

Yes, right now, if you ask for the dependencies of a method that's not decorated with depends, you'll get an error. That can be changed when we decide what the depends decorator means. Meanwhile, I think it's useful to get an error (given that the current behavior is that if you don't declare dependencies, you won't get any links in your panel).

from panel.

philippjfr avatar philippjfr commented on May 13, 2024

When I unassigned Chris I had forgotten about the KeyError, that is of course something that may need to be addressed when we have decided what to do. My general inclination there is that I don't think it should error but I'm also not entirely convinced that it should report all parameters and subobject parameters by default.

from panel.

ceball avatar ceball commented on May 13, 2024

In general, I prefer to be unassigned than assigned! ;)

@jlstevens isn't watching this repository yet, but I guess he'd also want to think about this issue.

from panel.

jbednar avatar jbednar commented on May 13, 2024

I've unassigned @ceball since any decision made here only affects holoviews and panel and will (probably) require no changes at the param level.

I think there are changes needed to param, i.e. that params_depended_on(parameterized.instance_method) needs to return all parameters for the instance if no more specific declaration has been made. But that's probably already what causes the KeyError, so I think we are in agreement about that.

from panel.

jbednar avatar jbednar commented on May 13, 2024

I'm also not entirely convinced that it should report all parameters and subobject parameters by default.

I don't think it should report subobject parameters (or parameter metadata) as dependencies by default. Even though it's logically true that we could depend on such things, I think it's relatively much rarer and thus I don't consider it practical to expect that we would be traversing everything deeply and building up such a list without an explicit request. I.e., that's an extreme measure (unlike what I see as a simple and obvious listing of all top-level parameters), and thus requires an explicit request from a user. This part is just opinion, though.

from panel.

jbednar avatar jbednar commented on May 13, 2024

Even then though, there are valid cases where you might want to use method on a parameterized and not depend on any of the parameters, would you force users to declare an empty @param.depends() in such cases?

Yes. An empty @depends() would be semantically meaningful, indicating that the method does not depend on any parameters.

from panel.

jbednar avatar jbednar commented on May 13, 2024

I'll have to spend some time thinking about it. I think I've missed out on some discussion elsewhere related to some of this and haven't caught up yet

I think Philipp and Jean-Luc discussed this earlier, but this issue contains all the context that I know about.

from panel.

jbednar avatar jbednar commented on May 13, 2024

This means that depending on all parameters was a very explicit declaration in the past, and the inverse, where you have a method that depends on none of the parameters, would also have required the very explicit choice not to list the class as a stream.

By saying that a DynamicMap should listen to all parameters on a parameterized method by default you are therefore directly counteracting the very deliberate choice of a user not to list the stream and therefore ignore changes in parameters. You are breaking their code even though the old definition was entirely unambiguous about whether to listen to the parameters or not and I absolutely do not think that is a good idea.

This still isn't enough detail for me to see what the implications are. Are there actual existing cases where someone would have (a) supplied a parameterized method to a DynamicMap, (b) not supplied the object as a stream to that DynamicMap, and (c) would be unhappy that the method now gets triggered by a change in the parameter? I can't think of a concrete example of that; anything I can think of would be a case where no one would be messing with that parameter anyway or else that I would want the method triggered by the parameter changing.

I may be convinced that being consistent across 1) and 2) is desirable with one exception, to maintain backward compatibility a DynamicMap would not automatically watch parameters on a Stream class but would watch a simple Parameterized

For the reasons I just mentioned, I can't tell whether such an exception is truly needed, i.e., whether it really makes things behave more consistently and correctly for old code in important cases. But if it is truly needed for old code, I don't think making an exception in that one case hurts anything else, because it seems like it covers a case we don't expect to come up any more. I.e., nowadays, I will no longer be declaring objects I want parameter widgets for to be streams; they can just be simple Parameterized as I always wanted them to be. So if your judgment is that an exception is needed in this case, I don't object.

from panel.

philippjfr avatar philippjfr commented on May 13, 2024

to see what the implications are. Are there actual existing cases where someone would have (a) supplied a parameterized method to a DynamicMap, (b) not supplied the object as a stream to that DynamicMap, and (c) would be unhappy that the method now gets triggered by a change in the parameter?

Just looking through EarthSim's and other client project's code I can see a couple of examples of this and I'm sure other users would have done this too. In most cases the methods depend on some other state on the class that's not a parameter though so it makes good sense and couldn't have been expressed as simple pure functions.

Personally I still feel that an empty @param.depends call to declare no dependencies is weird and magically making everything reactive is more surprising and likely to lead to poorly designed apps than leaving it up to the user to declare it explicitly. If your app doesn't respond at all you're forced to fix it, if it sort of works but very sub-optimally many users are likely to leave it in such a state, i.e. we'd be encouraging bad design. I also feel like by not depending on subobject parameters you are subverting your principle 1), which means it's not really a principle but a fairly arbitrary and in my opinion potentially harmful convention.

I'll defer to whatever you, @jlstevens and @ceball decide at this point but I will definitely insist on the exception I carved out.

from panel.

jlstevens avatar jlstevens commented on May 13, 2024

@ceball is right that I wasn't watching this repo (but now I am).

I am still mulling over this (it is tricky!) but generally I feel that I agree with the principles listed by @jbednar but I also agree with @philippjfr at the pragmatic level.

Personally I still feel that an empty @param.depends call to declare no dependencies is weird...

At least for the DynamicMap case, I believe you should be able to do this by using Callable(method, watch=False). The idea of the watch argument to Callable is to help make these things explicit. Alternatively you can have watch=True (listen to all parameters) or a list of specific parameter names.

If I let myself ignore backwards compatibility I think I agree with @jbednar: if the result is responsive enough listening to all the parameters, then great and the user doesn't need to worry about it! If not, the decorator is a tool to improve performance.

As I said, this is currently my vague feeling about this issue I'll post another comment once I've formulated a more concrete opinion or suggestion.

from panel.

philippjfr avatar philippjfr commented on May 13, 2024

Just to be clear my objection is almost exclusively about the DynamicMap case, in the case of passing parameterized methods to panel (as in Jim's original example) I have no issues with depending on everything.

from panel.

jlstevens avatar jlstevens commented on May 13, 2024

I think it is best if we meet to discuss this after thinking a bit more about it.

from panel.

philippjfr avatar philippjfr commented on May 13, 2024

Since holoviz/param#260 is merged this behavior is now well defined, if a method is not annotated it will now assume that it depends on all parameters.

from panel.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.