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

Overriding the name Parameter is taken into account at the class and instance level #740

Merged
merged 6 commits into from Jun 21, 2023

Conversation

maximlt
Copy link
Member

@maximlt maximlt commented Apr 13, 2023

Fixes #644

Overriding the name String Parameter was possible at the class level but it actually had no effect. This PR changes that so it is taken into account. This is the new behavior:

import param

class P(param.Parameterized):
    name = param.String(default='other')

assert P.name == 'other'

p = P()

assert p.name == 'other'

Overriding is only allowed when name is set to be a String Parameter, as this is something that Param relies on internally for other things. A Parameterized class and its instances will have its name overriden if the default value of the name String Parameter is set to something that is not None or not an empty string.

I used this branch to run the test suites of Panel and HoloViews (both somewhat partially as I don't have all the dependencies installed locally), which helped me to refine it, as there are indeed a few cases of name being overriden by both Panel and HoloViews.

  • I still have a question to resolve. For the need of the implementation I have set a new internal attribute on Parameterized, whose value is set to True when that particular class overrides the value of name. I am not particularly happy with adding new attributes, even if internal, to Parameterized as there is still a chance they will interact with some user defined attribute. However, I played around with some different implementation and this is the only one I found that worked. I would like this variable name to be as "hidden" as possible, I have called it __renamed so that it's mangled, and it happens I can always retrieve it later on the class via the attribute name '_ParameterizedMetaclass__renamed'. I don't find that particularly simple to understand and clean, even if I think it does a pretty good job at making the attribute internal and pretty safe from a user perspective. I'm open to better ideas on naming this internal attribute!

To the reviewers. I suggest to have a close look at the tests that have been added and capture the new behavior. Notably a test I added just before opening this PR on main needed to be updated (test_name_instance_generated_class_name_reset).

@maximlt maximlt requested a review from jlstevens April 13, 2023 19:04
@jbednar
Copy link
Member

jbednar commented May 11, 2023

I'm very much in support of allowing changes to the name parameter, but I wonder if there is a simpler way now that we have Undefined available. Can we make the name lookup bottom out in something that references the auto-generated name, and otherwise allow the user-specified name to be used (as it won't bottom out in that case)? I may not be understanding all the details involved, of course.

@jlstevens
Copy link
Contributor

I've always found the special behavior of the name parameter surprising and I think I like the approach used in this PR. If anything, I would like the default name to be something that is optional (set by a flag somewhere) so that we it could be deprecated, or at the very least disabled by people who do not wish to have this attribute (name is itself a common name in code that people may have other, important uses for).

@maximlt
Copy link
Member Author

maximlt commented May 23, 2023

I'm very much in support of allowing changes to the name parameter, but I wonder if there is a simpler way now that we have Undefined available. Can we make the name lookup bottom out in something that references the auto-generated name, and otherwise allow the user-specified name to be used (as it won't bottom out in that case)? I may not be understanding all the details involved, of course.

I remember that when I implemented this I tried an approach in this vain, and had to give up (which doesn't mean it's not possible!). name isn't quite like any other Parameter on a Parameterized class, as I realized. The class is given a name that is the class name (what you get with the more regular cls.__name__.

mcs.name = name

And every instance of a Parameterized class gets by default a unique name value that is independent of any class hierarchy, while with Undefined what we're doing is respecting the class hierarchy.

@maximlt
Copy link
Member Author

maximlt commented May 23, 2023

If anything, I would like the default name to be something that is optional (set by a flag somewhere) so that we it could be deprecated, or at the very least disabled by people who do not wish to have this attribute (name is itself a common name in code that people may have other, important uses for).

I would also like Parameterized classes not to have a default name. However this is definitely not going to happen in this PR. Let's please open a new issue (no one dared opening this issue already?) and discuss that more, for post Param 2.0.

Copy link
Member

@philippjfr philippjfr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get this merged! The __renamed attribute is a little bit unfortunate but I don't see a better way.

@maximlt maximlt merged commit a8b64bf into main Jun 21, 2023
9 checks passed
@maximlt maximlt deleted the override_name branch June 21, 2023 11:15
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.

Setting default value of 'name' in a Parameterized class doesn't work
4 participants