From 593849d5aec7004e74bd6e1885ef16d8026c161c Mon Sep 17 00:00:00 2001 From: Giovanni Pizzi Date: Thu, 10 Jun 2021 18:44:27 +0200 Subject: [PATCH 1/5] Adding a --engine-options option to the CLI This allows to specify a dictionary (as a JSON string) to add more content to each of the engines. Example usage and usecase: `--engine-options='{"account": "mr0"}'` when, in order to run, you are required to specify an account. This to me seems a still general enough approach, and important (without it I cannot run so the CLI is not very useful to me). --- aiida_common_workflows/cli/launch.py | 70 ++++++++++++++++----------- aiida_common_workflows/cli/options.py | 24 +++++++++ 2 files changed, 67 insertions(+), 27 deletions(-) diff --git a/aiida_common_workflows/cli/launch.py b/aiida_common_workflows/cli/launch.py index fad37d6e..80c6f558 100644 --- a/aiida_common_workflows/cli/launch.py +++ b/aiida_common_workflows/cli/launch.py @@ -32,11 +32,12 @@ def cmd_launch(): @options.DAEMON() @options.MAGNETIZATION_PER_SITE() @options.REFERENCE_WORKCHAIN() +@options.ENGINE_OPTIONS() @click.option('--show-engines', is_flag=True, help='Show information on the required calculation engines.') def cmd_relax( # pylint: disable=too-many-branches plugin, structure, codes, protocol, relax_type, electronic_type, spin_type, threshold_forces, threshold_stress, number_machines, number_mpi_procs_per_machine, wallclock_seconds, daemon, magnetization_per_site, - reference_workchain, show_engines + reference_workchain, engine_options, show_engines ): """Relax a crystal structure using the common relax workflow for one of the existing plugin implementations. @@ -90,6 +91,10 @@ def cmd_relax( # pylint: disable=too-many-branches return + if not isinstance(engine_options, dict): + message = f'You must pass a dictionary in JSON format (it is now {type(engine_options)}' + raise click.BadParameter(message, param_hint='engine-options') + engines = {} for index, engine in enumerate(generator.get_engine_types()): @@ -104,15 +109,15 @@ def cmd_relax( # pylint: disable=too-many-branches 'Either provide it with the -X option or make sure such a code is configured in the DB.' ) - engines[engine] = { - 'code': code.full_label, - 'options': { - 'resources': { - 'num_machines': number_machines[index], - }, - 'max_wallclock_seconds': wallclock_seconds[index], - } + all_options = { + 'resources': { + 'num_machines': number_machines[index], + }, + 'max_wallclock_seconds': wallclock_seconds[index], } + all_options.update(engine_options) + + engines[engine] = {'code': code.full_label, 'options': all_options} if number_mpi_procs_per_machine is not None: engines[engine]['options']['resources']['num_mpiprocs_per_machine'] = number_mpi_procs_per_machine[index] @@ -149,10 +154,12 @@ def cmd_relax( # pylint: disable=too-many-branches @options.WALLCLOCK_SECONDS() @options.DAEMON() @options.MAGNETIZATION_PER_SITE() +@options.ENGINE_OPTIONS() @click.option('--show-engines', is_flag=True, help='Show information on the required calculation engines.') def cmd_eos( # pylint: disable=too-many-branches plugin, structure, codes, protocol, relax_type, electronic_type, spin_type, threshold_forces, threshold_stress, - number_machines, number_mpi_procs_per_machine, wallclock_seconds, daemon, magnetization_per_site, show_engines + number_machines, number_mpi_procs_per_machine, wallclock_seconds, daemon, magnetization_per_site, engine_options, + show_engines ): """Compute the equation of state of a crystal structure using the common relax workflow. @@ -209,6 +216,10 @@ def cmd_eos( # pylint: disable=too-many-branches return + if not isinstance(engine_options, dict): + message = f'You must pass a dictionary in JSON format (it is now {type(engine_options)}' + raise click.BadParameter(message, param_hint='engine-options') + engines = {} for index, engine in enumerate(generator.get_engine_types()): @@ -222,15 +233,15 @@ def cmd_eos( # pylint: disable=too-many-branches 'Either provide it with the -X option or make sure such a code is configured in the DB.' ) - engines[engine] = { - 'code': code.full_label, - 'options': { - 'resources': { - 'num_machines': number_machines[index] - }, - 'max_wallclock_seconds': wallclock_seconds[index], - } + all_options = { + 'resources': { + 'num_machines': number_machines[index], + }, + 'max_wallclock_seconds': wallclock_seconds[index], } + all_options.update(engine_options) + + engines[engine] = {'code': code.full_label, 'options': all_options} if number_mpi_procs_per_machine is not None: engines[engine]['options']['resources']['num_mpiprocs_per_machine'] = number_mpi_procs_per_machine[index] @@ -273,10 +284,11 @@ def cmd_eos( # pylint: disable=too-many-branches @options.WALLCLOCK_SECONDS() @options.DAEMON() @options.MAGNETIZATION_PER_SITE() +@options.ENGINE_OPTIONS() @click.option('--show-engines', is_flag=True, help='Show information on the required calculation engines.') def cmd_dissociation_curve( # pylint: disable=too-many-branches plugin, structure, codes, protocol, electronic_type, spin_type, number_machines, number_mpi_procs_per_machine, - wallclock_seconds, daemon, magnetization_per_site, show_engines + wallclock_seconds, daemon, magnetization_per_site, engine_options, show_engines ): """Compute the dissociation curve of a diatomic molecule using the common relax workflow. @@ -337,6 +349,10 @@ def cmd_dissociation_curve( # pylint: disable=too-many-branches return + if not isinstance(engine_options, dict): + message = f'You must pass a dictionary in JSON format (it is now {type(engine_options)}' + raise click.BadParameter(message, param_hint='engine-options') + engines = {} for index, engine in enumerate(generator.get_engine_types()): @@ -351,15 +367,15 @@ def cmd_dissociation_curve( # pylint: disable=too-many-branches 'Either provide it with the -X option or make sure such a code is configured in the DB.' ) - engines[engine] = { - 'code': code.full_label, - 'options': { - 'resources': { - 'num_machines': number_machines[index] - }, - 'max_wallclock_seconds': wallclock_seconds[index], - } + all_options = { + 'resources': { + 'num_machines': number_machines[index], + }, + 'max_wallclock_seconds': wallclock_seconds[index], } + all_options.update(engine_options) + + engines[engine] = {'code': code.full_label, 'options': all_options} if number_mpi_procs_per_machine is not None: engines[engine]['options']['resources']['num_mpiprocs_per_machine'] = number_mpi_procs_per_machine[index] diff --git a/aiida_common_workflows/cli/options.py b/aiida_common_workflows/cli/options.py index f8994feb..7e899976 100644 --- a/aiida_common_workflows/cli/options.py +++ b/aiida_common_workflows/cli/options.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- """Module with pre-defined options and defaults for CLI command parameters.""" import pathlib +import json import click @@ -48,6 +49,21 @@ def get_spin_types(): return [entry.value for entry in SpinType] +class JsonParamType(click.ParamType): + """CLI parameter type that can load a JSON string into a python value.""" + name = 'json' + + def convert(self, value, param, ctx): + """Convert from a string to a python object.""" + if not isinstance(value, str): + self.fail(f'{value!r} is not a valid string to be converted to json', param, ctx) + try: + data = json.loads(value) + except ValueError: + self.fail(f'{value!r} is not a valid json', param, ctx) + return data + + class StructureDataParamType(click.Choice): """CLI parameter type that can load `StructureData` from identifier or from a CIF file on disk.""" @@ -241,3 +257,11 @@ def convert(self, value, param, ctx): OUTPUT_FILE = options.OverridableOption( '-o', '--output-file', type=click.STRING, required=False, help='Save the output to a specified file.' ) + +ENGINE_OPTIONS = options.OverridableOption( + '--engine-options', + type=JsonParamType(), + required=False, + default='{}', + help='Define a valid JSON string with a dictionary of options to add to the metadata options of each engine.' +) From 0cd5057eec316ee5a9bdb268d783de8cf3f01361 Mon Sep 17 00:00:00 2001 From: Giovanni Pizzi Date: Fri, 11 Jun 2021 00:15:16 +0200 Subject: [PATCH 2/5] Apply suggestions from code review Co-authored-by: Sebastiaan Huber --- aiida_common_workflows/cli/options.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/aiida_common_workflows/cli/options.py b/aiida_common_workflows/cli/options.py index 7e899976..4e3ea9d8 100644 --- a/aiida_common_workflows/cli/options.py +++ b/aiida_common_workflows/cli/options.py @@ -49,18 +49,18 @@ def get_spin_types(): return [entry.value for entry in SpinType] -class JsonParamType(click.ParamType): - """CLI parameter type that can load a JSON string into a python value.""" +class JsonParamType(click.StringParamType): + """CLI parameter type that can load a JSON string into a Python value.""" + name = 'json' def convert(self, value, param, ctx): """Convert from a string to a python object.""" - if not isinstance(value, str): - self.fail(f'{value!r} is not a valid string to be converted to json', param, ctx) + value = super().convert(value, param, ctx) try: data = json.loads(value) except ValueError: - self.fail(f'{value!r} is not a valid json', param, ctx) + self.fail(f'`{value!r}` is not a valid json', param, ctx) return data From a7a81f787df162047d4815839fef532bf3ce6ccc Mon Sep 17 00:00:00 2001 From: Giovanni Pizzi Date: Fri, 11 Jun 2021 13:18:37 +0200 Subject: [PATCH 3/5] Changing the format of JSON engine options Now it must be a dictionary, one item per engine type. Documentation (helpstring) clarified with one example. --- aiida_common_workflows/cli/launch.py | 37 ++++++++++++++++++--------- aiida_common_workflows/cli/options.py | 9 +++++-- tests/cli/test_options.py | 17 ++++++++++++ 3 files changed, 49 insertions(+), 14 deletions(-) diff --git a/aiida_common_workflows/cli/launch.py b/aiida_common_workflows/cli/launch.py index 80c6f558..0c818e43 100644 --- a/aiida_common_workflows/cli/launch.py +++ b/aiida_common_workflows/cli/launch.py @@ -11,6 +11,25 @@ from . import utils +def validate_engine_options(engine_options, all_engines): + """Validate the custom engine_options. + + It will check the type (dictionary) and if there are unknown engine types. + + :param engine_options: the engine_options returned from the command line option `options.ENGINE_OPTIONS`. + :param all_engines: a list of valid engine names + :raises click.BadParameter: if the options do not validate. + """ + if not isinstance(engine_options, dict): + message = f'You must pass a dictionary in JSON format (it is now {type(engine_options)}' + raise click.BadParameter(message, param_hint='engine-options') + + unknown_engines = set(engine_options).difference(all_engines) + if unknown_engines: + message = f'You are passing unknown engine types: {unknown_engines}' + raise click.BadParameter(message, param_hint='engine-options') + + @cmd_root.group('launch') def cmd_launch(): """Launch a common workflow.""" @@ -91,9 +110,7 @@ def cmd_relax( # pylint: disable=too-many-branches return - if not isinstance(engine_options, dict): - message = f'You must pass a dictionary in JSON format (it is now {type(engine_options)}' - raise click.BadParameter(message, param_hint='engine-options') + validate_engine_options(engine_options, generator.get_engine_types()) engines = {} @@ -115,7 +132,7 @@ def cmd_relax( # pylint: disable=too-many-branches }, 'max_wallclock_seconds': wallclock_seconds[index], } - all_options.update(engine_options) + all_options.update(engine_options.get(engine, {})) engines[engine] = {'code': code.full_label, 'options': all_options} @@ -216,9 +233,7 @@ def cmd_eos( # pylint: disable=too-many-branches return - if not isinstance(engine_options, dict): - message = f'You must pass a dictionary in JSON format (it is now {type(engine_options)}' - raise click.BadParameter(message, param_hint='engine-options') + validate_engine_options(engine_options, generator.get_engine_types()) engines = {} @@ -239,7 +254,7 @@ def cmd_eos( # pylint: disable=too-many-branches }, 'max_wallclock_seconds': wallclock_seconds[index], } - all_options.update(engine_options) + all_options.update(engine_options.get(engine, {})) engines[engine] = {'code': code.full_label, 'options': all_options} @@ -349,9 +364,7 @@ def cmd_dissociation_curve( # pylint: disable=too-many-branches return - if not isinstance(engine_options, dict): - message = f'You must pass a dictionary in JSON format (it is now {type(engine_options)}' - raise click.BadParameter(message, param_hint='engine-options') + validate_engine_options(engine_options, generator.get_engine_types()) engines = {} @@ -373,7 +386,7 @@ def cmd_dissociation_curve( # pylint: disable=too-many-branches }, 'max_wallclock_seconds': wallclock_seconds[index], } - all_options.update(engine_options) + all_options.update(engine_options.get(engine, {})) engines[engine] = {'code': code.full_label, 'options': all_options} diff --git a/aiida_common_workflows/cli/options.py b/aiida_common_workflows/cli/options.py index 4e3ea9d8..87010fb2 100644 --- a/aiida_common_workflows/cli/options.py +++ b/aiida_common_workflows/cli/options.py @@ -49,7 +49,7 @@ def get_spin_types(): return [entry.value for entry in SpinType] -class JsonParamType(click.StringParamType): +class JsonParamType(click.types.StringParamType): """CLI parameter type that can load a JSON string into a Python value.""" name = 'json' @@ -263,5 +263,10 @@ def convert(self, value, param, ctx): type=JsonParamType(), required=False, default='{}', - help='Define a valid JSON string with a dictionary of options to add to the metadata options of each engine.' + help='Define a valid JSON string with a dictionary of options to add to the metadata options of each engine. ' + """This could e.g. be in the format `--engine-options='{"relax": {"account": "ACCOUNT_NAME"}}'` (be careful """ + 'with bash escaping, using single quotes, and using instead double quotes for valid JSON strings! The backticks ' + 'are here used instead only to delimit code). This will add the `account` option to the "relax" engine. If your ' + 'common workflow needs multiple engines, you should pass the options for each engine that you need to modify. ' + ' Suggestion: use the `--show-engines` option to know which engines are required by this common workflow.' ) diff --git a/tests/cli/test_options.py b/tests/cli/test_options.py index c904dea4..33f086c9 100644 --- a/tests/cli/test_options.py +++ b/tests/cli/test_options.py @@ -2,6 +2,7 @@ # pylint: disable=redefined-outer-name,no-self-use """Tests for the :mod:`aiida_common_workflows.cli.launch` module.""" import pathlib +import json import click import pytest @@ -81,6 +82,22 @@ def test_parse_predefined_defaults(self, param_type, label, formula): assert result.get_formula() == formula +class TestJsonParamType: + """Test the ``JsonParamType``.""" + + @pytest.mark.parametrize('data', ({'a': 'b', 'c': True, 'd': 134}, 1, 'a', [1, True, '124aa'])) + def test_valid_json_data(self, param_type, data): + """Test loading from a valid JSON string (both dicts and non-dicts).""" + result = param_type.convert(json.dumps(data), None, None) + assert isinstance(result, dict) + assert result == data + + def test_parsing_fails(self, param_type): + """Test case where parsing of a non-valid JSON string fails.""" + with pytest.raises(click.BadParameter, match=r'.*not a valid json.*'): + param_type.convert('inV alidJSON', None, None) + + def test_reference_workchain(run_cli_command): """Test the ``options.REFERENCE_WORKCHAIN`` option.""" node = orm.WorkflowNode().store() From 926ea60585db68bfd1e90c0e766b8596107acf5d Mon Sep 17 00:00:00 2001 From: Giovanni Pizzi Date: Fri, 11 Jun 2021 13:36:38 +0200 Subject: [PATCH 4/5] Using the right param_type fixture for the new JSON tests --- tests/cli/test_options.py | 42 ++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/tests/cli/test_options.py b/tests/cli/test_options.py index 33f086c9..a5261fe8 100644 --- a/tests/cli/test_options.py +++ b/tests/cli/test_options.py @@ -20,46 +20,52 @@ def filepath_cif(): @pytest.fixture -def param_type(): +def structure_param_type(): """Return instance of ``StructureDataParamType``.""" return options.StructureDataParamType() +@pytest.fixture +def json_param_type(): + """Return instance of ``JsonParamType``.""" + return options.JsonParamType() + + class TestStructureDataParamType: """Test the ``StructureDataParamType``.""" - def test_from_identifier(self, param_type, generate_structure): + def test_from_identifier(self, structure_param_type, generate_structure): """Test loading from node identifier.""" structure = generate_structure().store() for identifier in [structure.pk, structure.uuid]: - result = param_type.convert(str(identifier), None, None) + result = structure_param_type.convert(str(identifier), None, None) assert isinstance(result, orm.StructureData) assert result.uuid == structure.uuid - def test_invalid_filepath(self, param_type): + def test_invalid_filepath(self, structure_param_type): """Test validation of invalid filepath.""" with pytest.raises(click.BadParameter, match=r'failed to load .* and it can also not be resolved as a file.'): - param_type.convert('non-existing.dat', None, None) + structure_param_type.convert('non-existing.dat', None, None) - def test_parsing_fails(self, param_type): + def test_parsing_fails(self, structure_param_type): """Test case where parsing of existing file fails.""" with pytest.raises(click.BadParameter, match=r'file `.*` could not be parsed into a `StructureData`: .*'): - param_type.convert(pathlib.Path(__file__), None, None) + structure_param_type.convert(pathlib.Path(__file__), None, None) - def test_parse_from_file(self, param_type, filepath_cif): + def test_parse_from_file(self, structure_param_type, filepath_cif): """Test successful parsing from file.""" - result = param_type.convert(filepath_cif, None, None) + result = structure_param_type.convert(filepath_cif, None, None) assert isinstance(result, orm.StructureData) assert len(result.sites) == 2 assert result.get_symbols_set() == {'Si'} - def test_parse_from_file_duplicate(self, param_type, filepath_cif): + def test_parse_from_file_duplicate(self, structure_param_type, filepath_cif): """Test successful parsing from file where node already exists in the database.""" - result = param_type.convert(filepath_cif, None, None) + result = structure_param_type.convert(filepath_cif, None, None) structure = result.store() - result = param_type.convert(filepath_cif, None, None) + result = structure_param_type.convert(filepath_cif, None, None) assert result.uuid == structure.uuid @pytest.mark.parametrize( @@ -72,13 +78,13 @@ def test_parse_from_file_duplicate(self, param_type, filepath_cif): ('NH3-planar', 'H3N'), ) ) - def test_parse_predefined_defaults(self, param_type, label, formula): + def test_parse_predefined_defaults(self, structure_param_type, label, formula): """Test the shortcut labels. The parameter type should preferentially treat the value as one of the default structure labels. If found it should load structure from the corresponding CIF file that is shipped with the repo. """ - result = param_type.convert(label, None, None) + result = structure_param_type.convert(label, None, None) assert result.get_formula() == formula @@ -86,16 +92,16 @@ class TestJsonParamType: """Test the ``JsonParamType``.""" @pytest.mark.parametrize('data', ({'a': 'b', 'c': True, 'd': 134}, 1, 'a', [1, True, '124aa'])) - def test_valid_json_data(self, param_type, data): + def test_valid_json_data(self, json_param_type, data): """Test loading from a valid JSON string (both dicts and non-dicts).""" - result = param_type.convert(json.dumps(data), None, None) + result = json_param_type.convert(json.dumps(data), None, None) assert isinstance(result, dict) assert result == data - def test_parsing_fails(self, param_type): + def test_parsing_fails(self, json_param_type): """Test case where parsing of a non-valid JSON string fails.""" with pytest.raises(click.BadParameter, match=r'.*not a valid json.*'): - param_type.convert('inV alidJSON', None, None) + json_param_type.convert('inV alidJSON', None, None) def test_reference_workchain(run_cli_command): From 6a3fd593024e5fbb0761509099faa8039c5a0187 Mon Sep 17 00:00:00 2001 From: Giovanni Pizzi Date: Fri, 11 Jun 2021 13:47:18 +0200 Subject: [PATCH 5/5] Don't test type (check is useless, and wrong) --- tests/cli/test_options.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/cli/test_options.py b/tests/cli/test_options.py index a5261fe8..ff1f69e2 100644 --- a/tests/cli/test_options.py +++ b/tests/cli/test_options.py @@ -95,7 +95,6 @@ class TestJsonParamType: def test_valid_json_data(self, json_param_type, data): """Test loading from a valid JSON string (both dicts and non-dicts).""" result = json_param_type.convert(json.dumps(data), None, None) - assert isinstance(result, dict) assert result == data def test_parsing_fails(self, json_param_type):