Honour the computer's default_mpiprocs_per_machine in HyperQueueJobResource

[elinscott]

Problem

HyperQueueJobResource.accepts_default_mpiprocs_per_machine() returns False, so aiida-core's Scheduler.preprocess_resources never injects the computer's default_mpiprocs_per_machine into the resources. The backward-compatibility path then computes num_cpus = num_machines * num_mpiprocs_per_machine with a hard-coded fallback of 1 — so every builder-driven CalcJob (e.g. anything built through aiida-quantumespresso's get_builder_from_protocol, which seeds resources = {'num_machines': 1}) silently runs on a single CPU, regardless of the "Default #procs/machine" configured on the computer.

See the existing TODO comment in validate_resources, which already describes this hole.

Change

accepts_default_mpiprocs_per_machine() now returns True, so aiida-core injects the computer's default as num_mpiprocs_per_machine whenever the user did not set it explicitly. The backward-compatibility product picks it up unchanged.

• When the computer defines no default, aiida-core injects None — the fallback to 1 is kept for that case
• Explicitly passed resources always win.
• The modern num_cpus path is untouched.

Testing

New tests in tests/test_scheduler.py

[elinscott]

@khsrali

[khsrali]

I should be able to have a look at this, soon

[khsrali]

Thanks @elinscott ,
Sorry I had a lot other responsibilities, took some time to get into this.

The changes are sound.
Tests require a fix, see below.
And, the changes are technically backward incompatible, let's put them it in the changelog on this PR, before it gets forgotten. You can add them in a section like:

## Unpublished:
balah blah

Or, if you don't mind, make a new release and put them there.

[khsrali]

Suggested change

	from aiida_hyperqueue.scheduler import (
	AiiDAHypereQueueDeprecationWarning,
	HyperQueueJobResource,
	HyperQueueScheduler,
	)

[elinscott]

Done. (Aside: Hyper-e-QueueDepecationWarning??!)

[khsrali]

From what I understand, one cannot test fix this way. because HyperQueueJobResource bypasses preprocess_resources entirely. The way to go would be using HyperQueueScheduler().create_job_resource which returns the genuine resources considered.

Suggested change

	def test_resource_validation_backward_compatibility():
	"""Tests for the deprecated `num_machines` / `num_mpiprocs_per_machine` path."""
	# num_cpus is the product of the two legacy keys
	with pytest.warns(Warning, match="deprecated"):
	resource = HyperQueueJobResource(num_machines=2, num_mpiprocs_per_machine=8)
	assert resource.num_cpus == 16
	# the computer's default_mpiprocs_per_machine, which aiida-core's
	# `Scheduler.preprocess_resources` injects as `num_mpiprocs_per_machine`,
	# must be honoured (this is what `accepts_default_mpiprocs_per_machine`
	# returning True enables)
	with pytest.warns(Warning, match="deprecated"):
	resource = HyperQueueJobResource(num_machines=1, num_mpiprocs_per_machine=8)
	assert resource.num_cpus == 8
	# a computer without a default leads aiida-core to inject `None`; fall back to 1
	with pytest.warns(Warning, match="deprecated"):
	resource = HyperQueueJobResource(num_machines=1, num_mpiprocs_per_machine=None)
	assert resource.num_cpus == 1
	# `num_mpiprocs_per_machine` omitted entirely also falls back to 1
	with pytest.warns(Warning, match="deprecated"):
	resource = HyperQueueJobResource(num_machines=1)
	assert resource.num_cpus == 1
	def test_accepts_default_mpiprocs_per_machine():
	"""The resource class must accept the computer's default so it reaches validate_resources."""
	assert HyperQueueJobResource.accepts_default_mpiprocs_per_machine()
	@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)

[elinscott]

Adopted. But for the record, the original tests did detect the fix — I ran both versions against the pre-fix base commit (84d8083) and against this branch, and both fail on the base and pass here. But the newer tests are cleaner, thanks!

[khsrali]

ok interesting

[elinscott]

The changes are sound. Tests require a fix, see below. And, the changes are technically backward incompatible, let's put them it in the changelog on this PR, before it gets forgotten.

Done — added an Unpublished section to CHANGELOG.md. N.B. I don't have permissions to create releases; will leave that up to you if you think it's necessary.

[elinscott]

Thanks @khsrali . Should be ready for another look from you

[khsrali]

all good @elinscott
Thanks again!