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

Consider adding a context to the Port validation methods #126

Closed
sphuber opened this issue Oct 17, 2019 · 5 comments · Fixed by #141
Closed

Consider adding a context to the Port validation methods #126

sphuber opened this issue Oct 17, 2019 · 5 comments · Fixed by #141

Comments

@sphuber
Copy link
Collaborator

sphuber commented Oct 17, 2019

This use case stems from the desire of writing a port validator whose implementation does not only require the input values for the port that it is set for, but also other inputs that are passed elsewhere in the namespace (even higher up the nesting). I am not sure if this is an indication of a badly designed input namespace, but it seems reasonable. Example, imagine the following namespace:

spec.input(namespace.a, validator=validate_a)
spec.input(namespace.b)

the validator validate_a with current implementation will only receive the input passed for namespace.a. But if its validation depends on the value of namespace.b it is impossible. One solution may be, simply moving the validator to the top level namespace:

spec.input.validator = validate_a

However, this may not always be possible. The actual use case stems from aiida-core where the CalcJob currently puts the validation of the metadata.options.resources on the top level namespace, but this validator includes other validation as well. In a sub class I know want to make the resources optional and so need to remove the validation of the resources in the top level validator and want to move it directly to the metadata.options.resources port. This would allow me to override that in the sub class, affecting only the part that I want to target. However, this is currently not possible, since the validation of resources depends on the computer input which is passed either indirectly through the code input or directly in the metadata.computer port. In either case, the validator will only get the values passed for metadata.options.resources directly and it is impossible to access the computer.

@sphuber
Copy link
Collaborator Author

sphuber commented Oct 17, 2019

@muhrin happy to have your input. I think technically it would be feasible but I am not sure if this would start to couple nested namespaces to deeply to the namespace in which they are nested. I know that the click callback signature provide in a comparable problem. Their parameter callbacks have the signature callback(ctx, param, value) where ctx will contain the context of the entire command in which the callback of this particular parameter is called. This for example gives access to the input values passed to other parameters in the same command.

@muhrin
Copy link
Collaborator

muhrin commented Oct 17, 2019

Yup. In general I like the 'context concept'. I think for this kind of validation it's perfectly reasonable for validity to be a function of your siblings/parents/etc, even though we have to acknowledge that this allows for a greater coupling between validators and therefore less reusability. But I think this is a price worth paying. If you're up for it I can put together a test implementation and we can see if It would meet your use case.

@sphuber
Copy link
Collaborator Author

sphuber commented Oct 17, 2019

Thanks but no need, I already have a starting implementation that seems to work. I will create a PR soon so we can discuss/iterate.

@muhrin
Copy link
Collaborator

muhrin commented Oct 17, 2019

Top lad. Looking forward to it.

If possible I would vote for a non-breaking implementation, say keyword argument with default for the context but I'm happy give my thoughts on whatever you bake up.

@sphuber
Copy link
Collaborator Author

sphuber commented Oct 17, 2019

In any case it will be breaking existing validators I am afraid, because now we will be passing a second argument at a minimum, but current validators will only allow one in their signature. Unless we start inspecting the argspec of validators before calling them, emitting a deprecation warning if we detect that they only define a single argument. That might be necessary thinking about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment