search

aiidateam / aiida-quantumespresso

The official AiiDA plugin for Quantum ESPRESSO
https://aiida-quantumespresso.readthedocs.io
Other
53 stars 78 forks source link

`PwCalculation`: refactor `parent_folder` validation #927

Closed mbercx closed 1 year ago

mbercx commented 1 year ago

In 3fea26b we added validation for restarting from a parent_folder to the PwCalculation. Here we ran into the following problem:

  1. We want to check that a parent_folder is present when CONTROL.calculation is nscf or bands
  2. The parent_folder will not be present in the initial inputs of e.g. the PwBandsWorkChain bands name space, since it is only created on runtime.
  3. The validation will hence always fail, raising a warning when the builder is put together using the get_builder_from_protocol() method.

Our solution at the time was to split up the validation in a base and full one, and override the validator. However, a more elegant solution is to check if the parent_folder is in the name space, and only do the restart validation in this case. Wrapping work chains that dynamically assign this input should exclude the parent_folder port when exposing the inputs of the PwBaseWorkChain.

mbercx commented 1 year ago

Note: Docs failing because of https://github.com/aiidateam/aiida-quantumespresso/pull/928

mbercx commented 1 year ago

@sphuber this change is unfortunately breaking backwards compatibility. I'm fine with leaving this PR blocked until v5 if you think it's necessary, but I would be inclined to just merge it and communicate these "small" breaking changes clearly in the change log and perhaps even the AiiDA Slack/mailing list.

sphuber commented 1 year ago

@sphuber this change is unfortunately breaking backwards compatibility. I'm fine with leaving this PR blocked until v5 if you think it's necessary, but I would be inclined to just merge it and communicate these "small" breaking changes clearly in the change log and perhaps even the AiiDA Slack/mailing list.

Do you mean breaking as in wrapping workchains using validate_inputs_base? I think we introduced this not so long ago right? Are there even any workchains out there calling that, except for the ones in here, that you update in the PR anyway?

mbercx commented 1 year ago

Are there even any workchains out there calling that, except for the ones in here, that you update in the PR anyway?

Well, I know of some in my other packages, but these are of course not an issue. Maybe @bastonero or @unkcpz might also have run into this validation. I'm fine with these small and easy to fix backwards-incompatible changes though. I favor dealing with the scorn instead of the red tape, and generally think "it's better to ask forgiveness than permission" ^^

sphuber commented 1 year ago

I'm fine with these small and easy to fix backwards-incompatible changes though. I favor dealing with the scorn instead of the red tape, and generally think "it's better to ask forgiveness than permission" ^^

Think it really depends. If the breaking change is going to affect few users and it can be relatively easily addressed, then it makes it more acceptable. I would be happy to accept this change so I will approve

mbercx commented 1 year ago

If the breaking change is going to affect few users and it can be relatively easily addressed, then it makes it more acceptable.

For sure. Anything more difficult would require a major release. That said, I'm also not shy to push out more major releases if we can bundle together some solid backwards-incompatible changes in each. An example is https://github.com/aiidateam/aiida-quantumespresso/issues/705, but we'll discuss that when I open the PR for it.