From 7331bc2a9dca44bb719ce4a4f8bd124d0e4302d3 Mon Sep 17 00:00:00 2001 From: Ali Date: Tue, 20 Aug 2024 14:05:30 +0200 Subject: [PATCH 1/6] _dynamic_info_firecrest_version updated --- aiida_firecrest/transport.py | 29 ++++++++++++++++++++++------- pyproject.toml | 2 +- tests/conftest.py | 22 ++++++++++++++++------ 3 files changed, 39 insertions(+), 14 deletions(-) diff --git a/aiida_firecrest/transport.py b/aiida_firecrest/transport.py index c3e5bec..60f9eef 100644 --- a/aiida_firecrest/transport.py +++ b/aiida_firecrest/transport.py @@ -158,16 +158,31 @@ def _dynamic_info_firecrest_version( small_file_size_mb=0.0, api_version="100.0.0", # version is irrelevant here ) + + parameters = transport._client.parameters() try: - transport.listdir(transport._cwd.joinpath(temp_directory), recursive=True) - _version = "1.16.0" - except Exception: - # all sort of exceptions can be raised here, but we don't care. Since this is just a workaround - _version = "1.15.0" + info = next( + ( + item + for item in parameters["general"] # type: ignore[typeddict-item] + if item["name"] == "FIRECREST_VERSION" + ), + None, + ) + if info is not None: + _version = str(info["value"]) + else: + raise KeyError + except KeyError as err: + raise click.BadParameter( + "Could not get the version of the FirecREST server" + ) from err + + if parse(_version) < parse("1.15.0"): + raise click.BadParameter(f"FirecREST api version {_version} is not supported") click.echo( - click.style("Fireport: ", bold=True, fg="magenta") - + f"Deployed version of FirecREST api: v{_version}" + click.style("Fireport: ", bold=True, fg="magenta") + f"FirecRESTapi: {_version}" ) return _version diff --git a/pyproject.toml b/pyproject.toml index 401a5e4..22d554d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -54,7 +54,7 @@ exclude = [ [tool.ruff] line-length = 120 -extend-select = ["A", "B", "C4", "ICN", "ISC", "N", "RUF", "SIM"] +lint.extend-select = ["A", "B", "C4", "ICN", "ISC", "N", "RUF", "SIM"] # extend-ignore = [] [tool.isort] diff --git a/tests/conftest.py b/tests/conftest.py index df53355..785aaa7 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -324,6 +324,20 @@ def parameters( "value": "Slurm", } ], + "general": [ + { + "description": "FirecREST version.", + "name": "FIRECREST_VERSION", + "unit": "", + "value": "v1.16.1-dev.9", + }, + { + "description": "FirecREST build timestamp.", + "name": "FIRECREST_BUILD", + "unit": "", + "value": "2024-08-16T15:58:47Z", + }, + ], "storage": [ { "description": "Type of object storage, like `swift`, `s3v2` or `s3v4`.", @@ -361,17 +375,13 @@ def parameters( ], "utilities": [ { - "description": "The maximum allowable file size for various operations" - " of the utilities microservice", + "description": "The maximum allowable file size for various operations of the utilities microservice.", "name": "UTILITIES_MAX_FILE_SIZE", "unit": "MB", "value": "5", }, { - "description": ( - "Maximum time duration for executing the commands " - "in the cluster for the utilities microservice." - ), + "description": "Maximum time duration for executing the commands in the cluster for the utilities microservice.", "name": "UTILITIES_TIMEOUT", "unit": "seconds", "value": "5", From ff9b65ef54b9767085b9baa64707003bcd17bdb4 Mon Sep 17 00:00:00 2001 From: Ali Date: Tue, 20 Aug 2024 14:37:27 +0200 Subject: [PATCH 2/6] tests added --- tests/test_computer.py | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/tests/test_computer.py b/tests/test_computer.py index 6a2041a..4b37ea4 100644 --- a/tests/test_computer.py +++ b/tests/test_computer.py @@ -107,9 +107,10 @@ def test_validate_temp_directory( ).exists() -def test_dynamic_info(firecrest_config, monkeypatch, tmpdir: Path): +def test_dynamic_info_direct_size(firecrest_config, monkeypatch, tmpdir: Path): from aiida_firecrest.transport import _dynamic_info_direct_size + assert monkeypatch is not None monkeypatch.setattr("click.echo", lambda x: None) ctx = Mock() ctx.params = { @@ -130,3 +131,28 @@ def test_dynamic_info(firecrest_config, monkeypatch, tmpdir: Path): # note: user cannot enter negative numbers anyways, click raise as this shoule be float not str result = _dynamic_info_direct_size(ctx, None, 10) assert result == 10 + + +def test_dynamic_info_firecrest_version(firecrest_config, monkeypatch, tmpdir: Path): + + from aiida_firecrest.transport import _dynamic_info_firecrest_version + + monkeypatch.setattr("click.echo", lambda x: None) + ctx = Mock() + ctx.params = { + "url": f"{firecrest_config.url}", + "token_uri": f"{firecrest_config.token_uri}", + "client_id": f"{firecrest_config.client_id}", + "client_secret": f"{firecrest_config.client_secret}", + "compute_resource": f"{firecrest_config.compute_resource}", + "temp_directory": f"{firecrest_config.temp_directory}", + "api_version": f"{firecrest_config.api_version}", + } + + # should catch FIRECREST_VERSION if value is not provided + result = _dynamic_info_firecrest_version(ctx, None, "0") + assert isinstance(result, str) + + # should use the value if provided + result = _dynamic_info_firecrest_version(ctx, None, "10.10.10") + assert result == "10.10.10" From 09b264a49880f6ac67d28c2e587f846988e6eb0e Mon Sep 17 00:00:00 2001 From: Ali Date: Thu, 12 Sep 2024 10:42:47 +0200 Subject: [PATCH 3/6] review applied --- aiida_firecrest/transport.py | 34 +++++++++++++++-------- tests/test_computer.py | 52 +++++++++++++++++++++++++++++++++--- 2 files changed, 71 insertions(+), 15 deletions(-) diff --git a/aiida_firecrest/transport.py b/aiida_firecrest/transport.py index 60f9eef..30eb6ee 100644 --- a/aiida_firecrest/transport.py +++ b/aiida_firecrest/transport.py @@ -128,17 +128,28 @@ def _validate_temp_directory(ctx: Context, param: InteractiveOption, value: str) def _dynamic_info_firecrest_version( ctx: Context, param: InteractiveOption, value: str ) -> str: - """Find the version of the FirecREST server.""" - # note: right now, unfortunately, the version is not exposed in the API. - # See issue https://github.com/eth-cscs/firecrest/issues/204 - # so here we just develope a workaround to get the version from the server - # basically we check if extract/compress endpoint is available + """ + Find the version of the FirecREST server. + This is a callback function for click, interface. + It's called during the parsing of the command line arguments, during `verdi computer configure` command. + If the user enters "None" as value, this function will connect to the server and get the version of the FirecREST server. + Otherwise, it will check if the version is supported. + """ import click + from packaging.version import InvalidVersion + + if value != "None": + try: + parse(value) + except InvalidVersion as err: + # raise in case the version is not valid, e.g. latest, stable, etc. + raise click.BadParameter(f"Invalid input {value}") from err - if value != "0": if parse(value) < parse("1.15.0"): raise click.BadParameter(f"FirecREST api version {value} is not supported") + # If version is provided by the user, and it's supported, we will just return it. + # No print confirmation is needed, to keep things less verbose. return value firecrest_url = ctx.params["url"] @@ -174,13 +185,14 @@ def _dynamic_info_firecrest_version( else: raise KeyError except KeyError as err: - raise click.BadParameter( - "Could not get the version of the FirecREST server" - ) from err + click.echo("Could not get the version of the FirecREST server") + raise click.Abort() from err if parse(_version) < parse("1.15.0"): - raise click.BadParameter(f"FirecREST api version {_version} is not supported") + click.echo(f"FirecREST api version {_version} is not supported") + raise click.Abort() + # for the sake of uniformity, we will print the version in style only if dynamically retrieved. click.echo( click.style("Fireport: ", bold=True, fg="magenta") + f"FirecRESTapi: {_version}" ) @@ -322,7 +334,7 @@ class FirecrestTransport(Transport): "api_version", { "type": str, - "default": "0", + "default": "None", "non_interactive_default": True, "prompt": "FirecREST api version [Enter 0 to get this info from server]", "help": "The version of the FirecREST api deployed on the server", diff --git a/tests/test_computer.py b/tests/test_computer.py index 4b37ea4..62f4aad 100644 --- a/tests/test_computer.py +++ b/tests/test_computer.py @@ -110,7 +110,6 @@ def test_validate_temp_directory( def test_dynamic_info_direct_size(firecrest_config, monkeypatch, tmpdir: Path): from aiida_firecrest.transport import _dynamic_info_direct_size - assert monkeypatch is not None monkeypatch.setattr("click.echo", lambda x: None) ctx = Mock() ctx.params = { @@ -133,11 +132,17 @@ def test_dynamic_info_direct_size(firecrest_config, monkeypatch, tmpdir: Path): assert result == 10 -def test_dynamic_info_firecrest_version(firecrest_config, monkeypatch, tmpdir: Path): +def test_dynamic_info_firecrest_version( + firecrest_config, monkeypatch, tmpdir: Path, capsys +): + + from unittest.mock import patch + + from click import Abort from aiida_firecrest.transport import _dynamic_info_firecrest_version + from conftest import MockFirecrest - monkeypatch.setattr("click.echo", lambda x: None) ctx = Mock() ctx.params = { "url": f"{firecrest_config.url}", @@ -150,9 +155,48 @@ def test_dynamic_info_firecrest_version(firecrest_config, monkeypatch, tmpdir: P } # should catch FIRECREST_VERSION if value is not provided - result = _dynamic_info_firecrest_version(ctx, None, "0") + result = _dynamic_info_firecrest_version(ctx, None, "None") assert isinstance(result, str) # should use the value if provided result = _dynamic_info_firecrest_version(ctx, None, "10.10.10") assert result == "10.10.10" + + # raise BadParameter if the version is not supported + with pytest.raises(BadParameter) as exc_info: + result = _dynamic_info_firecrest_version(ctx, None, "0.0.0") + assert "FirecREST api version 0.0.0 is not supported" in str(exc_info.value) + + # raise BadParameter if the input is nonsense, latest, stable, etc. + with pytest.raises(BadParameter) as exc_info: + result = _dynamic_info_firecrest_version(ctx, None, "latest") + assert "Invalid input" in str(exc_info.value) + + # in case could not get the version from the server, should abort, as it is a required parameter. + with patch.object(MockFirecrest, "parameters", autospec=True) as mock_parameters: + mock_parameters.return_value = {"there is no version key": "bye bye"} + with pytest.raises(Abort): + result = _dynamic_info_firecrest_version(ctx, None, "None") + capture = capsys.readouterr() + assert "Could not get the version of the FirecREST server" in capture.out + + # in case the version recieved from the server is not supported, should abort, as no magic input can solve this problem. + with patch.object(MockFirecrest, "parameters", autospec=True) as mock_parameters: + unsupported_version = "0.1" + mock_parameters.return_value = { + "general": [ + { + "description": "FirecREST version.", + "name": "FIRECREST_VERSION", + "unit": "", + "value": f"v{unsupported_version}", + }, + ] + } + with pytest.raises(Abort): + result = _dynamic_info_firecrest_version(ctx, None, "None") + capture = capsys.readouterr() + assert ( + f"FirecREST api version v{unsupported_version} is not supported" + in capture.out + ) From 668b66954338e9849db54f14c35f7243f57f2021 Mon Sep 17 00:00:00 2001 From: Alexander Goscinski Date: Thu, 12 Sep 2024 14:50:38 +0200 Subject: [PATCH 4/6] Apply suggestions from code review --- tests/test_computer.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/tests/test_computer.py b/tests/test_computer.py index 62f4aad..97d37b3 100644 --- a/tests/test_computer.py +++ b/tests/test_computer.py @@ -163,14 +163,12 @@ def test_dynamic_info_firecrest_version( assert result == "10.10.10" # raise BadParameter if the version is not supported - with pytest.raises(BadParameter) as exc_info: - result = _dynamic_info_firecrest_version(ctx, None, "0.0.0") - assert "FirecREST api version 0.0.0 is not supported" in str(exc_info.value) + with pytest.raises(BadParameter, match=r".*FirecREST api version 0.0.0 is not supported.*")): + _dynamic_info_firecrest_version(ctx, None, "0.0.0") # raise BadParameter if the input is nonsense, latest, stable, etc. - with pytest.raises(BadParameter) as exc_info: - result = _dynamic_info_firecrest_version(ctx, None, "latest") - assert "Invalid input" in str(exc_info.value) + with pytest.raises(BadParameter, match=r".*Invalid input.*"): + _dynamic_info_firecrest_version(ctx, None, "latest") # in case could not get the version from the server, should abort, as it is a required parameter. with patch.object(MockFirecrest, "parameters", autospec=True) as mock_parameters: From 21f5af64e589ffb5264a4ff113044941ed0b369a Mon Sep 17 00:00:00 2001 From: Alexander Goscinski Date: Thu, 12 Sep 2024 14:52:29 +0200 Subject: [PATCH 5/6] fix typo --- tests/test_computer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_computer.py b/tests/test_computer.py index 97d37b3..bddaefd 100644 --- a/tests/test_computer.py +++ b/tests/test_computer.py @@ -163,7 +163,7 @@ def test_dynamic_info_firecrest_version( assert result == "10.10.10" # raise BadParameter if the version is not supported - with pytest.raises(BadParameter, match=r".*FirecREST api version 0.0.0 is not supported.*")): + with pytest.raises(BadParameter, match=r".*FirecREST api version 0.0.0 is not supported.*"): _dynamic_info_firecrest_version(ctx, None, "0.0.0") # raise BadParameter if the input is nonsense, latest, stable, etc. From 32e5ec15a797a49169a0b4a3fe4a409ceeb9f66b Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 12 Sep 2024 12:53:01 +0000 Subject: [PATCH 6/6] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/test_computer.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/test_computer.py b/tests/test_computer.py index bddaefd..e89ff1c 100644 --- a/tests/test_computer.py +++ b/tests/test_computer.py @@ -163,8 +163,10 @@ def test_dynamic_info_firecrest_version( assert result == "10.10.10" # raise BadParameter if the version is not supported - with pytest.raises(BadParameter, match=r".*FirecREST api version 0.0.0 is not supported.*"): - _dynamic_info_firecrest_version(ctx, None, "0.0.0") + with pytest.raises( + BadParameter, match=r".*FirecREST api version 0.0.0 is not supported.*" + ): + _dynamic_info_firecrest_version(ctx, None, "0.0.0") # raise BadParameter if the input is nonsense, latest, stable, etc. with pytest.raises(BadParameter, match=r".*Invalid input.*"):