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

Fate of name? #770

Open
maximlt opened this issue Jun 21, 2023 · 24 comments
Open

Fate of name? #770

maximlt opened this issue Jun 21, 2023 · 24 comments
Labels
type-bug Bug report
Milestone

Comments

@maximlt
Copy link
Member

maximlt commented Jun 21, 2023

This was certainly already suggested and discussed multiple times (e.g. #740 (comment) 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.

@maximlt maximlt added the type-bug Bug report label Jun 21, 2023
@maximlt maximlt added this to the Wishlist milestone Jun 21, 2023
@jbednar
Copy link
Member

jbednar commented Jun 21, 2023

I think it's more urgent to make .name behave normally than to remove it. Right now it behaves weirdly (#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
Copy link
Member Author

maximlt commented Jun 21, 2023

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 #740, not relying on sentinel support as name is treated way too specially.

@jbednar
Copy link
Member

jbednar commented Nov 6, 2024

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
Copy link
Member Author

maximlt commented Nov 6, 2024

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 #701.

@jbednar
Copy link
Member

jbednar commented Nov 6, 2024

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
Copy link
Member

hoxbro commented Nov 7, 2024

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
Copy link
Member Author

maximlt commented Nov 7, 2024

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
Copy link
Member

jbednar commented Nov 8, 2024

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
Copy link
Member Author

maximlt commented Nov 8, 2024

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 (#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
Copy link
Member

hoxbro commented Nov 8, 2024

An example of that right in Param is ObjectSelector. It's been somewhat soft-deprecated 3 years ago (#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
Copy link
Contributor

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
Copy link
Member

jbednar commented Nov 9, 2024

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
Copy link
Member Author

maximlt commented Nov 9, 2024

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
Copy link
Member

jbednar commented Nov 9, 2024

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
Copy link
Member

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).

@maximlt
Copy link
Member Author

maximlt commented Jan 2, 2025

I've been thinking about this a bit more.

Specific behavior of name

Quick recap on name, it's defined as follows in `Parameterized:

    name = String(default=None, constant=True, doc="""
        String identifier for this object.""")

The metaclass and Parameterized class customize its default value:

  • at the class level: it's the class name (equivalent to <cls>.__name__)
  • at the instance level: it's the class name plus a number globally incremented on any Parameterized instance creation

Users can sort of opt-out of these behaviors. At the class level, setting name before creating any instance will disable the auto-generation, i.e. all the instances will have the value set. At the instance level, an object can be created by setting name=... in the constructor, again disabling the auto-generation. Finally, subclasses of Parameterized can override name with a String parameter (not any other type of Parameter), in which case it behaves like a usual parameter (still showing up in all the places where the regular name parameter shows up though).

Where does name show up in Param?

Everywhere :) !

Obviously, in pretty much all the operations that iterate over the parameters (repr, serialization, .param.values()/.objects()/etc.). Some places we'll need to discuss more assuming name is removed:

  • str representation of a Parameterized instance; what should it be?
>>> str(param.Parameterized())
'<Parameterized Parameterized00002>'
  • instance level logging; what should be the logger name?
>>> param.Parameterized().param.log(param.WARNING, 'test')
WARNING:param.Parameterized00002: test

Deprecation process

I did a bit of prototyping/planning to see how we could deprecate and remove name in a (hopefully) not-too-disruptive manner. In three parts:

  1. Add an AutoName Parameter

@hoxbro started to experiment in #976. I'm not convinced that's the way to go (value generated on get, has to be stored in the private namespace). Instead, I thought that a potentially better approach would be to implement it by adding first a factory attribute to Parameter like suggested in #508, since this is basically what we want, a value generated on instantiation. Ok, it's a bit more tricky than that as we also want a class value generated on class creation, but I think there's also a way to achieve that. AutoName would be a a new parameter with constant=True and a custom factory function. More details in the related issue.

  1. Warn when the user interacts with name

I don't think we can reasonably emit a warning for all the ways a user would interact with name (e.g. obj.param.values()['name']) but we can still warn for the main use cases: constructor (<Obj>(name=...)), get/set value (<obj>.name, <obj>.name = ... and getitem the parameter (<obj>.param['name']). The warnings emitted would point to the migration guide.

Internally, Param calls these operations in many places, we should not display a warning when it does, either not emitting warnings at all or capturing them internally (I haven't decided).

  1. Allow the user to opt-out name

This is for users who don't want to use AutoName or add their own name Parameter to replace the current one. Probably the trickier part as name is a descriptor declared in the base class Parameterized so it's hard to remove, even more so as Param doesn't have a mechanism to remove a Parameter (there's add_parameter() to add one). The advantage of implementing an opt-out solution is that it will help us identify all the places where name is relied upon in Param and force us to deal with them early. The approach I found sort of reasonable:

  • name is no longer declared in Parameterized but is added dynamically to Parameterized in ParameterizedMetaclass.__init__, only when Parameterized is created. It does that by default but this can be disabled with an environment variable (e.g. PARAM_NO_NAME).
  • Users can disable name either globally after import (e.g. param.parameterized.no_name = True) or for a specific class and its subclasses (e.g. class P(param.Parameterized, no_name=True): ...). Since name cannot be removed in this case (it's still in Parameterized since disabling name happens after import), there are ways to deal with it:
    • Parameters._cls_parameters excludes it from the dict it builds, so e.g. 'name' in <obj>.param would return False, and basically all iterating operations would not include it anymore (repr, serialization, etc.).
    • The name descriptor is overridden by a descriptor that blocks any get/set operation.

Little conclusion

Having done this small research, I am more confident now that there's a path to properly deprecate name. However, I think that we'll only be able to decide whether it's worth it or not when 1, 2, and 3 are implemented and tried against Panel at least. This should tell us:

  • How useful/efficient the warnings are; i.e. we run Panel's test suite against Param with the warnings and take care of them, before opting-out name
  • How robust is the opt-out approach in Param

If the whole exercise proves too painful, that'd be a sign name is meant to stay for another 20 years at least!

@MarcSkovMadsen
Copy link
Collaborator

MarcSkovMadsen commented Jan 2, 2025

Please also consider the 5 digits in the autogenerated name. Some applications might create more than 5 digit instances.

@maximlt
Copy link
Member Author

maximlt commented Jan 2, 2025

Please also consider the 5 digits in the autogenerated name. Some applications might create more than 5 digit instances.

You can already create more than 5 digit instances.

@jbednar
Copy link
Member

jbednar commented Jan 3, 2025

This all still sounds complex, while I thought the goal was to make it simpler.

@maximlt
Copy link
Member Author

maximlt commented Jan 4, 2025

The goal is to make it simpler in the end. Maybe you can precise what you find complex in this proposal?

@MarcSkovMadsen
Copy link
Collaborator

Please also consider the 5 digits in the autogenerated name. Some applications might create more than 5 digit instances.

You can already create more than 5 digit instances.

Do you mean that when MyClass99999 is reached then it will automatically continue with MyClass100000?

@maximlt
Copy link
Member Author

maximlt commented Jan 4, 2025

Yes.

import param
*_, last = (param.Parameterized().name for _ in range(100000))
print(last)  # Parameterized100001

@jbednar
Copy link
Member

jbednar commented Jan 6, 2025

Maybe you can precise what you find complex in this proposal?

My proposal (which got little support :-) ) was to fully isolate all special name handling to a subclass, and if someone isn't using that subclass, then they don't need to follow any of the logic involved. I think the point of the AutoName suggestion, which is a good alternative if it works, would be the same -- instantiate AutoName if you want, but otherwise no user would be exposed to the complexities of how name works. As I understood the suggestion in #770 (comment) , it sounds like all users will still have to encounter special rules and special handling for name for the foreseeable future, which doesn't sound like a significant improvement to me. Maybe I'm not appreciating some end point where things become clean?

@maximlt
Copy link
Member Author

maximlt commented Jan 6, 2025

My proposal (which got little support :-) ) was to fully isolate all special name handling to a subclass

I haven't tried to do that (as not very enthusiastic about the additive solution of creating another class) but I'm not sure that's even possible, name is in so many places that you'd have to add a lot of hooks, not helping much to make things cleaner in Param.

Maybe I'm not appreciating some end point where things become clean?

The endpoint is that name is removed entirely from Param, I hope this was clear? And the process is:

  • Deprecate: warnings are emitted when users interact with it (if they haven't explicitly declared it with name = param.String(...))
  • If they still need a name Parameter:
    • They can declare it with name = param.String(...) if they need just a regular String parameter
    • If they need the old behavior, they can declare it with name = param.AutoName()
  • Some libraries offer their users to adopt the future implementation early on (Pandas for instance) with some global setting, I'd like to offer no_name for those users who want to make sure they're future-proof (us in particular)
  • name is removed (when exactly TBD, HEP2 says the minimum period is 6 months)

I read this very nice Panel blog post today (https://kdheepak.com/blog/building-dashboards-using-panel-in-python/) that includes this little example. The author declared name to represent the title of a movie. I think title would have been a more appropriate word. It might be they chose name as they knew it's already there and they didn't want to have both name and title in their model. But because name is special and Panel knows that, by default, its Param pane doesn't render it (show_name=True fixes that).

Image

Tbh I could personally live with name, it's even useful sometimes for debugging purposes, but that's the sort of thing that motivates me to persevere in the direction suggested in this issue. Had name be named differently, e.g. param_name, I don't think I'd have bothered suggesting to remove it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug Bug report
Projects
Status: Todo
Development

No branches or pull requests

6 participants