Skip to content

Commit

Permalink
Last couple of fixes:
Browse files Browse the repository at this point in the history
- Call `default_memory_per_machine_validator` in `Computer.validate`
- Do not handle `0` in the `ComputerBuilder`
- Correct `run_cli_command` usage.
  • Loading branch information
sphuber authored and yakutovicha committed Dec 15, 2021
1 parent 30a6170 commit a38d025
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 47 deletions.
4 changes: 4 additions & 0 deletions aiida/orm/computers.py
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,7 @@ def validate(self) -> None:
self._transport_type_validator(self.transport_type)
self._scheduler_type_validator(self.scheduler_type)
self._workdir_validator(self.get_workdir())
self.default_memory_per_machine_validator(self.get_default_memory_per_machine())

try:
mpirun_cmd = self.get_mpirun_command()
Expand All @@ -267,6 +268,9 @@ def _default_mpiprocs_per_machine_validator(cls, def_cpus_per_machine: Optional[
@classmethod
def default_memory_per_machine_validator(cls, def_memory_per_machine: Optional[int]) -> None:
"""Validates the default amount of memory (kB) per machine (node)"""
if def_memory_per_machine is None:
return

if not isinstance(def_memory_per_machine, int) or def_memory_per_machine <= 0:
raise exceptions.ValidationError(
f'Invalid value for def_memory_per_machine, must be a positive int, got: {def_memory_per_machine}'
Expand Down
17 changes: 6 additions & 11 deletions aiida/orm/utils/builders/computer.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
# For further information please visit http://www.aiida.net #
###########################################################################
"""Manage computer objects with lazy loading of the db env"""

from aiida.cmdline.utils.decorators import with_dbenv
from aiida.common.exceptions import ValidationError
from aiida.common.utils import ErrorAccumulator


Expand Down Expand Up @@ -101,22 +101,17 @@ def new(self):
computer.set_default_mpiprocs_per_machine(mpiprocs_per_machine)

def_memory_per_machine = self._get_and_count('default_memory_per_machine', used)
if def_memory_per_machine == 0:
def_memory_per_machine = None
# In the command line, 0 means unspecified
if def_memory_per_machine is not None:
try:
def_memory_per_machine = int(def_memory_per_machine)
except ValueError:
raise self.ComputerValidationError(
'Invalid value provided for memory_per_machine, '
'must be a valid integer'
)
if def_memory_per_machine <= 0:
raise self.ComputerValidationError(
'Invalid value provided for default_memory_per_machine, must be positive'
'Invalid value provided for memory_per_machine, must be a valid integer'
)
computer.set_default_memory_per_machine(def_memory_per_machine)
try:
computer.set_default_memory_per_machine(def_memory_per_machine)
except ValidationError as exception:
raise self.ComputerValidationError(f'Invalid value for `default_memory_per_machine`: {exception}')

mpirun_command_internal = self._get_and_count('mpirun_command', used).strip().split(' ')
if mpirun_command_internal == ['']:
Expand Down
55 changes: 19 additions & 36 deletions tests/cmdline/commands/test_computer.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
# For further information on the license, see the LICENSE.txt file #
# For further information please visit http://www.aiida.net #
###########################################################################
# pylint: disable=unused-argument
# pylint: disable=unused-argument,invalid-name
"""Tests for the 'verdi computer' command."""
from collections import OrderedDict
import os
Expand Down Expand Up @@ -89,7 +89,7 @@ def generate_setup_options(ordereddict):
return options


def generate_setup_options_interactive(ordereddict): # pylint: disable=invalid-name
def generate_setup_options_interactive(ordereddict):
"""
Given an (ordered) dict, returns a list of options
Expand Down Expand Up @@ -181,7 +181,6 @@ def test_noninteractive(run_cli_command, aiida_localhost, non_interactive_editor

result = run_cli_command(computer_setup, options)

assert result.exception is None, result.output[-1000:]
new_computer = orm.Computer.objects.get(label=options_dict['label'])
assert isinstance(new_computer, orm.Computer)

Expand All @@ -198,86 +197,75 @@ def test_noninteractive(run_cli_command, aiida_localhost, non_interactive_editor
assert new_computer.get_append_text() == options_dict['append-text']

# Test that I cannot generate twice a computer with the same label
result = run_cli_command(computer_setup, options, catch_exceptions=False)
assert isinstance(result.exception, SystemExit)
result = run_cli_command(computer_setup, options, raises=True)
assert 'already exists' in result.output


@pytest.mark.usefixtures('clear_database_before_test')
def test_noninteractive_optional_default_mpiprocs(run_cli_command): # pylint: disable=invalid-name
def test_noninteractive_optional_default_mpiprocs(run_cli_command):
"""
Check that if is ok not to specify mpiprocs-per-machine
"""
options_dict = generate_setup_options_dict({'label': 'computer_default_mpiprocs'})
options_dict.pop('mpiprocs-per-machine')
options = generate_setup_options(options_dict)
result = run_cli_command(computer_setup, options, catch_exceptions=False)

assert result.exception is None, result.output[-1000:]
run_cli_command(computer_setup, options, catch_exceptions=False)

new_computer = orm.Computer.objects.get(label=options_dict['label'])
assert isinstance(new_computer, orm.Computer)
assert new_computer.get_default_mpiprocs_per_machine() is None


@pytest.mark.usefixtures('clear_database_before_test')
def test_noninteractive_optional_default_mpiprocs_2(run_cli_command): # pylint: disable=invalid-name
def test_noninteractive_optional_default_mpiprocs_2(run_cli_command):
"""
Check that if is the specified value is zero, it means unspecified
"""
options_dict = generate_setup_options_dict({'label': 'computer_default_mpiprocs_2'})
options_dict['mpiprocs-per-machine'] = 0
options = generate_setup_options(options_dict)
result = run_cli_command(computer_setup, options, catch_exceptions=False)

assert result.exception is None, result.output[-1000:]
run_cli_command(computer_setup, options, catch_exceptions=False)

new_computer = orm.Computer.objects.get(label=options_dict['label'])
assert isinstance(new_computer, orm.Computer)
assert new_computer.get_default_mpiprocs_per_machine() is None


@pytest.mark.usefixtures('clear_database_before_test')
def test_noninteractive_optional_default_mpiprocs_3(run_cli_command): # pylint: disable=invalid-name
def test_noninteractive_optional_default_mpiprocs_3(run_cli_command):
"""
Check that it fails for a negative number of mpiprocs
"""
options_dict = generate_setup_options_dict({'label': 'computer_default_mpiprocs_3'})
options_dict['mpiprocs-per-machine'] = -1
options = generate_setup_options(options_dict)
result = run_cli_command(computer_setup, options, catch_exceptions=False)

assert isinstance(result.exception, SystemExit)
result = run_cli_command(computer_setup, options, raises=True)
assert 'mpiprocs_per_machine, must be positive' in result.output


def test_noninteractive_optional_default_memory(run_cli_command): # pylint: disable=invalid-name
def test_noninteractive_optional_default_memory(run_cli_command):
"""
Check that if is ok not to specify default-memory-per-machine
"""
options_dict = generate_setup_options_dict({'label': 'computer_default_mem'})
options_dict.pop('default-memory-per-machine')
options = generate_setup_options(options_dict)
result = run_cli_command(computer_setup, options)

assert result.exception is None, result.output[-1000:]
run_cli_command(computer_setup, options)

new_computer = orm.Computer.objects.get(label=options_dict['label'])
assert isinstance(new_computer, orm.Computer)
assert new_computer.get_default_memory_per_machine() is None


def test_noninteractive_optional_default_mem_2(run_cli_command): # pylint: disable=invalid-name
def test_noninteractive_optional_default_memory_invalid(run_cli_command):
"""
Check that it fails for a negative number of default_memory
Check that it fails for a negative number of default_memory.
"""
options_dict = generate_setup_options_dict({'label': 'computer_default_memory_3'})
options_dict['default-memory-per-machine'] = -1
options = generate_setup_options(options_dict)
result = run_cli_command(computer_setup, options, catch_exceptions=False)

assert isinstance(result.exception, SystemExit)
assert 'default_memory_per_machine, must be positive' in result.output
result = run_cli_command(computer_setup, options, raises=True)
assert 'Invalid value for def_memory_per_machine, must be a positive int, got: -1' in result.output


@pytest.mark.usefixtures('clear_database_before_test')
Expand All @@ -288,8 +276,7 @@ def test_noninteractive_wrong_transport_fail(run_cli_command):
options_dict = generate_setup_options_dict(replace_args={'label': 'fail_computer'})
options_dict['transport'] = 'unknown_transport'
options = generate_setup_options(options_dict)
result = run_cli_command(computer_setup, options, catch_exceptions=False)
assert isinstance(result.exception, SystemExit)
result = run_cli_command(computer_setup, options, raises=True)
assert "entry point 'unknown_transport' is not valid" in result.output


Expand All @@ -301,9 +288,7 @@ def test_noninteractive_wrong_scheduler_fail(run_cli_command):
options_dict = generate_setup_options_dict(replace_args={'label': 'fail_computer'})
options_dict['scheduler'] = 'unknown_scheduler'
options = generate_setup_options(options_dict)
result = run_cli_command(computer_setup, options, catch_exceptions=False)

assert isinstance(result.exception, SystemExit)
result = run_cli_command(computer_setup, options, raises=True)
assert "entry point 'unknown_scheduler' is not valid" in result.output


Expand All @@ -315,9 +300,7 @@ def test_noninteractive_invalid_shebang_fail(run_cli_command):
options_dict = generate_setup_options_dict(replace_args={'label': 'fail_computer'})
options_dict['shebang'] = '/bin/bash' # Missing #! in front
options = generate_setup_options(options_dict)
result = run_cli_command(computer_setup, options, catch_exceptions=False)

assert isinstance(result.exception, SystemExit)
result = run_cli_command(computer_setup, options, raises=True)
assert 'The shebang line should start with' in result.output


Expand Down Expand Up @@ -371,7 +354,7 @@ def setUp(self):
self.comp_builder.prepend_text = ''
self.comp_builder.append_text = ''
self.comp_builder.mpiprocs_per_machine = 8
self.comp_builder.default_memory_per_machine = 0
self.comp_builder.default_memory_per_machine = 100000
self.comp_builder.mpirun_command = 'mpirun'
self.comp_builder.shebang = '#!xonsh'

Expand Down

0 comments on commit a38d025

Please sign in to comment.