Add support for instance parameters

[philippjfr]

This PR implements the notion of an instance parameter which can be accessed using a new API. All parameters can in theory become instance parameters (as long as the new per_instance slot is set to True). The main balance to strike here is that a) we do not want to pay the cost of copying parameters for all instances and b) we do not want users to have to add any special boilerplate to get instance parameters.

The approach I've therefore taken is to only make a copy of a parameter when it is actually needed. This means that all existing param code will never pay the cost of making instance parameters, there's only two situations where the per instance copy is made:

1. When a instance parameter attribute is watched. This is because you do not want events triggered by changes on the class parameter to trigger events on the instance parameter and vice versa.
2. The second situation when a copy is made is when one of the new APIs is used to access the parameter on an instance, e.g. when you access instance.param.param_name or instance.param['param_name']. This API will make it easy to change the attributes of instance parameters, but the old obj.params()/obj.param.params() method is unaffected by this.

Leaving the old API unaffected will ensure that old param code never pays the costs associated with instance parameters.

• Add new parameterized.param.__getitem__ and parameterized.param.__getattr__ accessors for parameters
• Modify setters to ensure they use instance parameters for validation
• Ensure that watching instance parameter attributes makes copies
• Add tests

[philippjfr]

Another thing to decide, a parameter already has an _owner slot which is usually the class but is now replaced when the instance when the parameter is copied. I feel like the owning instance will need to be public for a lot of the functionality we want to build so should we just remove the underscore or rename it completely?

[jbednar]

The approach I've therefore taken is to only make a copy of a parameter when it is actually needed.

That seems like a good approach, but it seems to me that user code could use params() on an instance to get a Parameter object from the class, then one of the cases you outlined above would result in a per-instance copy to be made of that Parameter, and then the saved Parameter object would now be stale. I guess the expectation is that such passing around of Parameter objects outside the class is not to be encouraged or supported anyway?

I guess making the owner be public would let you detect if this has happened...

[philippjfr]

Yes, that is a worry I share, I would indeed not encourage users to store a cache of parameters outside the instance and cannot envision any time I would have done this but then we can't prevent users from doing it either. This is also related to questions about the future API, do we expect to keep obj.param.params()? If we intend to say users should always access their parameters via the getitem/getattr on the Parameters object then we should probably also have a way to disable instance parameters at the class level instead of forcing users to set per_instance in each parameter constructor.

[jbednar]

That all sounds reasonable to me.

[jlstevens]

I guess the expectation is that such passing around of Parameter objects outside the class is not to be encouraged or supported anyway?

Does that means this isn't compatible with #317? I think there are valid uses of Parameters not bound to parameterized classes (and in that PR I use copy.copy to copy parameter instances).

Note, there is a distinction between storing/copying/working with parameter instances that were never bound to a class and caching parameters that were bound to a parameterized class. I don't expect I would ever have need of the latter...

[jlstevens]

Anyway, having stared at this a bit longer, I think the overall approach is sound. The tricky bit is figuring out the possible consequences of this API but I think the way the instance parameters are created and stored is reasonable.

[jbednar]

Does that means this isn't compatible with #317

I don't think so; #317 is about building up Parameterized from Parameters; the restriction here is the other way around (i.e. that pulling a Parameter out from a Parameterized might have surprising behavior later).

[jlstevens]

@jbednar That's what I thought. I just wanted to double check!

[philippjfr]

Okay I just wanted to summarize the outcome of our discussions today. The main starting point of our discussion was that the .params method was highly problematic in this new scheme because we had three bad options 1) it would always return class parameters for backward compatibility which would be confusing when instance parameters exist 2) it would always return instance parameters which would mean all kinds of old code would end up paying the unnecessary costs or 3) it would return the instance parameters at a particular point in time which would be confusing if a user cached them. The other reason we wanted to get rid of .params is because it was weirdly overloaded either returning a whole dict of params or a single parameter and because .param.params looks very redundant.

Therefore we decided we should replace with new API that is less ambiguous. In addition to the newly added __getitem__ and __getattr__ methods we therefore decided to add the following API:

Make foo.param an iterator over parameter names

list(parameterized.param)

This replaces code like this:

list(parameterized.param.params().keys())

Add a __contains__ implementation

param_name in parameterized.param

Replacing code like:

param_name in parameterized.param.params()

New .param.objects

Naming suggestions welcome, this method is roughly equivalent to .params but makes the choice between instance and class parameters explicit, the default is to return instance parameters and this will force copies of those parameters to be made, to force the return of class parameters you can do this:

foo.param.objects(instance=False)

One thing we hadn't discussed is that internal param (and external library) code wants to access the parameters as well and wants to avoid creating instance parameters if it's unnecessary. Therefore I've currently also allowed .param.objects(instance='current') which returns the parameter objects that are currently actually in use. Please suggest a better name for this option if you can think of one.

Warnings if using .params

Eventually we want to deprecate .params, however for the time being it should behave exactly as it did. To make sure this is safe and users don't accidentally use it to get a handle on class parameter instances when there are instance parameters we issue this warning:

The Parameterized instance has instance parameters created using
new-style param APIs, which are incompatible with .params.  Either use
the new APIs on the .param accessor, i.e. either .param.objects to
query all class or instance parameters or use .param[name] to access a
specific parameter object by name.

Disable instance parameters globally

We wanted to make the per_instance behavior to be toggleable globally, what we came up with was finding a way to set the default for Parameter.per_instance to False. I may be wavering on this as we'd have to use some tricks to get this to work with Parameter slots. The other suggestion was to add a special flag just like __abstract on the class, neither one of us was sure about this though.

[philippjfr]

Also I apologize for some of the diffs, I can't work on param without getting annoyed by the inconsistencies in spacing, outdated comments etc. so I ended up updating things that I touched.

[jbednar]

That all sounds like a good plan. I agree that the .params method should be deprecated and then removed in 2.0, but it seems like we can do so in a compatible way that doesn't currently raise any warnings for old code. Basically, I don't think any old code will cause instance parameters to be created, so can we make .params() return class parameters only, but warn loudly if there are any instance parameters defined? That way old code keeps working fine (and quietly) with param 1.x, but when we move to 2.0 we remove .params entirely.

[philippjfr]

Basically, I don't think any old code will cause instance parameters to be created, so can we make .params() return class parameters only, but warn loudly if there are any instance parameters defined?

That's exactly what we decided and what's implemented here.

[jbednar]

That's exactly what we decided and what's implemented here.

Ok, great. I misread your description as warning if there are instance parameters allowed, but it sounds like it's only warning if there are actually any created on this object, which is what I was after. I'm still sorting through the diffs...

[philippjfr]

I ended up renaming the .options(instance='current') to .options(instance='existing'). I also added a class decorator called disable_instance_params (better/shorter name welcome). This PR is now ready to review and merge.

[philippjfr]

Oh, one more thing, we added _owner to Parameters which now holds the owning instance on instance parameters but really we want a public API for this. Should I just add a owner property to make _owner public?

[jbednar]

Should I just add a owner property to make _owner public?

Seems fine, though it seems simpler just to rename _owner to owner.

[philippjfr]

True, it's private API so it should be safe.

[philippjfr]

For convenience we should also make the _attrib_name slot public since that information is important to be able to watch the parameter.

[philippjfr]

Any objection to simply calling that name, which will be complemented by a label?

[jbednar]

Sounds good.

[philippjfr]

This is ready to review and merge from my perspective. I'd like to merge and cut a dev release today.

[philippjfr]

I've also explicitly disallowed reusing Parameter instances on multiple classes, this would already have caused various issues but wasn't explicitly disallowed.

[jlstevens]

How about disable_instance_params to without_instance_params (or no_instance_params)? I don't think you are disabling instance parameters as such: instead you are stopping them from ever being created.

[philippjfr]

no_instance_params

It's already been renamed to that.

[jbednar]

I don't think you are disabling instance parameters as such: instead you are stopping them from ever being created.

That's precisely why I suggested no_instance_params; good to see we are on the same wavelength.

[jlstevens]

Going to be very happy with the functionality offered by this PR. There is a potential pickling issue that may need to be addressed but once that is done and the tests pass, I am happy to see this merged.