-
-
Notifications
You must be signed in to change notification settings - Fork 74
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
Comments
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 We could also separately support declaring that there shouldn't be a |
Famous last words! :) What you're asking for has been merged today after Philipp's review #740, not relying on sentinel support as |
The progress so far is good, but I think we can complete deprecating the |
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 Ah and about bikeshedding there's also #701. |
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. |
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: All documentation, examples, and help blog posts will still use |
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.
Usually it's to prevent getting 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 |
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.
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
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 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. |
Again I think you're being overly cautious. We would not release a new version of Param immediately without An example of that right in Param is
I'm definitely not voting for having a new class. If there's no better way, I would even reconsider removing |
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.
Same. My ideal solution would be to add a new parameter like |
I have been observing this conversation without getting involved but I like Simon's suggestion so much I will now chime in :-)
I think it should be a string therefore it should probably be called |
My understanding is that people wanted the name handling stripped out entirely from Parameterized, leaving absolutely nothing special about
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:
So that someone who wants the named behavior can just search and replace "Parameterized" in their code with "NamedParameterized". Maybe I'm missing something? |
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).
While just replacing I like the The auto-name isn't the only way |
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 |
Sorry for not chiming in earlier. Thanks for the really in-depth discussion here, which has swayed my thinking a good bit.
Yes, strong -1 for me. Composition should be preferred over subclassing and the |
I've been thinking about this a bit more. Specific behavior of
|
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. |
This all still sounds complex, while I thought the goal was to make it simpler. |
The goal is to make it simpler in the end. Maybe you can precise what you find complex in this proposal? |
Do you mean that when MyClass99999 is reached then it will automatically continue with MyClass100000? |
Yes. import param
*_, last = (param.Parameterized().name for _ in range(100000))
print(last) # Parameterized100001 |
My proposal (which got little support :-) ) was to fully isolate all special |
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,
The endpoint is that
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 Tbh I could personally live with |
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 aString
Parameter included by default in every Parameterized object. At the class level it's set to the class name soP.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 usesP.name
instead ofP.__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.The text was updated successfully, but these errors were encountered: