From 25f438d52a7cbeeafccb9987df9b2509db39e2ab Mon Sep 17 00:00:00 2001 From: Donald Campbell <125581724+donaldcampbelljr@users.noreply.github.com> Date: Thu, 14 Mar 2024 14:14:34 -0400 Subject: [PATCH 1/8] add simple tests, and return false if path ends with csv for is_registry_path #456 --- looper/utils.py | 2 +- tests/conftest.py | 18 ++++++++++++++++++ tests/smoketests/test_run.py | 21 +++++++++++++++++++++ 3 files changed, 40 insertions(+), 1 deletion(-) diff --git a/looper/utils.py b/looper/utils.py index aaee23336..1ba6af9e4 100644 --- a/looper/utils.py +++ b/looper/utils.py @@ -605,7 +605,7 @@ def is_registry_path(input_string: str) -> bool: :return bool: True if input is a registry path """ try: - if input_string.endswith(".yaml"): + if input_string.endswith(".yaml") or input_string.endswith(".csv"): return False except AttributeError: raise RegistryPathException( diff --git a/tests/conftest.py b/tests/conftest.py index ac8f71a2f..0293bd807 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -230,6 +230,24 @@ def prep_temp_pep_basic(example_pep_piface_path): return path_to_looper_config +@pytest.fixture +def prep_temp_pep_csv(example_pep_piface_path): + + # Get Path to local copy of hello_looper + hello_looper_dir_path = os.path.join( + example_pep_piface_path, "hello_looper-dev_derive" + ) + + # Make local temp copy of hello_looper + d = tempfile.mkdtemp() + shutil.copytree(hello_looper_dir_path, d, dirs_exist_ok=True) + + advanced_dir = os.path.join(d, "csv") + path_to_looper_config = os.path.join(advanced_dir, ".looper.yaml") + + return path_to_looper_config + + @pytest.fixture def prep_temp_config_with_pep(example_pep_piface_path): # temp dir diff --git a/tests/smoketests/test_run.py b/tests/smoketests/test_run.py index ee1d54cc6..18c10f98e 100644 --- a/tests/smoketests/test_run.py +++ b/tests/smoketests/test_run.py @@ -23,6 +23,16 @@ def test_cli(prep_temp_pep): raise pytest.fail("DID RAISE {0}".format(Exception)) +def test_running_csv_pep(prep_temp_pep_csv): + tp = prep_temp_pep_csv + + x = ["run", "--looper-config", tp, "--dry-run"] + try: + main(test_args=x) + except Exception: + raise pytest.fail("DID RAISE {0}".format(Exception)) + + def is_connected(): """Determines if local machine can connect to the internet.""" import socket @@ -597,3 +607,14 @@ def test_init_project_using_dict(self, prep_temp_config_with_pep): ) assert len(init_project.pipeline_interfaces) == 3 + + def test_init_project_using_csv(self, prep_temp_pep_csv): + """Verify looper runs using pephub in a basic case and return code is 0""" + tp = prep_temp_pep_csv + with mod_yaml_data(tp) as config_data: + pep_config_csv = config_data["pep_config"] + + pep_config_csv = os.path.join(os.path.dirname(tp), pep_config_csv) + init_project = Project(cfg=pep_config_csv) + + assert len(init_project.samples) == 2 From 199ba63f0dfa8fe2ddb305b59a4e70ffd17a11fa Mon Sep 17 00:00:00 2001 From: Donald Campbell <125581724+donaldcampbelljr@users.noreply.github.com> Date: Tue, 19 Mar 2024 13:50:08 -0400 Subject: [PATCH 2/8] skip simple test since functionality is broken --- tests/smoketests/test_run.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/smoketests/test_run.py b/tests/smoketests/test_run.py index 18c10f98e..932dfbe37 100644 --- a/tests/smoketests/test_run.py +++ b/tests/smoketests/test_run.py @@ -23,6 +23,7 @@ def test_cli(prep_temp_pep): raise pytest.fail("DID RAISE {0}".format(Exception)) +@pytest.mark.skip(reason="PEP via CSV is currently broken.") def test_running_csv_pep(prep_temp_pep_csv): tp = prep_temp_pep_csv From b6aeda05b0718604784149cdbdf44e4e3288dcc9 Mon Sep 17 00:00:00 2001 From: Donald Campbell <125581724+donaldcampbelljr@users.noreply.github.com> Date: Tue, 19 Mar 2024 15:51:42 -0400 Subject: [PATCH 3/8] skip simple test since functionality is broken --- looper/utils.py | 15 +++++++++++++++ tests/smoketests/test_run.py | 9 +++++++++ 2 files changed, 24 insertions(+) diff --git a/looper/utils.py b/looper/utils.py index 1ba6af9e4..8c233243a 100644 --- a/looper/utils.py +++ b/looper/utils.py @@ -598,6 +598,21 @@ def dotfile_path(directory=os.getcwd(), must_exist=False): cur_dir = parent_dir +def is_PEP_file_type(input_string: str) -> bool: + """ + Determines if the provided path is actually a file type that Looper can use for loading PEP + """ + + PEP_FILE_TYPES = ["yaml", "csv"] + + parsed_path = parse_registry_path(input_string) + + if parsed_path["subitem"] in PEP_FILE_TYPES: + return True + else: + return False + + def is_registry_path(input_string: str) -> bool: """ Check if input is a registry path to pephub diff --git a/tests/smoketests/test_run.py b/tests/smoketests/test_run.py index 932dfbe37..fc8650fd0 100644 --- a/tests/smoketests/test_run.py +++ b/tests/smoketests/test_run.py @@ -34,6 +34,15 @@ def test_running_csv_pep(prep_temp_pep_csv): raise pytest.fail("DID RAISE {0}".format(Exception)) +@pytest.mark.parametrize( + "path", ["something/example.yaml", "somethingelse/example2.csv"] +) +def test_is_PEP_file_type(path): + + result = is_PEP_file_type(path) + assert result == True + + def is_connected(): """Determines if local machine can connect to the internet.""" import socket From 11517edf4b6cb7638a34dca8bcf69450341f809f Mon Sep 17 00:00:00 2001 From: Donald Campbell <125581724+donaldcampbelljr@users.noreply.github.com> Date: Tue, 19 Mar 2024 15:51:42 -0400 Subject: [PATCH 4/8] add checking for pep file type and simple test --- looper/utils.py | 15 +++++++++++++++ tests/smoketests/test_run.py | 9 +++++++++ 2 files changed, 24 insertions(+) diff --git a/looper/utils.py b/looper/utils.py index 1ba6af9e4..8c233243a 100644 --- a/looper/utils.py +++ b/looper/utils.py @@ -598,6 +598,21 @@ def dotfile_path(directory=os.getcwd(), must_exist=False): cur_dir = parent_dir +def is_PEP_file_type(input_string: str) -> bool: + """ + Determines if the provided path is actually a file type that Looper can use for loading PEP + """ + + PEP_FILE_TYPES = ["yaml", "csv"] + + parsed_path = parse_registry_path(input_string) + + if parsed_path["subitem"] in PEP_FILE_TYPES: + return True + else: + return False + + def is_registry_path(input_string: str) -> bool: """ Check if input is a registry path to pephub diff --git a/tests/smoketests/test_run.py b/tests/smoketests/test_run.py index 932dfbe37..fc8650fd0 100644 --- a/tests/smoketests/test_run.py +++ b/tests/smoketests/test_run.py @@ -34,6 +34,15 @@ def test_running_csv_pep(prep_temp_pep_csv): raise pytest.fail("DID RAISE {0}".format(Exception)) +@pytest.mark.parametrize( + "path", ["something/example.yaml", "somethingelse/example2.csv"] +) +def test_is_PEP_file_type(path): + + result = is_PEP_file_type(path) + assert result == True + + def is_connected(): """Determines if local machine can connect to the internet.""" import socket From 089f9ee8bada38c7fdd15ff0a2e531d341751cc5 Mon Sep 17 00:00:00 2001 From: Donald Campbell <125581724+donaldcampbelljr@users.noreply.github.com> Date: Tue, 19 Mar 2024 16:12:24 -0400 Subject: [PATCH 5/8] add check for PEP file types first THEN attempt registry path, some tests broken due to exceptions --- looper/cli_pydantic.py | 36 +++++++++++++++++++++--------------- looper/utils.py | 8 ++------ 2 files changed, 23 insertions(+), 21 deletions(-) diff --git a/looper/cli_pydantic.py b/looper/cli_pydantic.py index 1f7808606..80beaeda4 100644 --- a/looper/cli_pydantic.py +++ b/looper/cli_pydantic.py @@ -47,6 +47,7 @@ init_generic_pipeline, read_yaml_file, inspect_looper_config_file, + is_PEP_file_type, ) from typing import List, Tuple @@ -176,41 +177,46 @@ def run_looper(args: TopLevelParser, parser: ArgumentParser, test_args=None): subcommand_args.ignore_flags = True # Initialize project - if is_registry_path(subcommand_args.config_file): - if vars(subcommand_args)[SAMPLE_PL_ARG]: + if is_PEP_file_type(subcommand_args.config_file) and os.path.exists(subcommand_args.config_file): + try: p = Project( + cfg=subcommand_args.config_file, amendments=subcommand_args.amend, divcfg_path=divcfg, runp=subcommand_name == "runp", - project_dict=PEPHubClient()._load_raw_pep( - registry_path=subcommand_args.config_file - ), **{ attr: getattr(subcommand_args, attr) for attr in CLI_PROJ_ATTRS if attr in subcommand_args }, ) - else: - raise MisconfigurationException( - f"`sample_pipeline_interface` is missing. Provide it in the parameters." - ) - else: - try: + except yaml.parser.ParserError as e: + _LOGGER.error(f"Project config parse failed -- {e}") + sys.exit(1) + elif is_registry_path(subcommand_args.config_file): + if vars(subcommand_args)[SAMPLE_PL_ARG]: p = Project( - cfg=subcommand_args.config_file, amendments=subcommand_args.amend, divcfg_path=divcfg, runp=subcommand_name == "runp", + project_dict=PEPHubClient()._load_raw_pep( + registry_path=subcommand_args.config_file + ), **{ attr: getattr(subcommand_args, attr) for attr in CLI_PROJ_ATTRS if attr in subcommand_args }, ) - except yaml.parser.ParserError as e: - _LOGGER.error(f"Project config parse failed -- {e}") - sys.exit(1) + else: + raise MisconfigurationException( + f"`sample_pipeline_interface` is missing. Provide it in the parameters." + ) + else: + raise MisconfigurationException( + f"Cannot load PEP. Check file path or registry path to pep." + ) + selected_compute_pkg = p.selected_compute_package or DEFAULT_COMPUTE_RESOURCES_NAME if p.dcc is not None and not p.dcc.activate_package(selected_compute_pkg): diff --git a/looper/utils.py b/looper/utils.py index 8c233243a..fe2e78175 100644 --- a/looper/utils.py +++ b/looper/utils.py @@ -605,12 +605,8 @@ def is_PEP_file_type(input_string: str) -> bool: PEP_FILE_TYPES = ["yaml", "csv"] - parsed_path = parse_registry_path(input_string) - - if parsed_path["subitem"] in PEP_FILE_TYPES: - return True - else: - return False + res = list(filter(input_string.endswith, PEP_FILE_TYPES)) != [] + return res def is_registry_path(input_string: str) -> bool: From f300c838f4f8bcf36dc448311994c4555caf0f2a Mon Sep 17 00:00:00 2001 From: Donald Campbell <125581724+donaldcampbelljr@users.noreply.github.com> Date: Wed, 20 Mar 2024 08:29:35 -0400 Subject: [PATCH 6/8] fix tests by changing expected Exception --- looper/cli_pydantic.py | 5 +++-- tests/smoketests/test_other.py | 5 ++--- tests/smoketests/test_run.py | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/looper/cli_pydantic.py b/looper/cli_pydantic.py index 80beaeda4..ef2a5809b 100644 --- a/looper/cli_pydantic.py +++ b/looper/cli_pydantic.py @@ -177,7 +177,9 @@ def run_looper(args: TopLevelParser, parser: ArgumentParser, test_args=None): subcommand_args.ignore_flags = True # Initialize project - if is_PEP_file_type(subcommand_args.config_file) and os.path.exists(subcommand_args.config_file): + if is_PEP_file_type(subcommand_args.config_file) and os.path.exists( + subcommand_args.config_file + ): try: p = Project( cfg=subcommand_args.config_file, @@ -217,7 +219,6 @@ def run_looper(args: TopLevelParser, parser: ArgumentParser, test_args=None): f"Cannot load PEP. Check file path or registry path to pep." ) - selected_compute_pkg = p.selected_compute_package or DEFAULT_COMPUTE_RESOURCES_NAME if p.dcc is not None and not p.dcc.activate_package(selected_compute_pkg): _LOGGER.info( diff --git a/tests/smoketests/test_other.py b/tests/smoketests/test_other.py index 1ad41bbbd..2527f4f25 100644 --- a/tests/smoketests/test_other.py +++ b/tests/smoketests/test_other.py @@ -3,8 +3,7 @@ import pytest from peppy import Project -from looper.const import FLAGS -from looper.exceptions import PipestatConfigurationException +from looper.exceptions import PipestatConfigurationException, MisconfigurationException from tests.conftest import * from looper.cli_pydantic import main import pandas as pd @@ -607,5 +606,5 @@ def test_inspect_config(self, prep_temp_pep, cmd): def test_inspect_no_config_found(self, cmd): "Checks inspect command" x = [cmd] - with pytest.raises(ValueError): + with pytest.raises(MisconfigurationException): results = main(test_args=x) diff --git a/tests/smoketests/test_run.py b/tests/smoketests/test_run.py index fc8650fd0..4c865dee9 100644 --- a/tests/smoketests/test_run.py +++ b/tests/smoketests/test_run.py @@ -74,7 +74,7 @@ def test_looper_cfg_required(self, cmd): x = test_args_expansion("", cmd) - with pytest.raises(ValueError): + with pytest.raises(MisconfigurationException): ff = main(test_args=x) print(ff) From a3833919993fc9672d5bd6611de5e7ea683c5563 Mon Sep 17 00:00:00 2001 From: Donald Campbell <125581724+donaldcampbelljr@users.noreply.github.com> Date: Wed, 20 Mar 2024 09:16:37 -0400 Subject: [PATCH 7/8] add basic pephub test --- tests/conftest.py | 19 +++++++++++++++++++ tests/test_comprehensive.py | 13 +++++++++++++ 2 files changed, 32 insertions(+) diff --git a/tests/conftest.py b/tests/conftest.py index 0293bd807..294c23ec5 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -305,3 +305,22 @@ def prep_temp_pep_pipestat_advanced(example_pep_piface_path): path_to_looper_config = os.path.join(advanced_dir, ".looper_advanced_pipestat.yaml") return path_to_looper_config + + +@pytest.fixture +def prep_temp_pep_pephub(example_pep_piface_path): + + # Get Path to local copy of hello_looper + + hello_looper_dir_path = os.path.join( + example_pep_piface_path, "hello_looper-dev_derive" + ) + + # Make local temp copy of hello_looper + d = tempfile.mkdtemp() + shutil.copytree(hello_looper_dir_path, d, dirs_exist_ok=True) + + advanced_dir = os.path.join(d, "pephub") + path_to_looper_config = os.path.join(advanced_dir, ".looper.yaml") + + return path_to_looper_config diff --git a/tests/test_comprehensive.py b/tests/test_comprehensive.py index a40a0f41d..421e6736e 100644 --- a/tests/test_comprehensive.py +++ b/tests/test_comprehensive.py @@ -146,3 +146,16 @@ def test_comprehensive_looper_pipestat(prep_temp_pep_pipestat): assert len(tsv_list) == 0 with pytest.raises(RecordNotFoundError): retrieved_result = psm.retrieve_one(record_identifier="frog_2") + + +def test_comprehensive_looper_pephub(prep_temp_pep_pephub): + """Basic test to determine if Looper can run a PEP from PEPHub""" + + path_to_looper_config = prep_temp_pep_pephub + + x = ["run", "--looper-config", path_to_looper_config] + + try: + results = main(test_args=x) + except Exception: + raise pytest.fail("DID RAISE {0}".format(Exception)) From fad869f8984beccb207871429b66360d25aedd7c Mon Sep 17 00:00:00 2001 From: Donald Campbell <125581724+donaldcampbelljr@users.noreply.github.com> Date: Wed, 20 Mar 2024 09:55:09 -0400 Subject: [PATCH 8/8] refactor remove unnecessary tests --- looper/cli_pydantic.py | 4 ++-- looper/utils.py | 27 +++++++++++++++------------ tests/smoketests/test_run.py | 15 +-------------- 3 files changed, 18 insertions(+), 28 deletions(-) diff --git a/looper/cli_pydantic.py b/looper/cli_pydantic.py index ef2a5809b..f9c510098 100644 --- a/looper/cli_pydantic.py +++ b/looper/cli_pydantic.py @@ -40,7 +40,7 @@ from .utils import ( dotfile_path, enrich_args_via_cfg, - is_registry_path, + is_pephub_registry_path, read_looper_config_file, read_looper_dotfile, initiate_looper_config, @@ -195,7 +195,7 @@ def run_looper(args: TopLevelParser, parser: ArgumentParser, test_args=None): except yaml.parser.ParserError as e: _LOGGER.error(f"Project config parse failed -- {e}") sys.exit(1) - elif is_registry_path(subcommand_args.config_file): + elif is_pephub_registry_path(subcommand_args.config_file): if vars(subcommand_args)[SAMPLE_PL_ARG]: p = Project( amendments=subcommand_args.amend, diff --git a/looper/utils.py b/looper/utils.py index fe2e78175..849395454 100644 --- a/looper/utils.py +++ b/looper/utils.py @@ -472,7 +472,7 @@ def initiate_looper_config( return False if pep_path: - if is_registry_path(pep_path): + if is_pephub_registry_path(pep_path): pass else: pep_path = expandpath(pep_path) @@ -562,10 +562,20 @@ def read_looper_config_file(looper_config_path: str) -> dict: for k, v in return_dict.items(): if isinstance(v, str): v = expandpath(v) - if not os.path.isabs(v) and not is_registry_path(v): - return_dict[k] = os.path.join(config_dir_path, v) - else: + # TODO this is messy because is_pephub_registry needs to fail on anything NOT a pephub registry path + # https://github.com/pepkit/ubiquerg/issues/43 + if is_PEP_file_type(v): + if not os.path.isabs(v): + return_dict[k] = os.path.join(config_dir_path, v) + else: + return_dict[k] = v + elif is_pephub_registry_path(v): return_dict[k] = v + else: + if not os.path.isabs(v): + return_dict[k] = os.path.join(config_dir_path, v) + else: + return_dict[k] = v return return_dict @@ -609,19 +619,12 @@ def is_PEP_file_type(input_string: str) -> bool: return res -def is_registry_path(input_string: str) -> bool: +def is_pephub_registry_path(input_string: str) -> bool: """ Check if input is a registry path to pephub :param str input_string: path to the PEP (or registry path) :return bool: True if input is a registry path """ - try: - if input_string.endswith(".yaml") or input_string.endswith(".csv"): - return False - except AttributeError: - raise RegistryPathException( - msg=f"Malformed registry path. Unable to parse {input_string} as a registry path." - ) try: registry_path = RegistryPath(**parse_registry_path(input_string)) except (ValidationError, TypeError): diff --git a/tests/smoketests/test_run.py b/tests/smoketests/test_run.py index 4c865dee9..48c7acb96 100644 --- a/tests/smoketests/test_run.py +++ b/tests/smoketests/test_run.py @@ -594,20 +594,7 @@ class TestLooperPEPhub: ], ) def test_pephub_registry_path_recognition(self, pep_path): - assert is_registry_path(pep_path) is True - - @pytest.mark.parametrize( - "pep_path", - [ - "some/path/to/pep.yaml", - "different/path.yaml", - "default/path/to/file/without/yaml", - "file_in_folder.yaml", - "not_yaml_file", - ], - ) - def test_config_recognition(self, pep_path): - assert is_registry_path(pep_path) is False + assert is_pephub_registry_path(pep_path) is True def test_init_project_using_dict(self, prep_temp_config_with_pep): """Verify looper runs using pephub in a basic case and return code is 0"""