Add context argument to Port validator method

[sphuber]

Fixes #126

For normal Port instances, the validator signature changes from
validator(value) to validator(value, ctx) where ctx is the
instance of the ValueSpec to which the called validator is assigned.
For PortNamespace validators the ctx argument is the instance of the
port namespace itself.

The ctx is especially useful in validators of namespaces because it
can be used to see what ports are actually present. The expose
functionality, which allows to exclude or include only certain ports can
fundamentally change the ports that are present in a namespace, however,
the validator remains untouced. To make the validator robust to these
changes it should check if the port of interest is still present in the
namespace before checking the corresponding value passed in the values
argument.

[sphuber]

@muhrin @greschd @chrisjsewell this is a first proposal to add a context to port validators. I made it backward compatible with a deprecation warning so people can update their validator specs. I will soon open a corresponding PR on aiida-core to see how it would work in practice.

Questions:

• argument name ctx: too vague? I chose it for now taking inspiration from click validators
• For InputPort and OutputPort the type of ctx is ValueSpec which might be a bit unexpected. Should we keep the type and or even make a thin wrapper class that is the same for all port types?

[greschd]

* For `InputPort` and `OutputPort` the type of `ctx` is `ValueSpec` which might be a bit unexpected. Should we keep the type and or even make a thin wrapper class that is the same for all port types?

Wouldn't it be more consistent with the namespace case to pass the Port instance instead? Or is there a problem with doing that?

[sphuber]

The reason is that ValueSpec is the base class of the InputPort and OutportPort classes and it is the ValueSpec class that implements the validate function and therefore passes self to the validator, which is therefore instance of ValueSpec. Could move the validate method maybe to the Port base class, or get rid of ValueSpec entirely. Or is there some way to return the "parent" class in ValueSpec.validate?

[greschd]

The reason is that ValueSpec is the base class of the InputPort and OutportPort classes..

Wait, isn't the Port the base class for InputPort and OutputPort, and ValueSpec is added to Port through composition?

Could move the validate method maybe to the Port base class, or get rid of ValueSpec entirely.

I don't think I'm familiar enough with this part of the code to have a valid opinion on this.

Or is there some way to return the "parent" class in ValueSpec.validate?

My comment was mostly in reply to the naming -- I wouldn't make things more complicated just because of that.

[sphuber]

Wait, isn't the Port the base class for InputPort and OutputPort, and ValueSpec is added to Port through composition?

No it is simply added as a class member, i.e. Port sets self._value_spec = ValueSpec() in its constructor. It then redefines a bunch of property getter and setters that just call through to self._value_spec. It seems to me the better approach would be to either just make Port a sub-class of ValueSpec or better just get rid of ValueSpec. I do not see it being used elsewhere and it seems an unnecessary layer. Unless @muhrin protests, who created this separation initially, I will go ahead with this simplification.

[chrisjsewell]

In general, it looks good to me and inline with what we discussed in aiidateam/aiida-quantumespresso#418 (comment) and #130 👍
ctx sounds as good as anything, and yeh, it does seem a bit redundant that the Port just reimplements all the ValueSpec methods. I'd note that Port was originally a subclass of ValueSpec

plumpy/plumpy/ports.py

Line 109 in 110c937

class Port(with_metaclass(ABCMeta, ValueSpec)):

But reimplemented by @muhrin in daa63de, so perhaps he can remember why!?

[chrisjsewell]

ctx sounds as good as anything

Actually, if the value can now only be Port or PortNamespace, maybe just call it ports?

[sphuber]

ctx sounds as good as anything

Actually, if the value can now only be Port or PortNamespace, maybe just call it ports?

Fair enough, but why plural? It will always be a single instance of a Port, so would port not be better?

[chrisjsewell]

Fair enough, but why plural? It will always be a single instance of a Port, so would port not be better?

Well wouldn't it be port for a Port and ports for a PortNamespace?

[sphuber]

Fair enough, but why plural? It will always be a single instance of a Port, so would port not be better?

Well wouldn't it be port for a Port and ports for a PortNamespace?

In the case of a PortNamespace it is still a single instance of a port, it just happens to happens to be a namespace. Note that PortNamespace is just a sub class of Port but is a port all the same

[sphuber]

OK, I have swapped the two commits, removing the ValueSpec first and integrating its functionality directly in Port, removing one layer of abstraction. The second commit then adds the port context variable to a validator call. Note that I changed it from ctx to the more specific but hopefully also more intuitive port, since it will always be a port instance. Be it a leaf port or PortNamespace.

@muhrin for me this is good to go. Let me know if you are happy with this. There are two issues in aiida-core that are blocked by this.

[muhrin]

Ok, this one is good to go. I was looking through to see why there was a ValueSpec at all. I think it's because there was a time when a Port didn't expose all the methods that ValueSpec (and InputPort further exposed the unused ones). This is no longer the case so what you've done is a good solution.