holoviz / param

Param: Make your Python code clearer and more reliable by declaring Parameters
https://param.holoviz.org
BSD 3-Clause "New" or "Revised" License
434 stars 74 forks source link

Fate of `name`? #770

Open maximlt opened 1 year ago

maximlt commented 1 year ago

This was certainly already suggested and discussed multiple times (e.g. https://github.com/holoviz/param/pull/740#issuecomment-1549323783 from @jlstevens) but I couldn't find a specific issue for it. name is a String Parameter included by default in every Parameterized object. At the class level it's set to the class name so P.name == P.__name__ and at the instance level it's set to the class name plus a global Parameterized instances count of 5 digits (e.g. P().name == "P00143").

The existence of name probably always comes as a surprise to Param users, I'm sure it had some very good reasons to exist when Param was created, however it seems to me this is no longer the case.

I'm sure .name is relied upon in many places in the HoloViz source codes (I definitively have code that uses P.name instead of P.__name__), removing it would certainly need to wait for Param 3.0. Until then, let's discuss how much we want to see it go away and if we do how it could be deprecated.

jbednar commented 1 year ago

I think it's more urgent to make .name behave normally than to remove it. Right now it behaves weirdly (https://github.com/holoviz/param/issues/644), but I think we should be able to make it so that if someone overrides it with their own parameter it works as normal. Now that sentinel support is implemented I don't think it should even be difficult. Param itself shouldn't care about name per se, and then anything that builds on it and depends on the current behavior should work, but if people want name to be just a normal Parameter that should also work.

We could also separately support declaring that there shouldn't be a name parameter created at all, configurable at runtime, but that's more likely to have implications. We'd probably want to have .name still supply something in that case even if there is no name parameter that shows up in listings. Could be tricky, but probably better than fixing every instance of .name in all codebases.

maximlt commented 1 year ago

I don't think it should even be difficult

Famous last words! :)

What you're asking for has been merged today after Philipp's review https://github.com/holoviz/param/pull/740, not relying on sentinel support as name is treated way too specially.

jbednar commented 3 weeks ago

The progress so far is good, but I think we can complete deprecating the name attribute of Parameterized without breaking existing code by moving all of Parameterized's functionality into a new superclass, so that users who don't wantname can derive their classes from the superclass instead. It's tricky to make a good generic name for that class, but how about Parametric? Parameterized would then be discouraged for new code but maybe not formally deprecated, since it's been used for decades now and there is no reason for anyone currently happy with how it works to move away from it.

maximlt commented 3 weeks ago

I'm not sure users (and us tbh) will migrate to the new class if the only difference is that it doesn't have the name parameter. I'd reserve such a big change to something bigger like e.g. typing that requires more fundamental changes. I'd hope we would at least try to look for another more conventional way to deprecate name.

Ah and about bikeshedding there's also https://github.com/holoviz/param/issues/701.

jbednar commented 3 weeks ago

My understanding was that Panel maintainers and users wished to not have the name parameter for its objects, and having a superclass would be a clean way for it to do so, with no apparent disruptions to any existing code. Simply removing the name parameter seems disruptive, as I believe various people have put in code checking for it to suppress it at various places they didn't want it to appear, so we'd break all those workflows.

hoxbro commented 3 weeks ago

with no apparent disruptions to any existing code.

Not directly, but if we don't pin to that potential version that supports that, all our code will need to be updated to something like this: class Viewer(getattr(param, "Parametric", param.Parametrized)): and checks need to be done to verify we check the name if we use Parametric.

All documentation, examples, and help blog posts will still use param.Parameterized. Some of them could be updated, but LLM, which we have no control over, cannot. Your suggestion is clean when looking backward but doesn't help developers and maintainers with the future in mind.

maximlt commented 3 weeks ago

Yep 100% agree with Simon. I think we have a tendency to be overly cautious with changes made to Param. Pydantic, who has a much much larger user base, made important changes to their API between V1 and V2 (e.g. https://docs.pydantic.dev/latest/migration/#changes-to-pydanticbasemodel) without providing a new base class. The trick is to give people sufficient time to migrate before actually removing the feature.

as I believe various people have put in code checking for it to suppress it at various places they didn't want it to appear, so we'd break all those workflows.

Usually it's to prevent getting name further in some logic, which would not break if the parameter were to be removed. Example from Param (that has dozens and dozens of occurrences of this pattern):

self._layout_panel.param.update(**{
    p: getattr(self, p) for p in Layoutable.param if p != 'name'
})

And if we were to break someone's code who isn't willing to adapt their logic, the workaround would be as simple as adding name = param.String() to their Parameterized classes!

jbednar commented 3 weeks ago

if we don't pin to that potential version that supports that, all our code will need to be updated to something like this

I'm not sure I understand that objection; it doesn't seem valid to me. Sure, if we didn't pin, we'd have problems, but isn't that why we ever pin? Panel should surely pin any dependent library when it uses API that's only available in a certain version, and adding such a pin is normal and typical and much better than adding checks like that. And because the change would be 100% backwards compatible, pinning to the new version shouldn't cause any issue with any other library in that environment that also uses Param.

All documentation, examples, and help blog posts will still use param.Parameterized. Some of them could be updated, but LLM, which we have no control over, cannot. Your suggestion is clean when looking backward but doesn't help developers and maintainers with the future in mind.

My assumption would be that Panel's own primary docs and examples would be updated, but that older examples that still use Parameterized would never need to be updated because they would (because backwards compatibility is still assured) just keep working. There could be a "meta" problem that even though things do keep working, developers are confused because they wonder what's the difference between Parameterized and Parametric, which is a fair point. So you have to weigh whether that "in the head" confusion is better or worse than "a new version of Param appeared and my code broke". I don't know how many people have code that will break when this change is made, so I don't have the information needed to make that call. I do appreciate that if p != 'name' will be handled gracefully, which is a good argument in favor of simply removing name.

the workaround would be as simple as adding name = param.String() to their Parameterized classes!

I don't think that's true, because there is also code that depends on the auto-generated names having numbers appended to them. But we can simply add a subclass NamedParameterized to hold the full current name-handling behavior, leaving Parameterized as the superclass.

So the actual change to the code is the same in both approaches -- strip out name and move its handling to a subclass. The only difference is whether the name 'Parameterized' is used for the superclass (much less disruption to docs and user experience, but potential to break some user code) or the subclass (requires disruption to Panel docs especially and may cause some user confusion, but no breaking user code).

Sounds like you two are voting for 'Parameterized' being the superclass, and if you truly believe that the user code affected will be rare and not a big deal, I'm ok with that.

maximlt commented 3 weeks ago

So you have to weigh whether that "in the head" confusion is better or worse than "a new version of Param appeared and my code broke". I don't know how many people have code that will break when this change is made, so I don't have the information needed to make that call.

Again I think you're being overly cautious. We would not release a new version of Param immediately without name, it wouldn't be removed finally before a long deprecation period. What other libraries do, like Pandas, is to offer a way to opt-in to the new behavior (no name here) so users can already adapt their code to future versions. Of course, this is all theoretical, I don't know how hard it would be in the code to deprecate name (perhaps replacing it by a special subclass of String that emits warnings when some operations are done), but I observe that in HoloViz-land we really haven't developed that deprecation muscle so we tend to approach it with non-conventional options.

An example of that right in Param is ObjectSelector. It's been somewhat soft-deprecated 3 years ago (https://github.com/holoviz/param/pull/497) but is still heavily used everywhere (Panel uses ObjectSelector internally more than Selector), and users have ended up with two Parameters that behave slightly differently but without a clear way to see how, at least from their name.

Sounds like you two are voting for 'Parameterized' being the superclass, and if you truly believe that the user code affected will be rare and not a big deal, I'm ok with that.

I'm definitely not voting for having a new class. If there's no better way, I would even reconsider removing name, and wait for bigger changes (typing, pydantic, whatever) to be needed to introduce a new class entry point.

hoxbro commented 3 weeks ago

An example of that right in Param is ObjectSelector. It's been somewhat soft-deprecated 3 years ago (https://github.com/holoviz/param/pull/497) but is still heavily used everywhere.

This annoys me. I would like to see a deprecation warning in the next minor release of Param. My opinion is very clear on this: if no warning is emitted, it is not deprecated.

I'm definitely not voting for having a new class.

Same. My ideal solution would be to add a new parameter like param.AutoIncrement(), which autoincrements each time a class is created. I'm not sure if this is currently feasible. A parameter like this would make it possible to restore previous behavior without having two competing classes.

jlstevens commented 3 weeks ago

I have been observing this conversation without getting involved but I like Simon's suggestion so much I will now chime in :-)

Same. My ideal solution would be to add a new parameter like param.AutoIncrement()

I think it should be a string therefore it should probably be called param.AutoName() but if that approach is possible, I would fully support it!

jbednar commented 3 weeks ago

I'm definitely not voting for having a new class.

My understanding is that people wanted the name handling stripped out entirely from Parameterized, leaving absolutely nothing special about name as a Parameter. I was assuming that the way we could do that while also making it clear what anyone affected by this change should do to restore their code to working would be to tell them they now need to use NamedParameterized instead, where all the name logic has been isolated safely away from Parameterized. Are people really objecting to such a class existing? It seems like such a clean and appropriate way to provide people a compatible fallback, with a docstring that makes it clear that most people won't need this special behavior. I'm not quite getting why the existence of this class in parameterized.py would hurt anyone in any meaningful way?

add a new parameter like param.AutoName()

Sounds good! But that won't be an easy change for anyone who was using the name parameter, unless I'm missing something. So even if we do have it isolated into a Parameter type rather than a Parameterized subclass, seems like we'd still need something like:

class NamedParameterized(param.Parameterized):
    name = param.AutoName() # Not sure how it will get the class name to become NamedParameterized00002, but whatever!

So that someone who wants the named behavior can just search and replace "Parameterized" in their code with "NamedParameterized". Maybe I'm missing something?

maximlt commented 3 weeks ago

where all the name logic has been isolated safely away from Parameterized.

We have to see how feasible that is :) ! One risk with two classes is that users migrate more than expected to the named one (cause it's just safer).

But that won't be an easy change for anyone who was using the name parameter

While just replacing Parameterized with NamedParameterized is indeed easier, adding name = param.AutoName() isn't that much more work. I had to update a code base from Pydantic V1 to V2, they were asking for much, much more!

I like the param.AutoName suggesting, working on this would help us identify all the ways name is used in Param (repr, etc.) and how name is created (from a globally incremented value formatted with at least 5 digits). Checking how Panel and HoloViews rely on name would also be useful.

The auto-name isn't the only way name is used, I have code that does Foo(name=<>) without declaring an explicit name parameter, migrating this code would just need to add name = param.String(...).

jbednar commented 3 weeks ago

where all the name logic has been isolated safely away from Parameterized. We have to see how feasible that is

Fair; there's some hairy logic in there that people put in over the years to cover up its weird ways, and the result became even weirder.

It would be good to make a prototype of param.AutoName to know if it's even possible. I can't quite imagine how it would get the self.__class__.__name__ part of the name, as that comes from the class, which doesn't exist when the parameter is being instantiated.

philippjfr commented 2 weeks ago

Sorry for not chiming in earlier. Thanks for the really in-depth discussion here, which has swayed my thinking a good bit.

need to use NamedParameterized instead, where all the name logic has been isolated safely away from Parameterized. Are people really objecting to such a class existing?

Yes, strong -1 for me. Composition should be preferred over subclassing and the AutoName suggestion is nicely compositional, while still straightforward to add. I very much like the fact that Param provides one singular base-class and would not like that to change. Agree that prototyping AutoName should be the first thing we do, then replace the internal hackery on Parameterized with AutoName and then eventually start issuing deprecation warnings when name is used (unless the user explicitly defines it themselves).