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

Deprecate passing positional arguments to **some** Parameters #751

Merged
merged 7 commits into from
May 24, 2023

Conversation

maximlt
Copy link
Member

@maximlt maximlt commented May 4, 2023

Following the discussion in #730.

I opened first #737 where I deprecated passing any argument by position to any Parameter. Our ultimate goal being to allow to make default the only positional argument of all the Parameters.

I felt #737 was going over the top, most Parameters already allowed passing default first by position, there's really no need to annoy users with deprecating that, and I have to admit that this is very practical when developing Param itself:

class P(param.Parameterized):
    s = param.String('foo')  # very happy not to have to write `default=` there!

What we needed to do was:

  • for those Parameters that have default as their first argument, deprecate passing any additional arguments by position
  • for the other ones (ClassSelector has class_ first, Selector has objects first), deprecate passing any argument by position

So effectively to avoid the deprecation warnings users will have to write code like below, which will in practice mostly affect Selector and ClassSelector:

class P(param.Parameterized):
    s = param.String('foo')
    o = param.Selector(default=2, objects=[1, 2, 3])  # default cannot be yet passed by position for Selector
    c = param.ClassSelector(default=1, class_=int)  # idem

The plan suggested for the future:

  1. Param 2.x: change the warning from ParamDeprecationWarning to ParamFutureWarning so that it's visible to all users.
  2. Param 3.0: remove the _deprecate_positional_args decorator, so that an error is raised instead of a warning
  3. Param 3.x: allow default to be passed as the only positional argument to Selector and ClassSelector

I'd say there should be at least 6 months between 1. and 2., and 6 months between 2. and 3.

This PR also allows to pass default by position to Dynamic, while before it could only have been passed by keyword.

@maximlt maximlt requested a review from jlstevens May 4, 2023 00:12
@jlstevens
Copy link
Contributor

Deprecating passing any additional arguments by position certainly doesn't seem like it would be a controversial change (to me at least!) and it makes it easier to make decisions about whether or not we want default as the sole-positional argument or not. Your plan sounds good to me!

@jbednar
Copy link
Member

jbednar commented May 11, 2023

Thanks! I completely agree that we should limit the number of positional arguments to at most one; anything else is too fragile and hard to read.

However, I guess I disagree about precisely what that positional argument should be. I think there should be one positional argument accepted if and only if there is a single meaningful value that would be sufficient to define that Parameter. To me, that's what positional arguments are best used for -- easily providing "the" argument that's needed. For the numeric and string types, the default value is that single, meaningful bit of configuration; param.Number(2.7) is often all you need to do to turn a fixed value 2.7 into a Parameter. Simple, obvious, concise!

Selector types also have one single meaningful bit of required configuration, but it isn't the default value, it's the range. If you specify the range (e.g. as a list), the default value can be implicit (currently the first item in the list). But if you specify the default value, then the range is still needed, and so I do not think it's appropriate to have the default value accepted as the single positional argument of a selector.

For instance, you can currently write c = param.Selector(["red","green","blue"]), and it's fine: c gets a value (red), c has a range defined, and if someone wants to change the default, just reorder the list. Simple, obvious (to me! :-) ), concise!

Whereas if a Selector had the default as a positional argument, the minimum sufficient specification for the parameter would be c = param.Selector("red", objects=["red","green","blue"]), which is duplicative and error prone (what if "red" isn't in the list"?), or colors = ["red","green","blue"]; c = param.Selector(colors[0], objects=colors), which is quite wordy and adds a new attribute, or c = param.Selector(objects=["red","green","blue"]), where the default is still implicit and the user is now required to learn and remember the keyword "objects".

For a ClassSelector I don't think the situation is as clear. A ClassSelector also has only a single meaningful required value (the class_), but there's no way to determine the default value in that case, so my guess is that in most uses of a ClassSelector both the default value and the class (range) need to be specified. So I could go either way for a ClassSelector; either accept the class_ as the only positional argument (on the theory that sometimes that's all that's needed), or don't accept anything as a positional argument (on the theory that nearly always both the default and the class are needed, negating any benefit of using positional arguments). Between these two possibilities I have very little opinon other than to break the tie by favoring whatever it already does, which currently is to accept the class_ as a required positional argument.

So that's what I think should happen: default as the one positional argument in nearly all cases, but the range as the one positional argument in the cases where the range is what's required.

Of course, a counterargument would be that most users won't neccessarily stop to think deeply about what's most meaningful, and that those users would best be served by having one simple, easy-to-remember rule "all Parameters accept up to one positional argument, the default value". In this view, even if the resulting syntax is wordier, duplicative, and error prone for the specific case of Selector types, that's outweighed by having a syntax that is more guessable and less error prone for users as they switch between multiple Parameter types.

If that's your argument, then I can grudgingly go along with it, if (a) you are very, very sure that the majority of our users would indeed make that guess, (b) the benefits outweigh the pain of having to deprecate the current behavior and move to the new behavior over several release cycles, and (c) we can realistically get to the point of realizing those benefits in a finite time. I'm dubious about all three, but especially (b) and (c).

In any case, I'm very happy with the changes to remove all positional arguments other than the first one; my concern here is solely about which particular argument that single one is.

@philippjfr
Copy link
Member

Don't have a ton of time to write up my thoughts today but just want to say that we had a meeting where we basically all agreed (I believe at least @hoxbro, @maximlt, @jlstevens and myself were part of that discussion) and that it is impossible to actually reason about what the correct default argument for a parameter should be. I agree that in theory your proposal is reasonable, in practice though it is simply to error prone and even I find myself guessing frequently (I guess with your heuristic in mind I'd do better), but we can't expect new users to do so. The only real way forward is to disable positional arguments, and then eventually maybe reintroduce them for the default value.

@maximlt
Copy link
Member Author

maximlt commented May 12, 2023

To me, that's what positional arguments are best used for -- easily providing "the" argument that's needed.

I agree that is the right way to make use of positional arguments when developing an API!

As said by Philipp, in practice we know that most Parameters accept default as the first argument, and we know a few of them don't, and because of that we mostly ended up using default as a keyword argument to make sure we're not making any mistake. I think the reason for that is that when you create a Parameterized class you write the Parameters one after the other, in a random order (Number, String, Selector, String, Integer, ...), it's easy at this point to get confused about the first argument the Parameters should take.

I opened two PRs on HoloViews and Panel that were merged and passed default by keyword instead of by position:

You can see from these PRs which don't touch that many lines that default was already passed as a keyword argument in most places. Now this is the view in code sources where the same group of people has been involved, I think it'd be interesting to look into some other places, I'll search Github for repos where Param is used (any suggestions welcome, of code not written by any of us).

The only real way forward is to disable positional arguments, and then eventually maybe reintroduce them for the default value.

@philippjfr what I've done in this PR, that is an alternative to #737, is to make a difference between the Parameters that had already default as their first positional argument and the other Parameters. For the first group, which encompasses the vast majority of the Parameter types, it is still possible to use default as a positional argument. Users would only get a warning if they passed more that default by position (e.g. param.String('string', 'regex')). For the second group (Selector, ClassSelector), passing any argument by position is deprecated, until we set default as the first and only positional argument of these Parameters.

I am suggesting this alternative as I found that updating HoloViews and Panel, many of the changes I made adding default=... didn't feel useful at all, as default being the positional argument feels quite obvious, to me at least.

It's a suggestion for which I have a preference as I believe it'll cause less churn. Now that's just a suggestion!

# No warning would be emitted for that code, which might be how users ended up writing Param code.
class P(Parameterized):
    a = String('foo')
    b = Selector(objects=['a', 'b', 'c'])
    c = ClassSelector(default=foo(), class_=Foo)

a) you are very, very sure that the majority of our users would indeed make that guess

I think that default being the first and only positional argument is the case that requires users to guess the least

b) the benefits outweigh the pain of having to deprecate the current behavior and move to the new behavior over several release cycles

I think the approach taken by this PR to deprecate the current behavior for only a couple of Parameters would cause less pain.

c) we can realistically get to the point of realizing those benefits in a finite time

I am not sure what this means. If it's about Param 3.0 not happening in a reasonable time frame, I'd say we could start discussing that part of this PR.

@jlstevens
Copy link
Contributor

Definitely a tricky topic but my gut feeling is that the value of a parameter (as explicitly set with default) is always the most important thing about any parameter regardless of any other wrinkles in semantics (which are associated with the more complex parameter types).

I see arguments both ways but I do think that having the default as the only positional argument will work well for the majority of usage of simple (literal) parameter types which imho are the most common (String, Number, Boolean etc).

This isn't a strong vote for changing things now, but I do think simple, consistent semantics here is a good API goal in the long term.

@jbednar
Copy link
Member

jbednar commented May 24, 2023

Ok, here's a proposal that I think is compatible with everyone's position above, at least in spirit:

  1. Remove support for all positional parameters except for a possible first positional parameter default.
  2. Only support default for cases where it is either already supported as the first positional argument, or no other positional parameter is already supported and it makes sense to add support for default.

I.e., we'd remove all positional arguments unless the first positional argument was already default, and we would add a positional argument if and only if default makes sense for that Parameter and no other positional arguments are currently supported.

I'm proposing this because then people can safely use the shorthand of assuming the first positional argument is default. It's safe because either it's actually true (with default accepted), or it's not true for a particular Parameter type (e.g. Selector) but they'd get a useful error message saying that no positional arguments are allowed, instead of falsely accepting something other than the default as the positional argument.

What I do not think we should do is to accept a first positional argument of default for any Parameter type that currently accepts some other value in that location, such as Selector. I'd rather accept no positional arguments there than even have some plan to eventually accept default there in a way incompatible with current usage.

I think my proposal is the same as this PR implements, except without having any plan to ever add default as a positional argument to any Parameter that currently accepts something else as first positional argument.

@maximlt
Copy link
Member Author

maximlt commented May 24, 2023

OK that's fine by me, as it's indeed what's been implemented in this PR. The only divergence with your plan and the one I suggested is on whether swapping the current first argument for default for a couple of Parameters should ever happen. Swapping those would anyway not happen before a couple of years (imagine someone bumping Param from now to a future where the first argument has been swapped, yuk), at which point we could have this discussion again, as in the meantime:

  1. Maybe there will be more Parameters with a first argument for which default doesn't make sense
  2. Maybe there will be less of such Parameters, or more likely more of the other kind
  3. Users are going to learn more easily about which arguments are positional only or not, and maybe they'll ask for default to be the first argument everywhere
  4. Users will write Param code and we will be able to observe how they construct Selector and ClassSelector params

On 4) and as a side note, I already write it this way and will keep doing so:

class P(Parameterized):
    s = param.Selector(default=2, objects=[1, 2, 3])
    c = param.ClassSelector(default=A(), class_=(A, B))

Thanks for unblocking this PR, merging!

@maximlt maximlt merged commit fe5f371 into main May 24, 2023
@maximlt maximlt deleted the deprecate_pos_args_parameters branch May 24, 2023 23:09
@jlstevens
Copy link
Contributor

For what it is worth, the behavior Jim describes does make good sense to me. Glad to see this merged!

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.

4 participants