`AbstractCode`: Add the `with_mpi` attribute #5922

sphuber · 2023-03-08T19:12:56Z

The with_mpi attribute serves to indicate whether a code instance
needs to be run with the MPI run command in front of the executable.
The attribute is exposed through the with_mpi property getter and
setter. The attribute can be set through the verdi code create command
using the --with-mpi/--no-with-mpi flags.

The attribute is set to None by default. This means that the code
doesn't enforce MPI being used or not whatsoever and it is left up to
the CalcJob plugin and the metadata.options.withmpi input.

The CalcJob.presubmit logic is updated to take the with_mpi setting
of the input codes into account. The default value whether to run with
or without MPI is defined by the metadata.options.withmpi input. This
can then be overridden by either the plugin or the code input, but if
both are explicitly specified, they have to agree or a RuntimeError is
raised.

sphuber · 2023-03-10T13:08:49Z

@ltalirz I have added the docs now and updated the logic to consider the value of the withmpi option.

ltalirz

thanks a lot @sphuber !

the implementation and logic looks good to me, just minor points
(leave it up to you whether you want a follow-up review or not)

aiida/cmdline/commands/cmd_code.py

aiida/engine/processes/calcjobs/calcjob.py

aiida/manage/tests/pytest_fixtures.py

ltalirz · 2023-03-11T10:18:14Z

aiida/orm/nodes/data/code/abstract.py

@@ -59,6 +61,7 @@ def __init__(
        :param append_text: The text that should be appended to the run line in the job script.
        :param prepend_text: The text that should be prepended to the run line in the job script.
        :param use_double_quotes: Whether the command line invocation of this code should be escaped with double quotes.
+        :param with_mpi: Whether the command should be run with the MPI run command in front.


Suggested change

:param with_mpi: Whether the command should be run with the MPI run command in front.

:param with_mpi: Whether the code is known to be a MPI program.

I am fine with changing it to "MPI program" but I think the language should include that it forces something. Your suggested phrasing seems to suggest it is just a label but it is not clear whether it will change behavior. How about "Whether the code should be run as an MPI program."

Sounds good!

ltalirz · 2023-03-11T11:01:22Z

docs/source/topics/calculations/usage.rst

+~~~~~~~~~~~~~~~~~~~~~~
+
+Finally, the MPI setting can be controlled on a per-instance basis, using the ``withmpi`` :ref:`metadata option<topics:calculations:usage:calcjobs:options>`.
+If MPI should be enabled or disabled, explicitly set this option to ``True`` or ``False``, respectively.


Suggested change

If MPI should be enabled or disabled, explicitly set this option to ``True`` or ``False``, respectively.

ltalirz · 2023-03-11T11:03:35Z

aiida/engine/processes/calcjobs/calcjob.py

+            # Here are the three values that will override the base value _if_ they are not ``None``. If any of them are
+            # explicitly defined but are not equivalent, an exception is raised. We use the ``self._raw_inputs`` to
+            # determine the actual value passed for ``metadata.options.withmpi`` and distinghuish it from the default.


Suggested change

# Here are the three values that will override the base value _if_ they are not ``None``. If any of them are

# explicitly defined but are not equivalent, an exception is raised. We use the ``self._raw_inputs`` to

# determine the actual value passed for ``metadata.options.withmpi`` and distinghuish it from the default.

# Any of these three values will determine MPI behavior _if_ they are not ``None``. If any of them are

# explicitly defined but are not equivalent, an exception is raised. We use the ``self._raw_inputs`` to

# determine the actual value passed to ``metadata.options.withmpi`` (to distinguish it from the default value).

ltalirz · 2023-03-11T11:06:56Z

docs/source/topics/calculations/usage.rst

+* The ``CalcJob`` implementation
+* The ``metadata.options.withmpi`` input
+
+MPI is enabled or disabled if any of these values is explicitly set to ``True`` or ``False``, respectively.


Suggested change

MPI is enabled or disabled if any of these values is explicitly set to ``True`` or ``False``, respectively.

tests/engine/processes/calcjobs/test_calc_job.py

ltalirz · 2023-03-11T11:16:14Z

tests/engine/processes/calcjobs/test_calc_job.py

+
+
+@pytest.mark.parametrize(
+    'code_key, with_mpi_code, with_mpi_option, expected', (


just pointing out that you don't seem to be testing the ('...',None,None) case (not sure whether there is a reason for it)

Reason was given the fact that CalcJob defines False as default. But I realized that that can of course change later. So I added the tests.

sphuber · 2023-03-11T16:10:19Z

Thanks for the review @ltalirz I have added a single commit with the addressed changes. I have adopted most of your suggestions but not everything, so maybe you want to have a quick final look, but not necessary if you don't have time

ltalirz

thanks @sphuber , looks good to me!

The `with_mpi` attribute serves to indicate whether a code instance needs to be run with the MPI run command in front of the executable. The attribute is exposed through the `with_mpi` property getter and setter. The attribute can be set through the `verdi code create` command using the `--with-mpi/--no-with-mpi` flags. The attribute is set to `None` by default. This means that the code doesn't enforce MPI being used or not whatsoever and it is left up to the `CalcJob` plugin and the `metadata.options.withmpi` input. The `CalcJob.presubmit` logic is updated to take the `with_mpi` setting of the input codes into account. The default value whether to run with or without MPI is defined by the `metadata.options.withmpi` input. This can then be overridden by either the plugin, code input or through the `metadata.options.withmpi` input, but if multiple are explicitly specified, they have to agree or a `RuntimeError` is raised.

The plugin was setting `CodeInfo.withmpi` based on the inputs from `metadata.options.withmpi`. This is not necessary as the metadata option is already handled by the `CalcJob` base class. The plugin implementation should only set `CodeInfo.withmpi` if it wants to _force_ the code to be run with MPI enabled or disabled. The `CalcJob` base implementation used to not distinguish between the two, but in aiidateam/aiida-core#5922 the handling of MPI was improved. A `Code` can now define whether it should be run with MPI or not and so the `CalcJob` class now has to raise if the MPI setting of the code and the plugin clash. The `withmpi` option, if explicitly set by the user, is also checked. Since the plugin doesn't intend to enforce MPI or not, the setting on the `CodeInfo` is removed.

sphuber requested review from unkcpz, ltalirz and yakutovicha March 8, 2023 19:13

sphuber mentioned this pull request Mar 8, 2023

Add with_mpi option to code setup. #5152

Closed

sphuber force-pushed the feature/3236/code-with-mpi-attribute branch 3 times, most recently from 8d27368 to 6a876cb Compare March 10, 2023 13:08

ltalirz previously approved these changes Mar 11, 2023

View reviewed changes

sphuber dismissed ltalirz’s stale review via 558c512 March 11, 2023 16:09

sphuber force-pushed the feature/3236/code-with-mpi-attribute branch from 6a876cb to 558c512 Compare March 11, 2023 16:09

ltalirz previously approved these changes Mar 12, 2023

View reviewed changes

sphuber dismissed ltalirz’s stale review via 2b73765 March 12, 2023 07:55

sphuber force-pushed the feature/3236/code-with-mpi-attribute branch from 558c512 to 2b73765 Compare March 12, 2023 07:55

sphuber merged commit cdb3eed into aiidateam:main Mar 12, 2023

sphuber deleted the feature/3236/code-with-mpi-attribute branch March 12, 2023 08:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`AbstractCode`: Add the `with_mpi` attribute #5922

`AbstractCode`: Add the `with_mpi` attribute #5922

sphuber commented Mar 8, 2023

sphuber commented Mar 10, 2023

ltalirz left a comment •

edited

Loading

ltalirz Mar 11, 2023

sphuber Mar 11, 2023

ltalirz Mar 12, 2023

ltalirz Mar 11, 2023

ltalirz Mar 11, 2023

ltalirz Mar 11, 2023

ltalirz Mar 11, 2023

sphuber Mar 11, 2023

sphuber commented Mar 11, 2023

ltalirz left a comment

	:param with_mpi: Whether the command should be run with the MPI run command in front.
	:param with_mpi: Whether the code is known to be a MPI program.



		@pytest.mark.parametrize(
		'code_key, with_mpi_code, with_mpi_option, expected', (

AbstractCode: Add the with_mpi attribute #5922

AbstractCode: Add the with_mpi attribute #5922

Conversation

sphuber commented Mar 8, 2023

sphuber commented Mar 10, 2023

ltalirz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sphuber commented Mar 11, 2023

ltalirz left a comment

Choose a reason for hiding this comment

`AbstractCode`: Add the `with_mpi` attribute #5922

`AbstractCode`: Add the `with_mpi` attribute #5922

ltalirz left a comment •

edited

Loading