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

PwCalculation: refactor parent_folder validation #927

Merged
merged 1 commit into from
May 5, 2023

Conversation

mbercx
Copy link
Member

@mbercx mbercx commented May 5, 2023

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
Copy link
Member Author

mbercx commented May 5, 2023

Note: Docs failing because of #928

@mbercx
Copy link
Member Author

mbercx commented May 5, 2023

@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.

@mbercx mbercx marked this pull request as ready for review May 5, 2023 17:57
@mbercx mbercx force-pushed the refactor/parent-folder-validation branch from 2a20c3f to 2bab85e Compare May 5, 2023 20:02
@sphuber
Copy link
Contributor

sphuber commented May 5, 2023

@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
Copy link
Member Author

mbercx commented May 5, 2023

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" ^^

@mbercx mbercx force-pushed the refactor/parent-folder-validation branch from 2bab85e to 9e684ba Compare May 5, 2023 21:29
@sphuber sphuber self-requested a review May 5, 2023 21:30
@sphuber
Copy link
Contributor

sphuber commented May 5, 2023

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

Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks. I agree with the approach. It makes sense for the parent workchains to exclude the parent_folder

@mbercx
Copy link
Member Author

mbercx commented May 5, 2023

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 #705, but we'll discuss that when I open the PR for it.

@mbercx mbercx merged commit a389629 into aiidateam:main May 5, 2023
@mbercx mbercx deleted the refactor/parent-folder-validation branch May 5, 2023 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants