aiida-quantumespresso icon indicating copy to clipboard operation
aiida-quantumespresso copied to clipboard

Protocol: Clarify/Improve API for custom protocols and overrides

Open mbercx opened this issue 3 years ago • 4 comments

The current API for the get_builder_from_protocol() method doesn't really allow the user to specify their own protocol, only change inputs using the overrides argument or once the builder has been obtained. Moreover, there is some discussion on how the overrides should work, and if there should be a clear separation between actual inputs of the work chain and so-called meta parameters. Below I try to separate these issues and give a clear description of each.

A "True" protocols vs "override" protocols

Defining your own protocol files is different from the overrides, since overrides are always defined in relation to the protocol. This means that e.g. not specifying a tag will lead to the default value being used, not the value being unset (although we could look into allowing this by setting the value to null, see https://github.com/aiidateam/aiida-quantumespresso/issues/653).

The challenge here is that there are multiple protocol files, i.e. one for each work chain. And of these, only the PwBaseWorkChain is a "true" protocol, i.e. the others are defined in relation to the PwBaseWorkChain one, as they override its inputs. This is for a good reason: The protocol files are much easier to maintain this way, and can be used across packages.

The question becomes if users should be able to define a "pure" protocol, and if so: how they can define this. Do we have one large YAML file where the user can specify the "pure" protocol for the PwBaseWorkChain, as well as the protocol overrides for the higher level ones? What if a certain work chain is missing from this file? One option for the latter would be to simply revert to the default in case the requested protocol is missing for a certain work chain.

B Meta parameters vs actual inputs

The protocol files specify two different kinds of inputs:

  1. Meta parameters: these are not actual input ports of the work chain or any process it wraps. Examples include:
  • pseudo_family
  • conv_thr_per_atom
  • etot_conv_thr_per_atom
  1. Actual inputs of the work chain or its subprocesses, i.e. those defined by their spec.

There has been some confusion on the fact that the overrides are used to specify both meta parameters and actual inputs, see #712. The example here is especially confusing since there is both a pseudo_family meta parameters and (deprecated) pseudo_family input. Still, it would be better to separate how these are overridden. A protocol should still define both, however.

C How to override inputs

In order to override the inputs of the protocol, there are two approaches:

  1. Use the overrides input argument, which is either a dict or pathlib.Path to a YAML file.
  2. After obtaining the builder, override the desired inputs in the builder.

Because some inputs in the protocol are meta parameters (see above), they can only be overridden using [1], since they are based on the input structure and define actual inputs inside the get_builder_from_protocol() method. So these must be accessible via an input argument. The question is if we want to remove the actual inputs from [1], i.e. the user should override them after obtaining the builder. In this case, it needs to be possible to do this with a single dictionary, since now I rely quite heavily on this (and I think so does the internal overrides for the protocol of the higher-level work chains (see above). E.g. it should properly convert the parameters into a Dict node when updating the builder based on simple dict.

mbercx avatar Sep 24 '21 12:09 mbercx

Some quick notes from a discussion with @sphuber on the topic, working backwards through the 3 issues:

C How to override inputs

One important argument to keeping the overrides input argument approach vs adapting the builder afterwards for "actual inputs" is that there currently is no way to do the latter with a simple dictionary, since e.g. for the parameters Dict node you would have to once again instantiate this node and pass it to the builder. Since working with YAML files for defining overrides (also within e.g. Dict nodes) is quite convenient (also for the protocols of higher-level work chains), it is preferable to keep the possibility of overriding "actual inputs" using the overrides input argument of the get_builder_from_protocol() method.

B Meta parameters vs actual inputs

In order to better distinguish "meta parameters" versus "actual inputs", both the protocol YAML files and overrides will have two top-level keys where we clearly separate what I described as "meta parameters" and "actual inputs" above:

  • protocol_inputs - Meta parameters used by the get_builder_from_protocol() method to generate inputs for the work chain.
  • process_inputs - Actual inputs of the work chain that correspond to the actual input spec it defines in the define method.

When it comes to precedence, each get_builder_from_protocol() method should:

  1. Load the protocol from the .yaml files in the repository.
  2. Merge in the overrides YAML
  3. Generate the process inputs based on the protocol_inputs.
  4. Add/override these inputs with those in the process_inputs.

Couple of notes:

  • the process_inputs in the overrides have the final say in all cases.
  • process_inputs overrides from higher-level work chains will take precedence over any inputs generated by protocol_inputs. This is important for e.g. https://github.com/aiidateam/aiida-quantumespresso/issues/685.
  • Since we'll be moving spin_type etc inside this dictionary under protocol_inputs, it becomes very important to document this properly or provide some alternative user-friendly way to provide these inputs. Since from the docstring of the get_builder_from_protocol() method alone all of the possible protocol inputs won't be immediately clear.

A "True" protocols vs "override" protocols

This hasn't been fully hashed out yet, but I'll leave some comments here.

  • Even when the user doesn't provide any overrides, it might be beneficial to have a way of popping process inputs using the override mechanism described above (also see https://github.com/aiidateam/aiida-quantumespresso/issues/653). An example use case here is the PdosWorkChain, more specifically the nscf step. Here, SYSTEM.occupations is set to tetrahedra, but the SYSTEM.smearing and SYSTEM.degauss tags are still set by the protocol of the PwBaseWorkChain. Now, QE simply ignores these tags, but there might be other cases (in other codes) where you need to unset some tags.
  • The problem of defining a protocol for a user remains. One idea I had here was to implement a Protocol class that can be loaded from the protocol string (e.g "moderate"), adapted, and then stored as a single YAML file that contains both protocol and process inputs for all the work chains. Have to play around with this a bit to see how this works though, and if it is truly beneficial.

Final comment: Writing all this down, I'm still wondering if we're not just overcomplicating things. 😅 Maybe having some fresh eyes on this problem would help, so I'll ping @ramirezfranciscof @qiaojunfeng @tsthakur, who have all had some experience with using the protocols.

mbercx avatar Sep 28 '21 10:09 mbercx

Thanks for the write up @mbercx . Gives a good overview of what I think the interface indeed should allow. Regarding your final comment, a fresh set of eyes is always valuable, but if we want an interface to automatically build the inputs for a workflow that is easy to use for users with automated protocols, while still being easy to customize for expert users as well, I don't see how we can get away from having a setup that resembles what we have. Of course the exact implementation can vary, but the concept of having protocol inputs in addition to the process inputs and the fact that both should be easily overridable, seems pretty fundamental to me.

sphuber avatar Sep 28 '21 11:09 sphuber

Just discussed this with Seb earlier today. I think whatever route is chosen should make it easy to specify some basic parameters that many users will need, such as

  • the options to be used with the calculation (the user should not need to know anything about the underlying structure of the workflow to do this)
  • specifying an alternative protocol provided by them (I understand that, currently, it is not possible to override the conv_thr_per_atom parameter)

Compromises can be made in terms of generality - a simple interface for a complex workflow will be somewhat restrictive. Power users can always modify the builder directly if needed.

ltalirz avatar Oct 04 '22 16:10 ltalirz

Power users can always modify the builder directly if needed.

Just came back to this issue and wanted to note that in my setup I rely heavily on overrides specified in YAML files. So adapting the builder afterwards is not desirable because:

  1. Having two locations where I specify overrides is more time consuming and prone to error. Also not very user friendly.
  2. In the infrastructure I have set up with aiida-submission-controller, changing the builder after obtaining it is not possible without hard-coding in some extra lines, and so not practical.

So I think we should definitely strive to be able to specify everything through the overrides, or the protocol directly, if we extend the concept beyond a single string -> parameters mapping.

Another note that came up again, that perhaps I already mentioned above: there has to be more validation on the overrides. Currently a typo in the input key argument passes silently. This is very dangerous and user unfriendly.

mbercx avatar Apr 27 '23 10:04 mbercx