From b05f036b256e610eb7a293e752512272fea865fc Mon Sep 17 00:00:00 2001 From: Dorota Jarecka Date: Tue, 6 Jun 2023 15:54:43 -0400 Subject: [PATCH 1/3] some refactoring to the unit tests, adding cases for slurm --- babs/utils.py | 8 ++-- tests/get_data.py | 20 +++++++-- tests/test_babs_check_setup.py | 67 ++++++++++++----------------- tests/test_babs_init.py | 78 +++++++++++++++++----------------- 4 files changed, 90 insertions(+), 83 deletions(-) diff --git a/babs/utils.py b/babs/utils.py index b8f3ba2b..a4022cf2 100644 --- a/babs/utils.py +++ b/babs/utils.py @@ -16,6 +16,9 @@ from datetime import datetime import re + +TYPE_SYSTEM_SUPPORTED = ['sge', 'slurm'] + # Disable the behavior of printing messages: def blockPrint(): sys.stdout = open(os.devnull, 'w') @@ -155,13 +158,12 @@ def validate_type_system(type_system): For valid ones, the type string will be changed to lower case. If not valid, raise error message. """ - list_supported = ['sge', 'slurm'] - if type_system.lower() in list_supported: + if type_system.lower() in TYPE_SYSTEM_SUPPORTED: type_system = type_system.lower() # change to lower case, if needed else: raise Exception("Invalid cluster system type: '" + type_system + "'!" + " Currently BABS only support one of these: " - + ', '.join(list_supported)) # names separated by ', ' + + ', '.join(TYPE_SYSTEM_SUPPORTED)) # names separated by ', ' return type_system diff --git a/tests/get_data.py b/tests/get_data.py index e0551404..3c30775b 100644 --- a/tests/get_data.py +++ b/tests/get_data.py @@ -107,9 +107,12 @@ def where_now(if_circleci): elif if_singularity_installed: where_now = "on_cluster" elif if_docker_installed: - where_now = "on_local" + if subprocess.call(["docker", "info"]) == 0: + where_now = "on_local" + else: + raise Exception("Docker is installed, but not running!" + + " Pytest cannot proceed.") else: - where_now = "" raise Exception("Not on CircleCI, and neither singularity nor docker is installed!" + " Pytest cannot proceed.") return where_now @@ -192,6 +195,11 @@ def container_ds_path(where_now, tmp_path_factory): url="dhub://"+docker_addr # e.g., "dhub://pennlinc/toy_bids_app:0.0.7" ) + # Container dataset - has been set up by fixture `prep_container_ds_toybidsapp()` + # TODO: when this could happen??? perhaps this should be also as pytest.skipif()? + assert op.exists(origin_container_ds) + assert op.exists(op.join(origin_container_ds, ".datalad/config")) + return origin_container_ds def get_container_config_yaml_filename(which_bidsapp, @@ -245,7 +253,13 @@ def get_container_config_yaml_filename(which_bidsapp, container_config_yaml_filename += "_" + type_system + "_" \ + dict_cluster_name[type_system] + ".yaml" - return container_config_yaml_filename + notebooks_dir = op.join(op.dirname(__location__), "notebooks") + container_config_yaml_file = op.join(notebooks_dir, container_config_yaml_filename) + + if not op.exists(container_config_yaml_file): + pytest.skip(f"container config yaml file doesn't exist in {notebooks_dir}") + + return container_config_yaml_file def if_command_installed(cmd): diff --git a/tests/test_babs_check_setup.py b/tests/test_babs_check_setup.py index 5799434a..258638bb 100644 --- a/tests/test_babs_check_setup.py +++ b/tests/test_babs_check_setup.py @@ -26,15 +26,21 @@ @pytest.mark.order(index=2) @pytest.mark.parametrize( - "which_case", - [("not_to_keep_failed"), - ("wrong_container_ds"), - ("wrong_input_ds")] + "which_case, error_type, error_msg",[ + # container_ds` has wrong path; not to `--keep-if-failed` + ("not_to_keep_failed", Exception, "`--project-root` does not exist!"), + # container_ds` has wrong path #todo: perhaps it should be changed to RuntimeError? + ("wrong_container_ds", AssertionError, "There is no containers DataLad dataset in folder:"), + # `input ds` has wrong path + ("wrong_input_ds", FileNotFoundError, "No such file or directory:") + ] ) def test_babs_check_setup( - which_case, + which_case, error_type, error_msg, tmp_path, tmp_path_factory, - container_ds_path, if_circleci): + container_ds_path, if_circleci, + type_system="sge" +): """ This is to test `babs-check-setup` in different failed `babs-init` cases. Successful `babs-init` has been tested in `test_babs_init.py`. @@ -53,6 +59,8 @@ def test_babs_check_setup( tmp_path_factory: fixture from pytest container_ds_path: fixture; str Path to the container datalad dataset + type_system: str + the type of job scheduling system, "sge" or "slurm" (shouldn't make a difference here) """ # fixed variables: which_bidsapp = "toybidsapp" @@ -64,33 +72,25 @@ def test_babs_check_setup( # Get the path to input dataset: path_in = get_input_data(which_input, type_session, if_input_local, tmp_path_factory) input_ds_cli = [[which_input, path_in]] - input_ds_cli_wrong = [[which_input, "/random/path/to/input_ds"]] - - # Container dataset - has been set up by fixture `prep_container_ds_toybidsapp()` - assert op.exists(container_ds_path) - assert op.exists(op.join(container_ds_path, ".datalad/config")) - container_ds_path_wrong = "/random/path/to/container_ds" + # TODO: should we just point to a dir in tmp_path? the assert should not be needed # Preparation of env variable `TEMPLATEFLOW_HOME`: os.environ["TEMPLATEFLOW_HOME"] = TEMPLATEFLOW_HOME assert os.getenv('TEMPLATEFLOW_HOME') is not None # assert env var has been set # Get the cli of `babs-init`: - where_project = tmp_path.absolute().as_posix() # turn into a string project_name = "my_babs_project" - project_root = op.join(where_project, project_name) + project_root = str(tmp_path / project_name) container_name = which_bidsapp + "-" + TOYBIDSAPP_VERSION_DASH - container_config_yaml_filename = "example_container_" + which_bidsapp + ".yaml" - container_config_yaml_filename = \ + + + container_config_yaml_file = \ get_container_config_yaml_filename(which_bidsapp, which_input, if_two_input=False, - type_system="sge") # TODO: also test slurm! - container_config_yaml_file = op.join(op.dirname(__location__), "notebooks", - container_config_yaml_filename) - assert op.exists(container_config_yaml_file) + type_system=type_system) # below are all correct options: babs_init_opts = argparse.Namespace( - where_project=where_project, + where_project=str(tmp_path), project_name=project_name, input=input_ds_cli, list_sub_file=None, @@ -98,22 +98,23 @@ def test_babs_check_setup( container_name=container_name, container_config_yaml_file=container_config_yaml_file, type_session=type_session, - type_system="sge", + type_system=type_system, keep_if_failed=True ) # inject something wrong --> `babs-init` will fail: - babs_init_opts.container_ds = container_ds_path_wrong - # `--keep-if-failed`: - if which_case == "not_to_keep_failed": - babs_init_opts.keep_if_failed = False - # each case, what went wrong: + container_ds_path_wrong = "/random/path/to/container_ds" + input_ds_cli_wrong = [[which_input, "/random/path/to/input_ds"]] if which_case == "not_to_keep_failed": babs_init_opts.container_ds = container_ds_path_wrong + # `--keep-if-failed`: + babs_init_opts.keep_if_failed = False elif which_case == "wrong_container_ds": babs_init_opts.container_ds = container_ds_path_wrong elif which_case == "wrong_input_ds": babs_init_opts.input = input_ds_cli_wrong + else: + pytest.skip("No rules provided for this case!") # run `babs-init`: with mock.patch.object( @@ -125,18 +126,6 @@ def test_babs_check_setup( project_root=project_root, job_test=False ) - # Set up expected error message from `babs-check-setup`: - if which_case == "not_to_keep_failed": - error_type = Exception # what's after `raise` in the source code - error_msg = "`--project-root` does not exist!" - # ^^ see `get_existing_babs_proj()` in CLI - elif which_case == "wrong_container_ds": - error_type = AssertionError # error from `assert` - error_msg = "There is no containers DataLad dataset in folder:" - elif which_case == "wrong_input_ds": - error_type = FileNotFoundError - error_msg = "No such file or directory:" - # ^^ No such file or directory: '/path/to/my_babs_project/analysis/inputs/data' # Run `babs-check-setup`: with mock.patch.object( diff --git a/tests/test_babs_init.py b/tests/test_babs_init.py index b56643ab..b73c17f7 100644 --- a/tests/test_babs_init.py +++ b/tests/test_babs_init.py @@ -7,7 +7,7 @@ from unittest import mock sys.path.append("..") sys.path.append("../babs") -from babs.utils import (read_yaml) # noqa +from babs.utils import (read_yaml, TYPE_SYSTEM_SUPPORTED) # noqa from babs.cli import ( # noqa babs_init_main, babs_check_setup_main) @@ -25,11 +25,13 @@ ) @pytest.mark.order(index=1) +@pytest.mark.parametrize("type_system", ["sge", "slurm"]) @pytest.mark.parametrize( - "which_bidsapp,which_input,type_session,if_input_local,if_two_input", + "which_bidsapp, which_input, type_session, if_input_local, if_two_input", # test toybidsapp: BIDS/zipped x single/multi-ses: # the input data will also be remote by default: - [("toybidsapp", "BIDS", "single-ses", False, False), + [ + ("toybidsapp", "BIDS", "single-ses", False, False), ("toybidsapp", "BIDS", "multi-ses", False, False), ("toybidsapp", "fmriprep", "single-ses", False, False), ("toybidsapp", "fmriprep", "multi-ses", False, False), @@ -44,9 +46,11 @@ ("fmriprep", "BIDS", "single-ses", False, True), ("fmriprep", "BIDS", "multi-ses", False, True), ]) -def test_babs_init(which_bidsapp, which_input, type_session, if_input_local, if_two_input, +def test_babs_init(which_bidsapp, which_input, type_session, + if_input_local, if_two_input, tmp_path, tmp_path_factory, container_ds_path, if_circleci, + type_system ): """ This is to test `babs-init` in different cases. @@ -72,11 +76,14 @@ def test_babs_init(which_bidsapp, which_input, type_session, if_input_local, if_ Path to the container datalad dataset if_circleci: fixture; bool Whether currently in CircleCI - - TODO: add `type_system` and to test out Slurm version! + type_system: str + the type of job scheduling system, "sge" or "slurm" """ # Sanity checks: - assert which_bidsapp in LIST_WHICH_BIDSAPP + if which_bidsapp not in LIST_WHICH_BIDSAPP: + pytest.skip(f"which_bidsapp must be one of {LIST_WHICH_BIDSAPP}, test can't be run") + if type_system not in TYPE_SYSTEM_SUPPORTED: + pytest.skip(f"type_system must be one of {TYPE_SYSTEM_SUPPORTED}, test can't be run") # Get the path to input dataset: path_in = get_input_data(which_input, type_session, if_input_local, tmp_path_factory) @@ -91,20 +98,14 @@ def test_babs_init(which_bidsapp, which_input, type_session, if_input_local, if_ tmp_path_factory) input_ds_cli.append([INFO_2ND_INPUT_DATA["which_input"], path_in_2nd]) - # Container dataset - has been set up by fixture `prep_container_ds_toybidsapp()` - assert op.exists(container_ds_path) - assert op.exists(op.join(container_ds_path, ".datalad/config")) # Preparation of freesurfer: for fmriprep and qsiprep: - # check if `--fs-license-file` is included in YAML file: - container_config_yaml_filename = \ + container_config_yaml_file = \ get_container_config_yaml_filename(which_bidsapp, which_input, if_two_input, - type_system="sge") # TODO: also test slurm! - container_config_yaml_file = op.join(op.dirname(__location__), "notebooks", - container_config_yaml_filename) - assert op.exists(container_config_yaml_file) + type_system=type_system) container_config_yaml = read_yaml(container_config_yaml_file) + # check if `--fs-license-file` is included in YAML file: if "--fs-license-file" in container_config_yaml["singularity_run"]: # ^^ this way is consistent with BABS re: how to determine if fs license is needed; flag_requested_fs_license = True @@ -112,7 +113,7 @@ def test_babs_init(which_bidsapp, which_input, type_session, if_input_local, if_ else: flag_requested_fs_license = False str_fs_license_file = "" - + # TODO: should we just point to a dir in tmp_path? the assert should not be needed # Preparation of env variable `TEMPLATEFLOW_HOME`: os.environ["TEMPLATEFLOW_HOME"] = TEMPLATEFLOW_HOME assert os.getenv('TEMPLATEFLOW_HOME') is not None # assert env var has been set @@ -133,7 +134,7 @@ def test_babs_init(which_bidsapp, which_input, type_session, if_input_local, if_ container_name=container_name, container_config_yaml_file=container_config_yaml_file, type_session=type_session, - type_system="sge", + type_system=type_system, keep_if_failed=False ) @@ -162,11 +163,11 @@ def test_babs_init(which_bidsapp, which_input, type_session, if_input_local, if_ # 3) for freesurfer: flag `--fs-license-file` # first, read in `-0-0-0_zip.sh`: - fn_bash_container_zip = op.join(project_root, "analysis/code", container_name + "_zip.sh") - file_bash_container_zip = open(fn_bash_container_zip, 'r') - lines_bash_container_zip = file_bash_container_zip.readlines() - file_bash_container_zip.close() - # check: + with open(op.join(project_root, "analysis/code", container_name + "_zip.sh")) as f: + bash_container_zip = f.read() + + # checking the content of files created by BABS init + # setting initial values for flags: if_bind_templateflow = False # `singularity run -B` to bind a path to container if_bind_freesurfer = False str_bind_freesurfer = "-B " + str_fs_license_file \ @@ -178,21 +179,22 @@ def test_babs_init(which_bidsapp, which_input, type_session, if_input_local, if_ if_flag_bidsfilterfile = False if_flag_fs_license = False flag_fs_license = '--fs-license-file /SGLR/FREESURFER_HOME/license.txt' - for line in lines_bash_container_zip: - if "--env TEMPLATEFLOW_HOME=/SGLR/TEMPLATEFLOW_HOME" in line: - if_set_singu_templateflow = True - if all(ele in line for ele in ["-B", - TEMPLATEFLOW_HOME + ":/SGLR/TEMPLATEFLOW_HOME"]): - # e.g., `-B /test/templateflow_home:/SGLR/TEMPLATEFLOW_HOME \` - if_bind_templateflow = True - if str_bind_freesurfer in line: - if_bind_freesurfer = True - if "filterfile=${PWD}/${sesid}_filter.json" in line: - if_generate_bidsfilterfile = True - if '--bids-filter-file "${filterfile}"' in line: - if_flag_bidsfilterfile = True - if flag_fs_license in line: - if_flag_fs_license = True + + if "--env TEMPLATEFLOW_HOME=/SGLR/TEMPLATEFLOW_HOME" in bash_container_zip: + if_set_singu_templateflow = True + if all(ele in bash_container_zip for ele in ["-B", + TEMPLATEFLOW_HOME + ":/SGLR/TEMPLATEFLOW_HOME"]): + # e.g., `-B /test/templateflow_home:/SGLR/TEMPLATEFLOW_HOME \` + if_bind_templateflow = True + if str_bind_freesurfer in bash_container_zip: + if_bind_freesurfer = True + if "filterfile=${PWD}/${sesid}_filter.json" in bash_container_zip: + if_generate_bidsfilterfile = True + if '--bids-filter-file "${filterfile}"' in bash_container_zip: + if_flag_bidsfilterfile = True + if flag_fs_license in bash_container_zip: + if_flag_fs_license = True + # assert they are found: # 1) TemplateFlow: should be found in all cases: assert if_bind_templateflow, \ From fe28f0aac57273c65eb7e1e51572c27d3fdb2c77 Mon Sep 17 00:00:00 2001 From: Dorota Jarecka Date: Fri, 16 Jun 2023 13:09:04 -0400 Subject: [PATCH 2/3] adding chenying suggestions --- babs/constants.py | 1 + babs/utils.py | 2 +- tests/get_data.py | 1 - tests/test_babs_check_setup.py | 4 ++++ tests/test_babs_init.py | 3 ++- 5 files changed, 8 insertions(+), 3 deletions(-) diff --git a/babs/constants.py b/babs/constants.py index 451ab9f6..650cf473 100644 --- a/babs/constants.py +++ b/babs/constants.py @@ -1,3 +1,4 @@ MSG_NO_ALERT_IN_LOGS = "BABS: No alert message found in log files." CHECK_MARK = u'\N{check mark}' # can be used by print(CHECK_MARK) PATH_FS_LICENSE_IN_CONTAINER = "/SGLR/FREESURFER_HOME/license.txt" +TYPE_SYSTEM_SUPPORTED = ['sge', 'slurm'] \ No newline at end of file diff --git a/babs/utils.py b/babs/utils.py index a4022cf2..b6ef9f38 100644 --- a/babs/utils.py +++ b/babs/utils.py @@ -16,8 +16,8 @@ from datetime import datetime import re +from babs.constants import TYPE_SYSTEM_SUPPORTED -TYPE_SYSTEM_SUPPORTED = ['sge', 'slurm'] # Disable the behavior of printing messages: def blockPrint(): diff --git a/tests/get_data.py b/tests/get_data.py index 3c30775b..dc5d1ddb 100644 --- a/tests/get_data.py +++ b/tests/get_data.py @@ -196,7 +196,6 @@ def container_ds_path(where_now, tmp_path_factory): ) # Container dataset - has been set up by fixture `prep_container_ds_toybidsapp()` - # TODO: when this could happen??? perhaps this should be also as pytest.skipif()? assert op.exists(origin_container_ds) assert op.exists(op.join(origin_container_ds, ".datalad/config")) diff --git a/tests/test_babs_check_setup.py b/tests/test_babs_check_setup.py index 258638bb..2c16df89 100644 --- a/tests/test_babs_check_setup.py +++ b/tests/test_babs_check_setup.py @@ -55,6 +55,10 @@ def test_babs_check_setup( All cases have something going wrong, leading to `babs-init` failure; Only in case `not_to_keep_failed`, flag `--keep-if-failed` in `babs-init` won't turn on, so expected error will be: BABS project does not exist. + error_type: Exception + The type of expected error raised by `babs-check-setup` + error_msg: str + The expected error message raised by `babs-check-setup` tmp_path: fixture from pytest tmp_path_factory: fixture from pytest container_ds_path: fixture; str diff --git a/tests/test_babs_init.py b/tests/test_babs_init.py index b73c17f7..4588bc88 100644 --- a/tests/test_babs_init.py +++ b/tests/test_babs_init.py @@ -7,7 +7,8 @@ from unittest import mock sys.path.append("..") sys.path.append("../babs") -from babs.utils import (read_yaml, TYPE_SYSTEM_SUPPORTED) # noqa +from babs.utils import (read_yaml) # noqa +from babs.constants import (TYPE_SYSTEM_SUPPORTED) # noqa from babs.cli import ( # noqa babs_init_main, babs_check_setup_main) From 0269e18882fd81236bd98e021605bc101ea4e75b Mon Sep 17 00:00:00 2001 From: Dorota Jarecka Date: Sun, 25 Jun 2023 22:48:51 -0400 Subject: [PATCH 3/3] removing some todos --- tests/test_babs_check_setup.py | 3 +-- tests/test_babs_init.py | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/test_babs_check_setup.py b/tests/test_babs_check_setup.py index 2c16df89..7e95346b 100644 --- a/tests/test_babs_check_setup.py +++ b/tests/test_babs_check_setup.py @@ -29,7 +29,7 @@ "which_case, error_type, error_msg",[ # container_ds` has wrong path; not to `--keep-if-failed` ("not_to_keep_failed", Exception, "`--project-root` does not exist!"), - # container_ds` has wrong path #todo: perhaps it should be changed to RuntimeError? + # container_ds` has wrong path ("wrong_container_ds", AssertionError, "There is no containers DataLad dataset in folder:"), # `input ds` has wrong path ("wrong_input_ds", FileNotFoundError, "No such file or directory:") @@ -77,7 +77,6 @@ def test_babs_check_setup( path_in = get_input_data(which_input, type_session, if_input_local, tmp_path_factory) input_ds_cli = [[which_input, path_in]] - # TODO: should we just point to a dir in tmp_path? the assert should not be needed # Preparation of env variable `TEMPLATEFLOW_HOME`: os.environ["TEMPLATEFLOW_HOME"] = TEMPLATEFLOW_HOME assert os.getenv('TEMPLATEFLOW_HOME') is not None # assert env var has been set diff --git a/tests/test_babs_init.py b/tests/test_babs_init.py index 4588bc88..45bf4ee8 100644 --- a/tests/test_babs_init.py +++ b/tests/test_babs_init.py @@ -114,7 +114,7 @@ def test_babs_init(which_bidsapp, which_input, type_session, else: flag_requested_fs_license = False str_fs_license_file = "" - # TODO: should we just point to a dir in tmp_path? the assert should not be needed + # Preparation of env variable `TEMPLATEFLOW_HOME`: os.environ["TEMPLATEFLOW_HOME"] = TEMPLATEFLOW_HOME assert os.getenv('TEMPLATEFLOW_HOME') is not None # assert env var has been set