Adding a --engine-options option to the CLI

[giovannipizzi]

This allows to specify a dictionary (as a JSON string) to
add more content to each of the engines.

Example usage and usecase:
--engine-options='{"account": "mr0"}'
when, in order to run, you are required to specify an account.

This to me seems a still general enough approach, and important
(without it I cannot run so the CLI is not very useful to me).

[sphuber]

Thanks @giovannipizzi . Indeed useful if not necessary option to have. Just one question about the design and some implementation suggestions. Also, some minimal tests would be good. See tests/cli/test_options.py:TestStructureDataParamType.

[bosonie]

Why don't we force the passed dictionary to be exactly the dictionary accepted by get_builder? For instance for fleur:

{"relax":{"code": ... ,"options": ... },"inpgen":{"code": ... ,"options": ...}}

Just now maybe everything is optional and the info here are overridden if -X -n and so on are used.

[sphuber]

Why don't we force the passed dictionary to be exactly the dictionary accepted by get_builder? For instance for fleur:

{"relax":{"code": ... ,"options": ... },"inpgen":{"code": ... ,"options": ...}}

Just now maybe everything is optional and the info here are overridden if -X -n and so on are used.

Thanks @bosonie that's a lot clearer way of putting what I also suggested 😅 Seems we are both on the same page then at least,

Edit: reading it a bit more closely, there might be a slight difference in what we are suggesting. I think it should match the exact namespace structure, but I think it should only override the inputs[ENGINE']['options'] subdirectory. Anyway, you won't be able to override any inputs that require nodes.

[bosonie]

Anyway, you won't be able to override any inputs that require nodes.

I do not understand this.
Why can't you override the code? We have the option -X to pass the code. An alternative could be to use inputs[ENGINE']['code'] of the passed dictionary. If both are present, the -X is used.

[sphuber]

I do not understand this.

What I mean is that you won't be able to use the --engine-options flag to override any input nodes, even if we wanted to. At least not literally:

aiida-common-workflows ... --engine-options "{'relax': {'metadata': {'options': {...}}, 'parameters': Dict(dict={})}}"

i.e. you cannot have literal python code to define nodes. Of course we could think of allowing identifiers such as the pk or UUID and we load those for the corresponding nodes.

Anyway, the question is do we have this format:

aiida-common-workflows ... --engine-options "{'relax': {'metadata': {'options': {'account': 'mr0'}}}}"

And directly overlay that on the actual inputs, so that is why it has to have the relax key to indicate to which engine type to apply it, or do we simplify and use

aiida-common-workflows ... --engine-options "{'account': 'mr0'}"

and then those options are simply applied to all the engines.

[giovannipizzi]

Thanks for the review. I also thought about this (and admittedly I only thought to my use case ;-) ).

I would go for a compromise:

aiida-common-workflows ... --engine-options='{"relax": {"account": "mr0"}}'

i.e., we now specify one key-value pair per engine, BUT only for the metadata.options part
(and - as a reminder to myself - we can get the keys with e.g. aiida-common-workflows launch eos --show-engines fleur).
It seems a good compromise between complexity and flexibility (remember that it has to be written on the CLI so I would avoid too many nested dicts).

What do you think?

[sphuber]

Thanks for the review. I also thought about this (and admittedly I only thought to my use case ;-) ).

I would go for a compromise:

aiida-common-workflows ... --engine-options='{"relax": {"account": "mr0"}}'

i.e., we now specify one key-value pair per engine, BUT only for the metadata.options part
(and - as a reminder to myself - we can get the keys with e.g. aiida-common-workflows launch eos --show-engines fleur).
It seems a good compromise between complexity and flexibility (remember that it has to be written on the CLI so I would avoid too many nested dicts).

What do you think?

Makes perfect sense. I think the most important thing is to give a concrete example in the help string of the format that is expected. Thinking about it now: since most plugins have just a single engine, for most cases, this requires additional (unnecessary) writing of a dictionary. The best solution for flexibility and usability would be to make it optional to type the explicit engine. Even when there are two, we might just accept directly the options, in which case we apply them to all. The only tricky part is the validation and just by looking at the string determine whether the engine keys are specified or not. Hypothetically there could be some overlap between valid option names and engine names, but I think those chances are minimal. Instead of a generic JsonParamType, we could make an actual OptionParamType that knows about all CalcJob options (get it from the spec) and actually validates the values.

[bosonie]

Ok. Agreed!

[giovannipizzi]

I think that the added complexity of always adding "relax" even if it's only one is acceptable (the logic would start to become complex, and as you say there's risk of overlapping keys.
I've committed my change with a new test.

[giovannipizzi]

I should have fixed my tests. Any idea of why the others are failing? (It's also hard for me to check - even when I did a mistake my tests were running locally - even the failed ones - while a test from aiida_castep was failing. Not sure what's going on...

[sphuber]

@giovannipizzi those failures are due to a bug in the fixtures that surfaced after an update in aiida-pseudo. I have opened a PR to fix it: #207
Once that is merged, we can update this branch and tests should run.

[sphuber]

Thanks @giovannipizzi