PhBaseWorkChain: skip q-points validation if ports are excluded

[mbercx]

Currently, the top-level validate_inputs validator for the PhBaseWorkChain will always check if either the qpoints or qpoints_distance inputs are provided. However, there are cases where a higher-level work chain that wraps the PhBaseWorkChain might provide the q-points input on the fly. Such a work chain would have to set the PhBaseWorkChain validator to None in its own define method.

However, as also discussed for a similar case for the PwCalculation in a389629, it's more elegant to check if the qpoints_distance and qpoints ports are present in the namespace. Typically a work chain that provides the q-points input on the fly will exclude these ports when exposing the PhBaseWorkChain inputs. By checking if the ports are present in the validator, we avoid having to set the validator to None as well in the higher-level work chain.

[bastonero]

Thanks @mbercx , this looks good to me. Only a curiosity/question: can the logic of the namespace be tested?

[mbercx]

Hehe, I was worried you might drop the word "tests". I'm also wondering what a good approach is for testing this change, maybe @sphuber can give a suggestion?

[sphuber]

Hehe, I was worried you might drop the word "tests". I'm also wondering what a good approach is for testing this change, maybe @sphuber can give a suggestion?

Something like this?

from aiida.engine import WorkChain
from aiida_quantumespresso.workflows.ph.base import PhBaseWorkChain

class SubPhBaseWorkChain(WorkChain):

    @classmethod
    def define(cls, spec):
        super().define(spec)
        spec.expose_inputs(PhBaseWorkChain, exclude=('qpoints', 'qpoints_distance'))


def test_validate_inputs_excluded_qpoints_distance(generate_inputs_ph_base):
    from aiida.engine.utils import instantiate_process
    from aiida.manage.manager import get_manager

    inputs = generate_inputs_ph_base
    inputs.pop('qpoints', None)
    inputs.pop('qpoints_distance', None)
    runner = get_manager().get_runner()
    instantiate_process(runner, SubPhBaseWorkChain, **inputs)

[bastonero]

Thanks @mbercx, very happy that a test has been added, and thanks @sphuber for the suggestion. Just a minor question, and it's good to go

[bastonero]

I don't think this is really needed right? Since it is an input of the PhBaseWorkChain

[bastonero]

I mean the qpoints_distance

[mbercx]

Oh fair, it's not in there anyways 👍

[sphuber]

I don't think this is really needed right? Since it is an input of the PhBaseWorkChain

I don't follow. That is the whole point of this test. It is checking that, even though qpoints and qpoints_distance are inputs to PhBaseWorkChain and so they are validated in the inputs namespace validator, when a wrapping workchain explicitly excludes them, the validator does not raise a warning. So the test needs to make sure that inputs are passed that do not contain either qpoints or qpoints_distance. Right?

[bastonero]

Correct, but generate_inputs_ph() would generate inputs of PhCalculation, which doesn't have qpoints_distance, so it's one line more which is not needed. The two qpoints input for PhBaseWorkChain are moved into the "top level" namespace, and not nested in the ph namespace.

[mbercx]

Yeah but I used the generate_inputs_ph fixture which wouldn't have the qpoints_distance (there is no generate_inputs_ph_base afaik).

[bastonero]

Fair enough, I do agree.

[bastonero]

I think one can use generate_ph_workchain_node(return_inputs=True) or similar (don't remember exactly the name of the fixutre)

[sphuber]

Ok, feel free to change as I suggested, but not necessary. Also fine to merge as is for me

[mbercx]

Somehow this absence of introduced scope creep feels like reverse psychology. 🤔

But I'll take it! 🥳

[bastonero]

I think we can approve it this way for the moment being. Thanks @mbercx