From 037b55ba7ddc6b89691f233b2d9d4230b1606118 Mon Sep 17 00:00:00 2001 From: Dries Schaumont <5946712+DriesSchaumont@users.noreply.github.com> Date: Mon, 22 Apr 2024 10:28:14 +0200 Subject: [PATCH] Add support for viash 0.9.0 (#23) * Add support for viash 0.9.0 * Fix python 3.9 * Move import up * Add PR number to CHANGELOG [ci skip] * Reverse info/build_info check --- CHANGELOG.rst | 15 +- .../unittests/fixtures/test_run_component.py | 208 +++++++++++++++++- tox.ini | 3 +- viashpy/_run.py | 79 ++++++- viashpy/testing.py | 19 +- viashpy/types.py | 3 +- 6 files changed, 295 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 15c3d98..9d6655c 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,19 +5,24 @@ Changelog 0.7.0 (TBD) =========== +New Functionality +----------------- + +* Added support for viash 0.9.0 (#23). + Breaking Changes ---------------- -* `run_component` now executes `viash run` with `--platform docker` when the test - is executed inline (and not as a result of using 'viash test') instead of the first - platform that is defined in the config. Another platform can be specified by using - the `platform` keyword argument (#22). +* `run_component` now executes `viash run` with `--engine docker` (or `--platform docker`) + when the test is executed inline (and not as a result of using 'viash test') + instead of the first engine that is defined in the config. Another engine can be + specified by using the `engine` keyword argument (#22). Minor Changes ------------- * `run_component` now uses `cachedbuild` setup strategy when tests are executed inline - with platform `docker`. This makes sure that the docker image is update when it already + with engine `docker`. This makes sure that the docker image is update when it already exists on the system (#22). * Added debugging information about the calls that are made to `viash`. If you want to enable diff --git a/tests/unittests/fixtures/test_run_component.py b/tests/unittests/fixtures/test_run_component.py index 97fc43e..b73d249 100644 --- a/tests/unittests/fixtures/test_run_component.py +++ b/tests/unittests/fixtures/test_run_component.py @@ -46,6 +46,126 @@ def test_loading_run_component(run_component): assert result.ret != 0 +def test_run_viash_returns_unknown_viash_version_format_raises( + pytester, makepyfile_and_add_meta, dummy_config +): + makepyfile_and_add_meta( + """ + import subprocess + from pathlib import Path + + def test_loading_run_component(mocker, run_component): + mocked = mocker.patch('viashpy._run.check_output', return_value=b"UNKNOWNVERSION") + run_component(["bar"]) + mocked.assert_called_once_with([Path("foo"), "bar"], + stderr=subprocess.STDOUT) + """, + dummy_config, + "foo", + ) + result = pytester.runpytest("-v") + result.stdout.fnmatch_lines( + [ + "*Could not parse the version information as output by viash.*", + ] + ) + assert result.ret != 0 + + +@pytest.mark.parametrize("viash_version", ["(0, 8, 2)", "(0, 9, 0)"]) +def test_run_both_platform_and_engine_raises( + pytester, makepyfile_and_add_meta, dummy_config, viash_version +): + makepyfile_and_add_meta( + f""" + import subprocess + from pathlib import Path + + def test_loading_run_component(mocker, run_component): + mocked_viash_check = mocker.patch('viashpy._run._get_viash_version', return_value={viash_version}) + run_component(["bar"], platform="native", engine="native") + """, + dummy_config, + "foo", + ) + result = pytester.runpytest("-v") + result.stdout.fnmatch_lines( + [ + "*ValueError: Cannot pass both an 'engine' and a 'plaform'.*", + ] + ) + assert result.ret != 0 + + +@pytest.mark.parametrize( + "viash_version, wrong_platform_or_engine", + [((0, 8, 2), "engine"), ((0, 9, 0), "platform")], +) +def test_engine_platform_specified_but_wrong_viash_version( + pytester, + makepyfile_and_add_meta, + dummy_config, + viash_version, + wrong_platform_or_engine, +): + makepyfile_and_add_meta( + f""" + import subprocess + from functools import partial + from pathlib import Path + + def test_loading_run_component(mocker, run_component): + mocked_viash_check = mocker.patch('viashpy._run._get_viash_version', + return_value=({','.join(map(str, viash_version))})) + run_component(["bar"], {wrong_platform_or_engine}="native") + """, + dummy_config, + "foo", + ) + result = pytester.runpytest("-v") + correct_arg = "engine" if wrong_platform_or_engine == "platform" else "platform" + result.stdout.fnmatch_lines( + [ + f"*ValueError: Viash version {'.'.join(map(str, viash_version))} requires using " + f"'{correct_arg}' instead of '{wrong_platform_or_engine}'.*", + ] + ) + assert result.ret != 0 + + +def test_could_not_get_viash_version_raises( + pytester, makepyfile_and_add_meta, dummy_config +): + makepyfile_and_add_meta( + """ + import subprocess + from functools import partial + from pathlib import Path + + def test_loading_run_component(mocker, run_component): + mocked_viash_check = mocker.patch('viashpy._run.check_output', + side_effect=subprocess.CalledProcessError(1, "foo")) + run_component(["bar"], platform="native") + """, + dummy_config, + "foo", + ) + result = pytester.runpytest("-v") + result.stdout.fnmatch_lines( + [ + "*subprocess.CalledProcessError: Command 'foo' returned non-zero exit status 1.*", + ] + ) + assert result.ret != 0 + + +@pytest.mark.parametrize( + "viash_version, platform_or_engine", + [ + ("viash 0.8.2 (c) 2020 Data Intuitive", "platform"), + ("viash 0.9.0 (c) 2020 Data Intuitive", "engine"), + ], +) @pytest.mark.parametrize( "memory_pb, memory_tb, memory_gb, memory_mb, memory_kb, memory_b, expected_bytes, expected_warning", [ @@ -135,6 +255,8 @@ def test_run_component_different_memory_specification_warnings( memory_b, expected_bytes, expected_warning, + viash_version, + platform_or_engine, ): expected_memory_args = ", " memory_specifiers = [ @@ -151,8 +273,8 @@ def test_run_component_different_memory_specification_warnings( if any(memory_specifiers): expected_memory_args = f', "--memory", "{expected_bytes}B", ' expected_base_command = ( - '["viash", "run", Path(meta["config"]), "-p", "docker", "-c",' - "\".platforms[.type == 'docker'].target_tag := 'test'\"" + f'["viash", "run", Path(meta["config"]), "--{platform_or_engine}", "docker", "-c",' + f"\".{platform_or_engine}s[.type == 'docker'].target_tag := 'test'\"" ) expected_build_call = ( f"mocker.call({expected_base_command}, " @@ -170,10 +292,11 @@ def test_run_component_different_memory_specification_warnings( def test_loading_run_component(mocker, run_component): mocked_check_output = mocker.patch('viashpy._run.check_output', - return_value=b"Some dummy output") + side_effect=[b"{viash_version}", None, b"Some dummy output"]) mocked_path = mocker.patch('viashpy.testing.Path.is_file', return_value=True) stdout = run_component(["bar"]) - mocked_check_output.assert_has_calls([{expected_build_call}, + mocked_check_output.assert_has_calls([mocker.call(['viash', '--version']), + {expected_build_call}, {expected_run_call}]) assert stdout == b"Some dummy output" """, @@ -191,12 +314,67 @@ def test_loading_run_component(mocker, run_component): if expected_warning: result.stdout.fnmatch_lines( [ - "*Different values were defined in the 'meta' dictionary that limit memory, choosing the one with the smallest unit.*" + "*Different values were defined in the 'meta' dictionary that " + "limit memory, choosing the one with the smallest unit.*" ] ) assert result.ret == 0 +@pytest.mark.parametrize( + "viash_version, platform_or_engine", + [((0, 8, 2), "platform"), ((0, 9, 0), "engine")], +) +@pytest.mark.parametrize( + "expected_build_cmd, expected_run_cmd", + [ + ( + '["viash", "run", Path(meta["config"]), "--{0}", "docker", "-c", ' + '".{0}s[.type == \'docker\'].target_tag := \'test\'", "--", "---setup", "cachedbuild"]', + '["viash", "run", Path(meta["config"]), "--{0}", "docker", ' + '"-c", ".{0}s[.type == \'docker\'].target_tag := \'test\'", "--", "bar"]', + ), + ], +) +def test_run_component_set_platform_or_engine( + pytester, + makepyfile_and_add_meta, + dummy_config, + expected_build_cmd, + expected_run_cmd, + viash_version, + platform_or_engine, +): + expected_build_cmd_call, excpected_run_cmd_call = "", "" + if expected_build_cmd: + expected_build_cmd = expected_build_cmd.format(platform_or_engine) + expected_build_cmd_call = f"mocker.call({expected_build_cmd}, stderr=-2)" + if expected_run_cmd: + expected_run_cmd = expected_run_cmd.format(platform_or_engine) + excpected_run_cmd_call = f"mocker.call({expected_run_cmd}, stderr=-2)" + expected = ", ".join( + filter(None, [expected_build_cmd_call, excpected_run_cmd_call]) + ) + makepyfile_and_add_meta( + f""" + import subprocess + from pathlib import Path + + def test_loading_run_component(mocker, run_component): + mocked_viash_check = mocker.patch('viashpy._run._get_viash_version', return_value={viash_version}) + mocked_check_output = mocker.patch('viashpy._run.check_output', return_value=b"Some dummy output") + mocked_path = mocker.patch('viashpy.testing.Path.is_file', return_value=True) + stdout = run_component(["bar"], {platform_or_engine}="docker") + mocked_check_output.assert_has_calls([{expected}]) + assert stdout == b"Some dummy output" + """, + dummy_config, + "foo", + ) + result = pytester.runpytest("-v") + result.assert_outcomes(passed=1) + + @pytest.mark.parametrize("memory, expected_bytes", [(None, None), (6, 6442450944)]) @pytest.mark.parametrize("cpu", [None, 2]) @pytest.mark.parametrize( @@ -204,9 +382,9 @@ def test_loading_run_component(mocker, run_component): [ ( "dummy_config", - '["viash", "run", Path(meta["config"]), "-p", "docker", "-c", ' + '["viash", "run", Path(meta["config"]), "--platform", "docker", "-c", ' '".platforms[.type == \'docker\'].target_tag := \'test\'", "--", "---setup", "cachedbuild"]', - '["viash", "run", Path(meta["config"]), "-p", "docker", ' + '["viash", "run", Path(meta["config"]), "--platform", "docker", ' '"-c", ".platforms[.type == \'docker\'].target_tag := \'test\'"%s%s, "--", "bar"]', "--", ), @@ -230,14 +408,24 @@ def test_run_component_executes_subprocess( f', "{expected_prefix}memory", "{expected_bytes}B"' if memory else "", ) expected_run_cmd = expected_run_cmd % format_string - expected_build_cmd_call, excpected_run_cmd_call = "", "" + ( + expected_version_cmd, + expected_build_cmd_call, + excpected_run_cmd_call, + mocked_return, + ) = [""] * 4 if expected_build_cmd: + mocked_return += 'b"viash 0.8.2 (c) 2020 Data Intuitive", None, ' expected_build_cmd_call = f"mocker.call({expected_build_cmd}, stderr=-2)" if expected_run_cmd: excpected_run_cmd_call = f"mocker.call({expected_run_cmd}, stderr=-2)" expected = ", ".join( - filter(None, [expected_build_cmd_call, excpected_run_cmd_call]) + filter( + None, + [expected_version_cmd, expected_build_cmd_call, excpected_run_cmd_call], + ) ) + mocked_return += 'b"Some dummy output"' makepyfile_and_add_meta( f""" import subprocess @@ -245,7 +433,7 @@ def test_run_component_executes_subprocess( def test_loading_run_component(mocker, run_component): mocked_check_output = mocker.patch('viashpy._run.check_output', - return_value=b"Some dummy output") + side_effect=[{mocked_return}]) mocked_path = mocker.patch('viashpy.testing.Path.is_file', return_value=True) stdout = run_component(["bar"]) mocked_check_output.assert_has_calls([{expected}]) diff --git a/tox.ini b/tox.ini index c95b9d3..97db884 100644 --- a/tox.ini +++ b/tox.ini @@ -2,7 +2,7 @@ [tox] envlist = py{39}-pytest{62} - py{39,310,311,312}-pytest{70,71,72,73,74,80} + py{39,310,311,312}-pytest{70,71,72,73,74,80,81} flake8 isolated_build = True @@ -15,6 +15,7 @@ deps = pytest73: pytest>=7.3,<7.4 pytest74: pytest>=7.4,<7.5 pytest80: pytest>=8.0,<8.1 + pytest81: pytest>=8.1,<8.2 coverage pytest-mock commands = diff --git a/viashpy/_run.py b/viashpy/_run.py index 367b384..aaf8759 100644 --- a/viashpy/_run.py +++ b/viashpy/_run.py @@ -1,9 +1,10 @@ from __future__ import annotations -from subprocess import check_output, STDOUT, DEVNULL, PIPE +from subprocess import check_output, STDOUT, DEVNULL, PIPE, CalledProcessError from pathlib import Path from typing import Any -from .types import Platform +from .types import Engine, Platform import logging +import re logger = logging.getLogger(__name__) @@ -49,6 +50,58 @@ def _format_cpu_and_memory(cpus, memory, arg_prefix="---"): return result +def _get_viash_version(viash_executable): + try: + version_string = check_output([viash_executable, "--version"]).decode().strip() + version_pattern = re.compile( + r"^viash ([0-9]+.[0-9]+.[0-9]+).* \(c\) [0-9]{4} Data Intuitive$" + ) + version_match = re.fullmatch(version_pattern, version_string) + if not version_match: + raise ValueError( + "Could not parse the version information as output by viash." + ) + return tuple(map(int, (version_match.group(1).split(".")))) + except CalledProcessError as e: + logger.error( + "Could not determine the viash version. " + "Please make sure that viash is in your PATH " + "or that you overwrite the 'viash_executable' fixture " + "to return the correct location of the viash binairy." + ) + raise e + + +def _check_platform_or_engine( + viash_version: tuple[int, int, int], + engine: Engine | None = None, + platform: Platform | None = None, +): + major_viash_version, minor_viash_version, _ = viash_version + if platform is None and engine is None: + # Use an appropriate default for the viash version + if major_viash_version == 0 and minor_viash_version < 9: + return "platform", "docker" + else: + return "engine", "docker" + if platform and engine: + raise ValueError("Cannot pass both an 'engine' and a 'plaform'.") + if major_viash_version == 0 and minor_viash_version < 9: + if engine: + raise ValueError( + f"Viash version {'.'.join(map(str, viash_version))} " + "requires using 'platform' instead of 'engine'." + ) + return "platform", platform + else: + if platform: + raise ValueError( + f"Viash version {'.'.join(map(str, viash_version))} " + "requires using 'engine' instead of 'platform'." + ) + return "engine", engine + + def run_build_component( executable_location: str | Path, args: list[str], @@ -74,7 +127,8 @@ def viash_run( *, cpus: int | None = None, memory: str | None = None, - platform: Platform = "docker", + engine: Engine | None = None, + platform: Platform | None = None, viash_location: str | Path = "viash", stderr: STDOUT | PIPE | DEVNULL | -1 | -2 | -3 = STDOUT, **popen_kwargs, @@ -83,15 +137,20 @@ def viash_run( if not config.is_file(): raise FileNotFoundError(f"{config} does not exist or is not a file.") args = ["--"] + args - if platform == "docker": + + viash_version = _get_viash_version(viash_location) + platform_or_engine, engine_or_platform_val = _check_platform_or_engine( + viash_version, engine=engine, platform=platform + ) + if engine_or_platform_val == "docker": build_args = [ viash_location, "run", config, - "-p", - "docker", + f"--{platform_or_engine}", + engine_or_platform_val, "-c", - ".platforms[.type == 'docker'].target_tag := 'test'", + f".{platform_or_engine}s[.type == 'docker'].target_tag := 'test'", "--", "---setup", "cachedbuild", @@ -105,10 +164,10 @@ def viash_run( viash_location, "run", config, - "-p", - platform, + f"--{platform_or_engine}", + engine_or_platform_val, "-c", - ".platforms[.type == 'docker'].target_tag := 'test'", + f".{platform_or_engine}s[.type == 'docker'].target_tag := 'test'", ] + _format_cpu_and_memory(cpus, memory, "--") + args diff --git a/viashpy/testing.py b/viashpy/testing.py index 8e4eb46..7fa1854 100644 --- a/viashpy/testing.py +++ b/viashpy/testing.py @@ -1,7 +1,8 @@ +from __future__ import annotations import pytest import logging from ._run import run_build_component, viash_run, tobytesconverter -from .types import Platform +from .types import Engine, Platform from .config import read_viash_config from pathlib import Path from functools import wraps @@ -134,10 +135,15 @@ def viash_source_config_path(meta_config_path, meta_config): """ try: # meta_config is a parsed viash config, retreive the location of the source - return Path(meta_config["info"]["config"]) + return Path(meta_config["build_info"]["config"]) except KeyError: - # If .['info']['config'] is not defined, assume that the config is a source config - return meta_config_path + # viash < 0.9 defines info instead of build_info + try: + return Path(meta_config["info"]["config"]) + except KeyError: + # If .['info']['config'] or .['build_info']['config'] is not defined, + # assume that the config is a source config + return meta_config_path @pytest.fixture @@ -184,13 +190,16 @@ def wrapper(*args, **kwargs): if viash_source_config_path.is_file(): @run_and_handle_errors - def wrapper(args_as_list, platform: Platform = "docker"): + def wrapper( + args_as_list, engine: Engine | None = None, platform: Platform | None = None + ): return viash_run( viash_source_config_path, args_as_list, viash_location=viash_executable, cpus=cpus, memory=memory_bytes, + engine=engine, platform=platform, ) diff --git a/viashpy/types.py b/viashpy/types.py index 748e2fd..a027dd6 100644 --- a/viashpy/types.py +++ b/viashpy/types.py @@ -1,3 +1,4 @@ from typing import Literal -Platform = Literal["native", "docker", "nextflow"] +Engine = Literal["native", "docker", "nextflow"] +Platform = Engine