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

Move CalcJob spec validator to corresponding namespaces #3702

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Dec 24, 2019

Fixes #3449 and fixes #3431

The CalcJob process had a single validator defined on the top level
input namespace, which validated many ports scattered across the
namespace, such as the metadata.options.parser_name as well as the
metadata.options.resources. The validator assumed that all of these
ports would always be present, however, this is not necessarily true.
The expose functionality allows a wrapping process to expose only part
of the namespace but the validator remains the same.

To ammeliorate this, the signature of validators is updated to also
receive a context in the form of a second argument port in addition
to the value passed to the port. This port will be the instance of the
port to which the validator is assigned. This allows the validator
implementation to first check whether a specific port is present before
trying to validate the corresponding value.

Note that the port will represent the port to which the validator is
attached and so it will have no knowledge of any namespace it might be
embedded in, as it shouldn't, because that will break the portability of
the namespace.

The port argument being passed to the validator call was introduced in
plumpy==0.14.5 so we upgrade the minimum requirement here. Since that
version also requires pyyaml~=5.1.2 we also update that explicit
version in aiida-core. Going to pyyaml==5.2 will break until we fix
the serialization and deserialization of process instances that are
currently using the FullLoader that is no longer allowed to serialize
arbitrary python objects as we do.

@sphuber
Copy link
Contributor Author

sphuber commented Dec 24, 2019

This is blocked by this PR on plumpy. Note that I could keep the exact validation behavior by splitting up the CalcJob inputs validator except for the resources. By placing it on the metadata.options.resources namespace itself, I no longer have access to the scheduler class, which is defined through the computer. If we think this validation is crucial I can move it back to validation on the top level.

@sphuber sphuber force-pushed the fix_3449_calcjob_input_validation branch from 76ab711 to 8e2a86c Compare December 24, 2019 10:48
The `CalcJob` process had a single validator defined on the top level
input namespace, which validated many ports scattered across the
namespace, such as the `metadata.options.parser_name` as well as the
`metadata.options.resources`. The validator assumed that all of these
ports would always be present, however, this is not necessarily true.
The expose functionality allows a wrapping process to expose only part
of the namespace but the validator remains the same.

To ammeliorate this, the signature of validators is updated to also
receive a `context` in the form of a second argument `port` in addition
to the value passed to the port. This `port` will be the instance of the
port to which the validator is assigned. This allows the validator
implementation to first check whether a specific port is present before
trying to validate the corresponding value.

Note that the `port` will represent the port to which the validator is
attached and so it will have no knowledge of any namespace it might be
embedded in, as it shouldn't, because that will break the portability of
the namespace.

The `port` argument being passed to the validator call was introduced in
`plumpy==0.14.5` so we upgrade the minimum requirement here. Since that
version also requires `pyyaml~=5.1.2` we also update that explicit
version in `aiida-core`. Going to `pyyaml==5.2` will break until we fix
the serialization and deserialization of process instances that are
currently using the `FullLoader` that is no longer allowed to serialize
arbitrary python objects as we do.
@sphuber sphuber force-pushed the fix_3449_calcjob_input_validation branch from 8e2a86c to 3a3f88f Compare January 22, 2020 10:34
@sphuber sphuber changed the title [BLOCKED] Move CalcJob spec validator to corresponding namespaces Move CalcJob spec validator to corresponding namespaces Jan 22, 2020
Copy link
Contributor

@muhrin muhrin left a comment

Choose a reason for hiding this comment

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

Beautiful...I especially like the square brackets

@sphuber sphuber merged commit 0b21cec into aiidateam:develop Jan 23, 2020
@sphuber sphuber deleted the fix_3449_calcjob_input_validation branch January 23, 2020 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants