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

CalcJob: Fix MPI behavior if withmpi option default is True #6218

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Dec 8, 2023

The documentation states that if neither:

  • the Code input (through the withmpi attribute)
  • the CalcJob implementation (through `CodeInfo.withpi')
  • nor the metadata.options.withmpi input

explicitly defines a boolean value, then the implementation falls back on the default of the metadata.options.withmpi input port. However, the code was actually ignoring this and always falling back to False. This is now corrected, with False still as ultimate fallback value in case a default for metadata.options.withmpi is not defined at all.

The documentation states that if neither:

* the `Code` input (through the `withmpi` attribute)
* the `CalcJob` implementation (through `CodeInfo.withpi')
* nor the `metadata.options.withmpi` input

explicitly defines a boolean value, then the implementation falls back
on the _default_ of the `metadata.options.withmpi` input port. However,
the code was actually ignoring this and always falling back to `False`.
This is now corrected, with `False` still as ultimate fallback value in
case a default for `metadata.options.withmpi` is not defined at all.
@sphuber sphuber force-pushed the fix/calcjob-withmpi-option-default branch from 8b7d66b to 2dfabf5 Compare December 8, 2023 21:58
@sphuber sphuber requested a review from unkcpz December 11, 2023 15:42
Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

Thanks! Looks all good to me. @sphuber

@sphuber sphuber merged commit 8473750 into aiidateam:main Dec 12, 2023
17 checks passed
@sphuber sphuber deleted the fix/calcjob-withmpi-option-default branch December 12, 2023 14:00
sphuber added a commit that referenced this pull request Jan 16, 2024
The documentation states that if neither:

* the `Code` input (through the `withmpi` attribute)
* the `CalcJob` implementation (through `CodeInfo.withpi')
* nor the `metadata.options.withmpi` input

explicitly defines a boolean value, then the implementation falls back
on the _default_ of the `metadata.options.withmpi` input port. However,
the code was actually ignoring this and always falling back to `False`.
This is now corrected, with `False` still as ultimate fallback value in
case a default for `metadata.options.withmpi` is not defined at all.

Cherry-pick: 8473750
sphuber added a commit that referenced this pull request Jan 17, 2024
The documentation states that if neither:

* the `Code` input (through the `withmpi` attribute)
* the `CalcJob` implementation (through `CodeInfo.withpi')
* nor the `metadata.options.withmpi` input

explicitly defines a boolean value, then the implementation falls back
on the _default_ of the `metadata.options.withmpi` input port. However,
the code was actually ignoring this and always falling back to `False`.
This is now corrected, with `False` still as ultimate fallback value in
case a default for `metadata.options.withmpi` is not defined at all.

Cherry-pick: 8473750
unkcpz added a commit to aiidalab/aiidalab-docker-stack that referenced this pull request Jan 22, 2024
aiida-core 2.4.3 include the fix aiidateam/aiida-core#6218 for correctly setup the mpi option of code.
unkcpz added a commit to aiidalab/aiidalab-docker-stack that referenced this pull request Jan 23, 2024
aiida-core 2.4.3 include the fix aiidateam/aiida-core#6218 for correctly setup the mpi option of code.
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