Honour the computer's default_mpiprocs_per_machine in HyperQueueJobResource

CHANGELOG.md

@@ -1,3 +1,9 @@
+## Unpublished
+
+### ‼️ Behavior changes
+
+* `HyperQueueJobResource.accepts_default_mpiprocs_per_machine()` now returns `True`: with the deprecated `num_machines`/`num_mpiprocs_per_machine` resource keys, the computer's `default_mpiprocs_per_machine` is now honoured instead of silently falling back to a single CPU. Jobs that relied on that fallback on a computer with a default set will now request `num_machines * default_mpiprocs_per_machine` CPUs. [[#49](https://github.com/aiidateam/aiida-hyperqueue/pull/49)]
+
 ## v0.3.0
 
 ### ⬆️ Update dependencies

aiida_hyperqueue/scheduler.py

@@ -61,25 +61,26 @@ def validate_resources(cls, **kwargs):
         try:
             resources.num_cpus = kwargs.pop("num_cpus")
         except KeyError:
+            # For backward compatibility where `num_machines` and `num_mpiprocs_per_machine`
+            # are set. `num_mpiprocs_per_machine` is normally injected by aiida-core's
+            # `Scheduler.preprocess_resources` from the computer's
+            # `default_mpiprocs_per_machine`, which is `None` when the computer defines no
+            # default — hence the `or`-fallback to 1 rather than a `pop` default.
+            num_mpiprocs_per_machine = kwargs.pop("num_mpiprocs_per_machine", None) or 1
             try:
-                # For backward compatibility where `num_machines` and `num_mpiprocs_per_machine` are setting
-                # TODO: I only setting the default value as 1 for `num_mpiprocs_per_machine` because aiida-quantumespresso override
-                # resources default with `num_machines` set to 1 and then get builder with such setting.
-                # The `num_mpiprocs_per_machine` sometime can be read from "Default #procs/machine" of computer setup but if it is not exist
-                # the builder can not be properly get without passing `option` to builder generator.
-                # It is anyway a workaround for backward compatibility so this default is implemented despite it is quite specific for the qe plugin.
-                resources.num_cpus = kwargs.pop("num_machines") * kwargs.pop(
-                    "num_mpiprocs_per_machine", 1
+                resources.num_cpus = (
+                    kwargs.pop("num_machines") * num_mpiprocs_per_machine
                 )
             except KeyError:
                 raise KeyError(
                     "Must specify `num_cpus`, or (`num_machines` and `num_mpiprocs_per_machine`)"
                 )
             else:
-                message = "The `num_machines` and `num_mpiprocs_per_machine` for setting hyperqueue resources are deprecated. "
-                "Please set `num_cpus` and `memory_mb`."
-
-                message = f"{message} (this will be removed in aiida-hyperqueue v1.0)"
+                message = (
+                    "The `num_machines` and `num_mpiprocs_per_machine` for setting hyperqueue "
+                    "resources are deprecated. Please set `num_cpus` and `memory_mb`. "
+                    "(this will be removed in aiida-hyperqueue v1.0)"
+                )
                 warnings.warn(message, AiiDAHypereQueueDeprecationWarning, stacklevel=3)
         else:
             if not isinstance(resources.num_cpus, int):
@@ -97,8 +98,13 @@ def validate_resources(cls, **kwargs):
 
     @classmethod
     def accepts_default_mpiprocs_per_machine(cls):
-        """Return True if this subclass accepts a `default_mpiprocs_per_machine` key, False otherwise."""
-        return False
+        """Return True if this subclass accepts a `default_mpiprocs_per_machine` key, False otherwise.
+
+        Accepting it lets the backward-compatibility `num_machines` path pick up the
+        computer's ``default_mpiprocs_per_machine`` instead of silently running on a
+        single CPU.
+        """
+        return True
 
     def get_tot_num_mpiprocs(self):
         """Return the total number of cpus of this job resource."""

tests/test_scheduler.py

@@ -4,7 +4,11 @@
 from pathlib import Path
 
 import pytest
-from aiida_hyperqueue.scheduler import HyperQueueJobResource, HyperQueueScheduler
+from aiida_hyperqueue.scheduler import (
+    AiiDAHypereQueueDeprecationWarning,
+    HyperQueueJobResource,
+    HyperQueueScheduler,
+)
 
 from aiida.common.datastructures import CodeRunMode
 from aiida.schedulers import JobState
@@ -65,6 +69,33 @@ def test_resource_validation():
         HyperQueueJobResource(num_cpus=4, memory_mb=1.2)
 
 
+@pytest.mark.parametrize(
+    "resources, default_mpiprocs, expected_num_cpus",
+    (
+        ({"num_machines": 1}, 8, 8),  # the computer's default must be honoured
+        ({"num_machines": 1}, None, 1),  # no default on the computer -> fall back to 1
+        ({"num_machines": 2, "num_mpiprocs_per_machine": 8}, 4, 16),  # explicit wins
+        ({"num_cpus": 4}, 8, 4),  # the modern path ignores the default
+    ),
+)
+@pytest.mark.filterwarnings("ignore:The `num_machines`")
+def test_resource_preprocessing(resources, default_mpiprocs, expected_num_cpus):
+    """The computer's `default_mpiprocs_per_machine` must reach the deprecated `num_machines` path.
+
+    Mirrors `CalcJob`, which calls `preprocess_resources` with the computer's default before the
+    resources are validated.
+    """
+    HyperQueueScheduler.preprocess_resources(resources, default_mpiprocs)
+    resource = HyperQueueScheduler().create_job_resource(**resources)
+    assert resource.num_cpus == expected_num_cpus
+
+
+def test_resource_deprecation():
+    """The `num_machines` / `num_mpiprocs_per_machine` path is deprecated."""
+    with pytest.warns(AiiDAHypereQueueDeprecationWarning, match="deprecated"):
+        HyperQueueJobResource(num_machines=1, num_mpiprocs_per_machine=8)
+
+
 def test_submit_command():
     """Test submit command"""
     scheduler = HyperQueueScheduler()