From 060da32b852a7d317185eb85d7cd5e0d507ccc69 Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Tue, 3 Dec 2024 01:27:45 -0500 Subject: [PATCH 01/22] update type hint --- bagel/mappings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bagel/mappings.py b/bagel/mappings.py index 5bb3f4c..bd45b4a 100644 --- a/bagel/mappings.py +++ b/bagel/mappings.py @@ -73,7 +73,7 @@ def get_pipeline_catalog(url: str, path: Path) -> list[dict]: ) from e -def parse_pipeline_catalog(): +def parse_pipeline_catalog() -> tuple[dict, dict]: """ Load the pipeline catalog and return a dictionary of pipeline names and their URIs in the Nipoppy namespace, and a dictionary of pipeline names and their supported versions in Nipoppy. From 0cf56d883b67bab68d87a18b0d8e5fa10b3b1f9c Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Tue, 3 Dec 2024 12:21:11 -0500 Subject: [PATCH 02/22] refactor utils to include check if all pipeline names/vers are unrecognized --- bagel/cli.py | 14 +--- bagel/utilities/derivative_utils.py | 101 ++++++++++++++++++++++------ 2 files changed, 83 insertions(+), 32 deletions(-) diff --git a/bagel/cli.py b/bagel/cli.py index b793c7d..d017c85 100644 --- a/bagel/cli.py +++ b/bagel/cli.py @@ -414,18 +414,10 @@ def derivatives( f"We found missing values in the following rows (first row is zero): {row_indices}." ) - pipelines = status_df[PROC_STATUS_COLS["pipeline_name"]].unique() - derivative_utils.check_pipelines_are_recognized(pipelines) - # TODO: Do we need to check all versions across all pipelines first, and report all unrecognized versions together? - for pipeline in pipelines: - versions = status_df[ - status_df[PROC_STATUS_COLS["pipeline_name"]] == pipeline - ][PROC_STATUS_COLS["pipeline_version"]].unique() - - derivative_utils.check_pipeline_versions_are_recognized( - pipeline, versions - ) + derivative_utils.check_at_least_one_pipeline_version_is_recognized( + status_df=status_df + ) jsonld_dataset = model_utils.extract_and_validate_jsonld_dataset( jsonld_path diff --git a/bagel/utilities/derivative_utils.py b/bagel/utilities/derivative_utils.py index 8badbd0..d595432 100644 --- a/bagel/utilities/derivative_utils.py +++ b/bagel/utilities/derivative_utils.py @@ -1,3 +1,4 @@ +import warnings from typing import Iterable import pandas as pd @@ -17,35 +18,89 @@ } -def check_pipelines_are_recognized(pipelines: Iterable[str]): +# TODO: Rename +def check_pipelines_are_recognized(pipelines: Iterable[str]) -> list: """Check that all pipelines in the processing status file are supported by Nipoppy.""" + allowed_pipelines_message = ( + f"Allowed pipeline names are the following pipelines supported natively in Nipoppy (https://github.com/nipoppy/pipeline-catalog):\n" + f"{mappings.KNOWN_PIPELINE_URIS}" + ) + recognized_pipelines = list( + set(pipelines).intersection(mappings.KNOWN_PIPELINE_URIS) + ) unrecognized_pipelines = list( set(pipelines).difference(mappings.KNOWN_PIPELINE_URIS) ) - if len(unrecognized_pipelines) > 0: + + if len(recognized_pipelines) == 0: raise LookupError( + f"The processing status file contains no recognized pipelines in the column '{PROC_STATUS_COLS['pipeline_name']}'.\n" + f"{allowed_pipelines_message}" + ) + if len(unrecognized_pipelines) > 0: + warnings.warn( f"The processing status file contains unrecognized pipelines in the column '{PROC_STATUS_COLS['pipeline_name']}': " - f"{unrecognized_pipelines}. " - f"Allowed pipeline names are the following pipelines supported natively in Nipoppy (https://github.com/nipoppy/pipeline-catalog): \n" - f"{mappings.KNOWN_PIPELINE_URIS}" + f"{unrecognized_pipelines}. These will be ignored.\n" + f"{allowed_pipelines_message}" ) + return recognized_pipelines +# TODO: Rename def check_pipeline_versions_are_recognized( pipeline: str, versions: Iterable[str] -): +) -> tuple[list, list]: """ Check that all pipeline versions in the processing status file are supported by Nipoppy. Assumes that the input pipeline name is recognized. """ + recognized_versions = list( + set(versions).intersection(mappings.KNOWN_PIPELINE_VERSIONS[pipeline]) + ) unrecognized_versions = list( set(versions).difference(mappings.KNOWN_PIPELINE_VERSIONS[pipeline]) ) - if len(unrecognized_versions) > 0: + + return recognized_versions, unrecognized_versions + + +def check_at_least_one_pipeline_version_is_recognized(status_df: pd.DataFrame): + more_info_message = ( + "Allowed processing pipelines and versions are those supported natively in Nipoppy. " + "For a full list, see https://github.com/nipoppy/pipeline-catalog." + ) + + # TODO: Handle error in this func here as well? + recognized_pipelines = check_pipelines_are_recognized( + status_df[PROC_STATUS_COLS["pipeline_name"]].unique() + ) + + total_num_recognized_versions = 0 + unrecognized_pipeline_versions = {} + for pipeline in recognized_pipelines: + versions = status_df[ + status_df[PROC_STATUS_COLS["pipeline_name"]] == pipeline + ][PROC_STATUS_COLS["pipeline_version"]].unique() + + recognized_versions, unrecognized_versions = ( + check_pipeline_versions_are_recognized(pipeline, versions) + ) + + total_num_recognized_versions += len(recognized_versions) + if len(unrecognized_versions) > 0: + unrecognized_pipeline_versions[pipeline] = unrecognized_versions + + if total_num_recognized_versions == 0: + # TODO: Consider simply exiting with a message and no output instead? raise LookupError( - f"The processing status file contains unrecognized {pipeline} versions in the column '{PROC_STATUS_COLS['pipeline_version']}': {unrecognized_versions}. " - f"Allowed {pipeline} versions are the following versions supported natively in Nipoppy (https://github.com/nipoppy/pipeline-catalog): \n" - f"{mappings.KNOWN_PIPELINE_VERSIONS[pipeline]}" + f"The processing status file contains no recognized versions of any pipelines in the column '{PROC_STATUS_COLS['pipeline_version']}'.\n" + f"{more_info_message}" + ) + if len(unrecognized_pipeline_versions) > 0: + warnings.warn( + f"The processing status file contains unrecognized versions of the following pipelines in the column '{PROC_STATUS_COLS['pipeline_version']}': " + f"{unrecognized_pipeline_versions}. These will be ignored.\n" + f"{more_info_message}" ) @@ -61,17 +116,21 @@ def create_completed_pipelines(session_proc_df: pd.DataFrame) -> list: PROC_STATUS_COLS["pipeline_version"], ] ): - # Check that all pipeline steps have succeeded if ( - session_pipe_df[PROC_STATUS_COLS["status"]].str.lower() - == "success" - ).all(): - completed_pipeline = models.CompletedPipeline( - hasPipelineName=models.Pipeline( - identifier=mappings.KNOWN_PIPELINE_URIS[pipeline] - ), - hasPipelineVersion=version, - ) - completed_pipelines.append(completed_pipeline) + pipeline in mappings.KNOWN_PIPELINE_URIS + and version in mappings.KNOWN_PIPELINE_VERSIONS[pipeline] + ): + # Check that all pipeline steps have succeeded + if ( + session_pipe_df[PROC_STATUS_COLS["status"]].str.lower() + == "success" + ).all(): + completed_pipeline = models.CompletedPipeline( + hasPipelineName=models.Pipeline( + identifier=mappings.KNOWN_PIPELINE_URIS[pipeline] + ), + hasPipelineVersion=version, + ) + completed_pipelines.append(completed_pipeline) return completed_pipelines From 90987b6e701b8de06ea92d14e69135080d8dbc06 Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Tue, 3 Dec 2024 12:41:36 -0500 Subject: [PATCH 03/22] rename utility functions --- bagel/cli.py | 1 - bagel/utilities/derivative_utils.py | 19 ++++++++++--------- tests/unit/test_derivative_utils.py | 4 ++-- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/bagel/cli.py b/bagel/cli.py index d017c85..8c6e583 100644 --- a/bagel/cli.py +++ b/bagel/cli.py @@ -414,7 +414,6 @@ def derivatives( f"We found missing values in the following rows (first row is zero): {row_indices}." ) - # TODO: Do we need to check all versions across all pipelines first, and report all unrecognized versions together? derivative_utils.check_at_least_one_pipeline_version_is_recognized( status_df=status_df ) diff --git a/bagel/utilities/derivative_utils.py b/bagel/utilities/derivative_utils.py index d595432..eabdc16 100644 --- a/bagel/utilities/derivative_utils.py +++ b/bagel/utilities/derivative_utils.py @@ -18,8 +18,7 @@ } -# TODO: Rename -def check_pipelines_are_recognized(pipelines: Iterable[str]) -> list: +def get_recognized_pipelines(pipelines: Iterable[str]) -> list: """Check that all pipelines in the processing status file are supported by Nipoppy.""" allowed_pipelines_message = ( f"Allowed pipeline names are the following pipelines supported natively in Nipoppy (https://github.com/nipoppy/pipeline-catalog):\n" @@ -46,8 +45,7 @@ def check_pipelines_are_recognized(pipelines: Iterable[str]) -> list: return recognized_pipelines -# TODO: Rename -def check_pipeline_versions_are_recognized( +def classify_pipeline_versions( pipeline: str, versions: Iterable[str] ) -> tuple[list, list]: """ @@ -65,17 +63,20 @@ def check_pipeline_versions_are_recognized( def check_at_least_one_pipeline_version_is_recognized(status_df: pd.DataFrame): + """ + Check that at least one pipeline name and version combination found in the processing status file is supported by Nipoppy. + """ more_info_message = ( "Allowed processing pipelines and versions are those supported natively in Nipoppy. " "For a full list, see https://github.com/nipoppy/pipeline-catalog." ) # TODO: Handle error in this func here as well? - recognized_pipelines = check_pipelines_are_recognized( + recognized_pipelines = get_recognized_pipelines( status_df[PROC_STATUS_COLS["pipeline_name"]].unique() ) - total_num_recognized_versions = 0 + total_recognized_versions = 0 unrecognized_pipeline_versions = {} for pipeline in recognized_pipelines: versions = status_df[ @@ -83,14 +84,14 @@ def check_at_least_one_pipeline_version_is_recognized(status_df: pd.DataFrame): ][PROC_STATUS_COLS["pipeline_version"]].unique() recognized_versions, unrecognized_versions = ( - check_pipeline_versions_are_recognized(pipeline, versions) + classify_pipeline_versions(pipeline, versions) ) - total_num_recognized_versions += len(recognized_versions) + total_recognized_versions += len(recognized_versions) if len(unrecognized_versions) > 0: unrecognized_pipeline_versions[pipeline] = unrecognized_versions - if total_num_recognized_versions == 0: + if total_recognized_versions == 0: # TODO: Consider simply exiting with a message and no output instead? raise LookupError( f"The processing status file contains no recognized versions of any pipelines in the column '{PROC_STATUS_COLS['pipeline_version']}'.\n" diff --git a/tests/unit/test_derivative_utils.py b/tests/unit/test_derivative_utils.py index 4119c2c..f4ad243 100644 --- a/tests/unit/test_derivative_utils.py +++ b/tests/unit/test_derivative_utils.py @@ -124,7 +124,7 @@ def test_pipeline_versions_are_loaded(): def test_unrecognized_pipeline_names_raise_error(pipelines, unrecog_pipelines): """Test that pipeline names not found in the pipeline catalog raise an informative error.""" with pytest.raises(LookupError) as e: - derivative_utils.check_pipelines_are_recognized(pipelines) + derivative_utils.get_recognized_pipelines(pipelines) assert all( substr in str(e.value) @@ -144,7 +144,7 @@ def test_unrecognized_pipeline_versions_raise_error( ): """Test that versions of a pipeline not found in the pipeline catalog raise an informative error.""" with pytest.raises(LookupError) as e: - derivative_utils.check_pipeline_versions_are_recognized( + derivative_utils.classify_pipeline_versions( "fmriprep", fmriprep_versions ) From b9ca0391a273490cf2d7d031d29f21bd3f27e982 Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Tue, 3 Dec 2024 13:39:44 -0500 Subject: [PATCH 04/22] update docstrings --- bagel/utilities/derivative_utils.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/bagel/utilities/derivative_utils.py b/bagel/utilities/derivative_utils.py index eabdc16..b00de5d 100644 --- a/bagel/utilities/derivative_utils.py +++ b/bagel/utilities/derivative_utils.py @@ -19,7 +19,10 @@ def get_recognized_pipelines(pipelines: Iterable[str]) -> list: - """Check that all pipelines in the processing status file are supported by Nipoppy.""" + """ + Check that all pipelines in the processing status file are supported by Nipoppy. + Raise an error if all pipelines are unrecognized, otherwise warn about unrecognized pipelines. + """ allowed_pipelines_message = ( f"Allowed pipeline names are the following pipelines supported natively in Nipoppy (https://github.com/nipoppy/pipeline-catalog):\n" f"{mappings.KNOWN_PIPELINE_URIS}" @@ -49,8 +52,8 @@ def classify_pipeline_versions( pipeline: str, versions: Iterable[str] ) -> tuple[list, list]: """ - Check that all pipeline versions in the processing status file are supported by Nipoppy. - Assumes that the input pipeline name is recognized. + For a given pipeline, return the recognized and unrecognized pipeline versions in the processing status file + based on the Nipoppy pipeline-catalog, and return both as lists. """ recognized_versions = list( set(versions).intersection(mappings.KNOWN_PIPELINE_VERSIONS[pipeline]) From ca01e9c8a0e22a09b2a6411f0b05c7ca8f0be294 Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Tue, 3 Dec 2024 14:17:06 -0500 Subject: [PATCH 05/22] update tests of unrecognized pipeline names, fix warning msg --- bagel/utilities/derivative_utils.py | 2 +- tests/unit/test_derivative_utils.py | 76 +++++++++++++++++------------ 2 files changed, 46 insertions(+), 32 deletions(-) diff --git a/bagel/utilities/derivative_utils.py b/bagel/utilities/derivative_utils.py index b00de5d..9cfc6ca 100644 --- a/bagel/utilities/derivative_utils.py +++ b/bagel/utilities/derivative_utils.py @@ -25,7 +25,7 @@ def get_recognized_pipelines(pipelines: Iterable[str]) -> list: """ allowed_pipelines_message = ( f"Allowed pipeline names are the following pipelines supported natively in Nipoppy (https://github.com/nipoppy/pipeline-catalog):\n" - f"{mappings.KNOWN_PIPELINE_URIS}" + f"{mappings.KNOWN_PIPELINE_URIS.keys()}" ) recognized_pipelines = list( set(pipelines).intersection(mappings.KNOWN_PIPELINE_URIS) diff --git a/tests/unit/test_derivative_utils.py b/tests/unit/test_derivative_utils.py index f4ad243..408f65a 100644 --- a/tests/unit/test_derivative_utils.py +++ b/tests/unit/test_derivative_utils.py @@ -114,44 +114,58 @@ def test_pipeline_versions_are_loaded(): ) -@pytest.mark.parametrize( - "pipelines, unrecog_pipelines", - [ - (["fmriprep", "pipeline1"], ["pipeline1"]), - (["pipelineA", "pipelineB"], ["pipelineA", "pipelineB"]), - ], -) -def test_unrecognized_pipeline_names_raise_error(pipelines, unrecog_pipelines): - """Test that pipeline names not found in the pipeline catalog raise an informative error.""" - with pytest.raises(LookupError) as e: - derivative_utils.get_recognized_pipelines(pipelines) +def test_warning_raised_when_some_pipeline_names_unrecognized(): + """ + Test that when a subset of pipeline names are not found in the pipeline catalog, + an informative warning is raised but the recognized pipeline names are successfully returned. + """ + pipelines = ["fmriprep", "fakepipeline1"] + + with pytest.warns(UserWarning) as w: + recognized_pipelines = derivative_utils.get_recognized_pipelines( + pipelines + ) assert all( - substr in str(e.value) - for substr in ["unrecognized pipelines"] + unrecog_pipelines + substr in str(w[0].message.args[0]) + for substr in ["unrecognized pipelines", "fakepipeline1"] ) + assert recognized_pipelines == ["fmriprep"] -@pytest.mark.parametrize( - "fmriprep_versions, unrecog_versions", - [ - (["20.2.7", "vA.B"], ["vA.B"]), - (["C.D.E", "F.G.H"], ["C.D.E", "F.G.H"]), - ], -) -def test_unrecognized_pipeline_versions_raise_error( - fmriprep_versions, unrecog_versions -): - """Test that versions of a pipeline not found in the pipeline catalog raise an informative error.""" +def test_error_raised_when_no_pipeline_names_recognized(): + """ + Test that when no provided pipeline names are found in the pipeline catalog, + an informative error is raised. + """ + pipelines = ["fakepipeline1", "fakepipeline2"] + with pytest.raises(LookupError) as e: - derivative_utils.classify_pipeline_versions( - "fmriprep", fmriprep_versions - ) + derivative_utils.get_recognized_pipelines(pipelines) - assert all( - substr in str(e.value) - for substr in ["unrecognized fmriprep versions"] + unrecog_versions - ) + assert "no recognized pipelines" in str(e.value) + + +# @pytest.mark.parametrize( +# "fmriprep_versions, unrecog_versions", +# [ +# (["20.2.7", "vA.B"], ["vA.B"]), +# (["C.D.E", "F.G.H"], ["C.D.E", "F.G.H"]), +# ], +# ) +# def test_unrecognized_pipeline_versions_raise_error( +# fmriprep_versions, unrecog_versions +# ): +# """Test that versions of a pipeline not found in the pipeline catalog raise an informative error.""" +# with pytest.raises(LookupError) as e: +# derivative_utils.classify_pipeline_versions( +# "fmriprep", fmriprep_versions +# ) + +# assert all( +# substr in str(e.value) +# for substr in ["unrecognized fmriprep versions"] + unrecog_versions +# ) def test_create_completed_pipelines(): From 3e2481c3664f91f10cbf4195bb505f671874ab2e Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Tue, 3 Dec 2024 14:29:21 -0500 Subject: [PATCH 06/22] update tests of pipeline version check --- bagel/utilities/derivative_utils.py | 2 +- tests/unit/test_derivative_utils.py | 38 ++++++++++++++--------------- 2 files changed, 19 insertions(+), 21 deletions(-) diff --git a/bagel/utilities/derivative_utils.py b/bagel/utilities/derivative_utils.py index 9cfc6ca..a08b2e5 100644 --- a/bagel/utilities/derivative_utils.py +++ b/bagel/utilities/derivative_utils.py @@ -53,7 +53,7 @@ def classify_pipeline_versions( ) -> tuple[list, list]: """ For a given pipeline, return the recognized and unrecognized pipeline versions in the processing status file - based on the Nipoppy pipeline-catalog, and return both as lists. + based on the Nipoppy pipeline catalog, and return both as lists. """ recognized_versions = list( set(versions).intersection(mappings.KNOWN_PIPELINE_VERSIONS[pipeline]) diff --git a/tests/unit/test_derivative_utils.py b/tests/unit/test_derivative_utils.py index 408f65a..47df375 100644 --- a/tests/unit/test_derivative_utils.py +++ b/tests/unit/test_derivative_utils.py @@ -146,26 +146,24 @@ def test_error_raised_when_no_pipeline_names_recognized(): assert "no recognized pipelines" in str(e.value) -# @pytest.mark.parametrize( -# "fmriprep_versions, unrecog_versions", -# [ -# (["20.2.7", "vA.B"], ["vA.B"]), -# (["C.D.E", "F.G.H"], ["C.D.E", "F.G.H"]), -# ], -# ) -# def test_unrecognized_pipeline_versions_raise_error( -# fmriprep_versions, unrecog_versions -# ): -# """Test that versions of a pipeline not found in the pipeline catalog raise an informative error.""" -# with pytest.raises(LookupError) as e: -# derivative_utils.classify_pipeline_versions( -# "fmriprep", fmriprep_versions -# ) - -# assert all( -# substr in str(e.value) -# for substr in ["unrecognized fmriprep versions"] + unrecog_versions -# ) +@pytest.mark.parametrize( + "fmriprep_versions, expctd_recog_versions, expctd_unrecog_versions", + [ + (["20.2.7", "vA.B"], ["20.2.7"], ["vA.B"]), + (["C.D.E", "F.G.H"], [], ["C.D.E", "F.G.H"]), + ], +) +def test_pipeline_versions_classified_correctly( + fmriprep_versions, expctd_recog_versions, expctd_unrecog_versions +): + """Test that versions of a pipeline are correctly classified as recognized or unrecognized according to the pipeline catalog.""" + recog_versions, unrecog_versions = ( + derivative_utils.classify_pipeline_versions( + "fmriprep", fmriprep_versions + ) + ) + assert recog_versions == expctd_recog_versions + assert unrecog_versions == expctd_unrecog_versions def test_create_completed_pipelines(): From 8a543749022338748c060c9e8b817c1e8d0f79c8 Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Tue, 3 Dec 2024 21:49:39 -0500 Subject: [PATCH 07/22] add e2e test of proc status file with some unrecognized pipelines --- tests/integration/test_cli_derivatives.py | 64 +++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/tests/integration/test_cli_derivatives.py b/tests/integration/test_cli_derivatives.py index 9cbcfc2..350a834 100644 --- a/tests/integration/test_cli_derivatives.py +++ b/tests/integration/test_cli_derivatives.py @@ -2,6 +2,7 @@ import pytest +from bagel import mappings from bagel.cli import bagel @@ -205,3 +206,66 @@ def test_custom_imaging_sessions_created_for_missing_session_labels( # Note: order of items does not matter for dict comparison assert custom_ses_completed_pipes == completed_pipes_for_missing_ses_sub + + +def test_unrecognized_pipelines_excluded_from_output( + runner, + test_data, + test_data_upload_path, + default_derivatives_output_path, + load_test_json, +): + """ + Test that when a subset of pipelines or versions from a processing status file are unrecognized, + they are excluded from the output JSONLD with informative warnings, without causing the derivatives command to fail. + """ + with pytest.warns(UserWarning) as w: + result = runner.invoke( + bagel, + [ + "derivatives", + "-t", + test_data / "proc_status_unrecognized_pipelines.tsv", + "-p", + test_data_upload_path / "example_synthetic.jsonld", + "-o", + default_derivatives_output_path, + ], + catch_exceptions=False, + ) + + assert result.exit_code == 0, f"Errored out. STDOUT: {result.output}" + + assert len(w) == 2 + warnings = [warning.message.args[0] for warning in w] + for warn_substrings in [ + ("unrecognized pipelines", "unknown-pipeline"), + ("unrecognized versions", "{'fmriprep': ['unknown.version']}"), + ]: + assert any( + all(substr in warning for substr in warn_substrings) + for warning in warnings + ) + + output = load_test_json(default_derivatives_output_path) + + sessions_with_completed_pipes = {} + for sub in output["hasSamples"]: + if sub["hasLabel"] == "sub-01": + for ses in sub["hasSession"]: + if ( + ses["schemaKey"] == "ImagingSession" + and "hasCompletedPipeline" in ses + ): + sessions_with_completed_pipes[ses["hasLabel"]] = ses[ + "hasCompletedPipeline" + ] + + assert sessions_with_completed_pipes.keys() == {"ses-01"} + ses01_completed_pipes = sessions_with_completed_pipes["ses-01"] + assert len(ses01_completed_pipes) == 1 + assert ( + ses01_completed_pipes[0]["hasPipelineName"]["identifier"] + == f"{mappings.NP.pf}:freesurfer" + ) + assert ses01_completed_pipes[0]["hasPipelineVersion"] == "7.3.2" From 37220a9d5b3699aa70bc8de22f63178a7a58ae1e Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Tue, 3 Dec 2024 22:29:46 -0500 Subject: [PATCH 08/22] fix message formatting --- bagel/utilities/derivative_utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bagel/utilities/derivative_utils.py b/bagel/utilities/derivative_utils.py index a08b2e5..d940b47 100644 --- a/bagel/utilities/derivative_utils.py +++ b/bagel/utilities/derivative_utils.py @@ -25,7 +25,7 @@ def get_recognized_pipelines(pipelines: Iterable[str]) -> list: """ allowed_pipelines_message = ( f"Allowed pipeline names are the following pipelines supported natively in Nipoppy (https://github.com/nipoppy/pipeline-catalog):\n" - f"{mappings.KNOWN_PIPELINE_URIS.keys()}" + f"{list(mappings.KNOWN_PIPELINE_URIS.keys())}" ) recognized_pipelines = list( set(pipelines).intersection(mappings.KNOWN_PIPELINE_URIS) @@ -97,7 +97,7 @@ def check_at_least_one_pipeline_version_is_recognized(status_df: pd.DataFrame): if total_recognized_versions == 0: # TODO: Consider simply exiting with a message and no output instead? raise LookupError( - f"The processing status file contains no recognized versions of any pipelines in the column '{PROC_STATUS_COLS['pipeline_version']}'.\n" + f"The processing status file contains no recognized versions of {recognized_pipelines} in the column '{PROC_STATUS_COLS['pipeline_version']}'.\n" f"{more_info_message}" ) if len(unrecognized_pipeline_versions) > 0: From b85e5a14db864181230e3d2c2b439f374e6dca2b Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Tue, 3 Dec 2024 22:36:41 -0500 Subject: [PATCH 09/22] test error when no pipeline-versions recognized --- tests/integration/test_cli_derivatives.py | 34 ++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/tests/integration/test_cli_derivatives.py b/tests/integration/test_cli_derivatives.py index 350a834..92bd74b 100644 --- a/tests/integration/test_cli_derivatives.py +++ b/tests/integration/test_cli_derivatives.py @@ -208,7 +208,7 @@ def test_custom_imaging_sessions_created_for_missing_session_labels( assert custom_ses_completed_pipes == completed_pipes_for_missing_ses_sub -def test_unrecognized_pipelines_excluded_from_output( +def test_unrecognized_pipelines_and_versions_excluded_from_output( runner, test_data, test_data_upload_path, @@ -269,3 +269,35 @@ def test_unrecognized_pipelines_excluded_from_output( == f"{mappings.NP.pf}:freesurfer" ) assert ses01_completed_pipes[0]["hasPipelineVersion"] == "7.3.2" + + +def test_error_when_no_pipeline_version_combos_recognized( + runner, + test_data, + test_data_upload_path, + default_derivatives_output_path, + load_test_json, +): + """ + Test that when there is no recognized pipeline-version combination in the processing status file, + an error is raised and no output JSONLD is created. + """ + with pytest.raises(LookupError) as e: + runner.invoke( + bagel, + [ + "derivatives", + "-t", + test_data / "proc_status_no_recognized_pipelines.tsv", + "-p", + test_data_upload_path / "example_synthetic.jsonld", + "-o", + default_derivatives_output_path, + ], + catch_exceptions=False, + ) + + assert "no recognized versions" in str(e.value) + assert ( + not default_derivatives_output_path.exists() + ), "A JSONLD was created despite inputs being invalid." From cd92b512958d63a23f3aa8b9b8d600c6fa05ca5f Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Tue, 3 Dec 2024 22:36:54 -0500 Subject: [PATCH 10/22] fix assertion --- tests/unit/test_derivative_utils.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/unit/test_derivative_utils.py b/tests/unit/test_derivative_utils.py index 47df375..de50222 100644 --- a/tests/unit/test_derivative_utils.py +++ b/tests/unit/test_derivative_utils.py @@ -162,8 +162,9 @@ def test_pipeline_versions_classified_correctly( "fmriprep", fmriprep_versions ) ) - assert recog_versions == expctd_recog_versions - assert unrecog_versions == expctd_unrecog_versions + # The order of the versions in the lists is not guaranteed + assert set(recog_versions) == set(expctd_recog_versions) + assert set(unrecog_versions) == set(expctd_unrecog_versions) def test_create_completed_pipelines(): From 2f031f1f56269d8500d1e31372b7a3ba1682ca6a Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Tue, 3 Dec 2024 22:37:49 -0500 Subject: [PATCH 11/22] remove TODO --- bagel/utilities/derivative_utils.py | 1 - 1 file changed, 1 deletion(-) diff --git a/bagel/utilities/derivative_utils.py b/bagel/utilities/derivative_utils.py index d940b47..1952a26 100644 --- a/bagel/utilities/derivative_utils.py +++ b/bagel/utilities/derivative_utils.py @@ -74,7 +74,6 @@ def check_at_least_one_pipeline_version_is_recognized(status_df: pd.DataFrame): "For a full list, see https://github.com/nipoppy/pipeline-catalog." ) - # TODO: Handle error in this func here as well? recognized_pipelines = get_recognized_pipelines( status_df[PROC_STATUS_COLS["pipeline_name"]].unique() ) From ebc8186aca13f9352c1844d3d74ffbbbffbc6255 Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Tue, 3 Dec 2024 22:40:12 -0500 Subject: [PATCH 12/22] add new test processing status TSVs --- tests/data/proc_status_no_recognized_pipelines.tsv | 6 ++++++ tests/data/proc_status_unrecognized_pipelines.tsv | 6 ++++++ 2 files changed, 12 insertions(+) create mode 100644 tests/data/proc_status_no_recognized_pipelines.tsv create mode 100644 tests/data/proc_status_unrecognized_pipelines.tsv diff --git a/tests/data/proc_status_no_recognized_pipelines.tsv b/tests/data/proc_status_no_recognized_pipelines.tsv new file mode 100644 index 0000000..b08df9e --- /dev/null +++ b/tests/data/proc_status_no_recognized_pipelines.tsv @@ -0,0 +1,6 @@ +participant_id bids_participant_id session_id bids_session_id pipeline_name pipeline_version pipeline_step status +01 sub-01 01 ses-01 fmriprep unknown.version1 step1 FAIL +01 sub-01 01 ses-01 fmriprep unknown.version1 step2 INCOMPLETE +01 sub-01 01 ses-01 fmriprep unknown.version2 default SUCCESS +01 sub-01 01 ses-01 freesurfer unknown.version3 default SUCCESS +01 sub-01 02 ses-02 freesurfer unknown.version3 default UNAVAILABLE \ No newline at end of file diff --git a/tests/data/proc_status_unrecognized_pipelines.tsv b/tests/data/proc_status_unrecognized_pipelines.tsv new file mode 100644 index 0000000..738199d --- /dev/null +++ b/tests/data/proc_status_unrecognized_pipelines.tsv @@ -0,0 +1,6 @@ +participant_id bids_participant_id session_id bids_session_id pipeline_name pipeline_version pipeline_step status +01 sub-01 01 ses-01 fmriprep unknown.version step1 FAIL +01 sub-01 01 ses-01 fmriprep unknown.version step2 INCOMPLETE +01 sub-01 01 ses-01 unknown-pipeline 1.0.0 default SUCCESS +01 sub-01 01 ses-01 freesurfer 7.3.2 default SUCCESS +01 sub-01 02 ses-02 freesurfer 7.3.2 default UNAVAILABLE \ No newline at end of file From 1c8a4d34aec578d20ad0b3b7119836c5874a5f21 Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Tue, 3 Dec 2024 22:44:31 -0500 Subject: [PATCH 13/22] update test data README --- tests/data/README.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/data/README.md b/tests/data/README.md index d47e758..c1171e4 100644 --- a/tests/data/README.md +++ b/tests/data/README.md @@ -43,6 +43,9 @@ _incomplete.tsv | Has a missing value in the `bids_participant_id` column | Fail _unique_sessions.csv | Includes a unique subject-session (`sub-01`, `ses-03`) not found in the synthetic dataset | Pass _missing_sessions.tsv | One subject (`sub-02`) is missing all session labels | Pass _no_bids_sessions.tsv | Has session labels in all rows for `session_id`, but no values in `bids_session_id` column | Pass +_unrecognized_pipelines.tsv | Includes some pipeline names and versions not found in the pipeline catalog | Pass +_no_recognized_pipelines.tsv | Includes pipeline names found in the pipeline catalog, but no recognized versions | Fail + ## Example expected CLI outputs From a5ff5b4737158abf602fac14313a7298f47ab43a Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Tue, 3 Dec 2024 22:47:07 -0500 Subject: [PATCH 14/22] simplify conditionals --- bagel/utilities/derivative_utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bagel/utilities/derivative_utils.py b/bagel/utilities/derivative_utils.py index 1952a26..8848336 100644 --- a/bagel/utilities/derivative_utils.py +++ b/bagel/utilities/derivative_utils.py @@ -39,7 +39,7 @@ def get_recognized_pipelines(pipelines: Iterable[str]) -> list: f"The processing status file contains no recognized pipelines in the column '{PROC_STATUS_COLS['pipeline_name']}'.\n" f"{allowed_pipelines_message}" ) - if len(unrecognized_pipelines) > 0: + if unrecognized_pipelines: warnings.warn( f"The processing status file contains unrecognized pipelines in the column '{PROC_STATUS_COLS['pipeline_name']}': " f"{unrecognized_pipelines}. These will be ignored.\n" @@ -99,7 +99,7 @@ def check_at_least_one_pipeline_version_is_recognized(status_df: pd.DataFrame): f"The processing status file contains no recognized versions of {recognized_pipelines} in the column '{PROC_STATUS_COLS['pipeline_version']}'.\n" f"{more_info_message}" ) - if len(unrecognized_pipeline_versions) > 0: + if unrecognized_pipeline_versions: warnings.warn( f"The processing status file contains unrecognized versions of the following pipelines in the column '{PROC_STATUS_COLS['pipeline_version']}': " f"{unrecognized_pipeline_versions}. These will be ignored.\n" From de86836ce817e5d3887b61d4eef7ef29d7dae632 Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Tue, 3 Dec 2024 22:53:24 -0500 Subject: [PATCH 15/22] merge if statements --- bagel/utilities/derivative_utils.py | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/bagel/utilities/derivative_utils.py b/bagel/utilities/derivative_utils.py index 8848336..942c477 100644 --- a/bagel/utilities/derivative_utils.py +++ b/bagel/utilities/derivative_utils.py @@ -122,18 +122,16 @@ def create_completed_pipelines(session_proc_df: pd.DataFrame) -> list: if ( pipeline in mappings.KNOWN_PIPELINE_URIS and version in mappings.KNOWN_PIPELINE_VERSIONS[pipeline] - ): - # Check that all pipeline steps have succeeded - if ( - session_pipe_df[PROC_STATUS_COLS["status"]].str.lower() - == "success" - ).all(): - completed_pipeline = models.CompletedPipeline( - hasPipelineName=models.Pipeline( - identifier=mappings.KNOWN_PIPELINE_URIS[pipeline] - ), - hasPipelineVersion=version, - ) - completed_pipelines.append(completed_pipeline) + ) and ( + session_pipe_df[PROC_STATUS_COLS["status"]].str.lower() + == "success" + ).all(): + completed_pipeline = models.CompletedPipeline( + hasPipelineName=models.Pipeline( + identifier=mappings.KNOWN_PIPELINE_URIS[pipeline] + ), + hasPipelineVersion=version, + ) + completed_pipelines.append(completed_pipeline) return completed_pipelines From 0aad5ecc6a591a8253739df88089c4b4252b3da6 Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Wed, 4 Dec 2024 12:34:47 -0500 Subject: [PATCH 16/22] simplify conditionals --- bagel/utilities/derivative_utils.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/bagel/utilities/derivative_utils.py b/bagel/utilities/derivative_utils.py index 942c477..04c1cb5 100644 --- a/bagel/utilities/derivative_utils.py +++ b/bagel/utilities/derivative_utils.py @@ -34,7 +34,7 @@ def get_recognized_pipelines(pipelines: Iterable[str]) -> list: set(pipelines).difference(mappings.KNOWN_PIPELINE_URIS) ) - if len(recognized_pipelines) == 0: + if not recognized_pipelines: raise LookupError( f"The processing status file contains no recognized pipelines in the column '{PROC_STATUS_COLS['pipeline_name']}'.\n" f"{allowed_pipelines_message}" @@ -78,7 +78,7 @@ def check_at_least_one_pipeline_version_is_recognized(status_df: pd.DataFrame): status_df[PROC_STATUS_COLS["pipeline_name"]].unique() ) - total_recognized_versions = 0 + any_recognized_versions = 0 unrecognized_pipeline_versions = {} for pipeline in recognized_pipelines: versions = status_df[ @@ -89,11 +89,11 @@ def check_at_least_one_pipeline_version_is_recognized(status_df: pd.DataFrame): classify_pipeline_versions(pipeline, versions) ) - total_recognized_versions += len(recognized_versions) + any_recognized_versions += len(recognized_versions) if len(unrecognized_versions) > 0: unrecognized_pipeline_versions[pipeline] = unrecognized_versions - if total_recognized_versions == 0: + if not any_recognized_versions: # TODO: Consider simply exiting with a message and no output instead? raise LookupError( f"The processing status file contains no recognized versions of {recognized_pipelines} in the column '{PROC_STATUS_COLS['pipeline_version']}'.\n" From 42940408113a0b4d019a653574245ea895ec8d5f Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Fri, 6 Dec 2024 14:09:47 -0500 Subject: [PATCH 17/22] update message template for unrecognized pipelines --- bagel/utilities/derivative_utils.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/bagel/utilities/derivative_utils.py b/bagel/utilities/derivative_utils.py index 04c1cb5..e0b51d2 100644 --- a/bagel/utilities/derivative_utils.py +++ b/bagel/utilities/derivative_utils.py @@ -23,10 +23,6 @@ def get_recognized_pipelines(pipelines: Iterable[str]) -> list: Check that all pipelines in the processing status file are supported by Nipoppy. Raise an error if all pipelines are unrecognized, otherwise warn about unrecognized pipelines. """ - allowed_pipelines_message = ( - f"Allowed pipeline names are the following pipelines supported natively in Nipoppy (https://github.com/nipoppy/pipeline-catalog):\n" - f"{list(mappings.KNOWN_PIPELINE_URIS.keys())}" - ) recognized_pipelines = list( set(pipelines).intersection(mappings.KNOWN_PIPELINE_URIS) ) @@ -34,16 +30,20 @@ def get_recognized_pipelines(pipelines: Iterable[str]) -> list: set(pipelines).difference(mappings.KNOWN_PIPELINE_URIS) ) + unrecognized_pipelines_template = ( + f"Unrecognized processing pipelines: {unrecognized_pipelines}\n" + f"Supported pipelines are those in the Nipoppy pipeline catalog (https://github.com/nipoppy/pipeline-catalog):\n" + f"{list(mappings.KNOWN_PIPELINE_URIS.keys())}" + ) if not recognized_pipelines: raise LookupError( - f"The processing status file contains no recognized pipelines in the column '{PROC_STATUS_COLS['pipeline_name']}'.\n" - f"{allowed_pipelines_message}" + f"The processing status file contains no recognized pipelines in the column: '{PROC_STATUS_COLS['pipeline_name']}'.\n" + f"{unrecognized_pipelines_template}" ) if unrecognized_pipelines: warnings.warn( - f"The processing status file contains unrecognized pipelines in the column '{PROC_STATUS_COLS['pipeline_name']}': " - f"{unrecognized_pipelines}. These will be ignored.\n" - f"{allowed_pipelines_message}" + f"The processing status file contains unrecognized pipelines in the column: '{PROC_STATUS_COLS['pipeline_name']}'. These will be ignored.\n" + f"{unrecognized_pipelines_template}" ) return recognized_pipelines From 87945fb615c7742fc5eec8bee93042a050be259f Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Fri, 6 Dec 2024 14:12:45 -0500 Subject: [PATCH 18/22] rename util func --- bagel/utilities/derivative_utils.py | 4 ++-- tests/unit/test_derivative_utils.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/bagel/utilities/derivative_utils.py b/bagel/utilities/derivative_utils.py index e0b51d2..0e38a13 100644 --- a/bagel/utilities/derivative_utils.py +++ b/bagel/utilities/derivative_utils.py @@ -48,7 +48,7 @@ def get_recognized_pipelines(pipelines: Iterable[str]) -> list: return recognized_pipelines -def classify_pipeline_versions( +def validate_pipeline_versions( pipeline: str, versions: Iterable[str] ) -> tuple[list, list]: """ @@ -86,7 +86,7 @@ def check_at_least_one_pipeline_version_is_recognized(status_df: pd.DataFrame): ][PROC_STATUS_COLS["pipeline_version"]].unique() recognized_versions, unrecognized_versions = ( - classify_pipeline_versions(pipeline, versions) + validate_pipeline_versions(pipeline, versions) ) any_recognized_versions += len(recognized_versions) diff --git a/tests/unit/test_derivative_utils.py b/tests/unit/test_derivative_utils.py index de50222..51035da 100644 --- a/tests/unit/test_derivative_utils.py +++ b/tests/unit/test_derivative_utils.py @@ -158,7 +158,7 @@ def test_pipeline_versions_classified_correctly( ): """Test that versions of a pipeline are correctly classified as recognized or unrecognized according to the pipeline catalog.""" recog_versions, unrecog_versions = ( - derivative_utils.classify_pipeline_versions( + derivative_utils.validate_pipeline_versions( "fmriprep", fmriprep_versions ) ) From 0581446b76e31744939b6920a70c935b8fa14753 Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Fri, 6 Dec 2024 15:17:22 -0500 Subject: [PATCH 19/22] switch to boolean flag for recognized vers --- bagel/utilities/derivative_utils.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/bagel/utilities/derivative_utils.py b/bagel/utilities/derivative_utils.py index 0e38a13..a01b9f7 100644 --- a/bagel/utilities/derivative_utils.py +++ b/bagel/utilities/derivative_utils.py @@ -69,6 +69,7 @@ def check_at_least_one_pipeline_version_is_recognized(status_df: pd.DataFrame): """ Check that at least one pipeline name and version combination found in the processing status file is supported by Nipoppy. """ + more_info_message = ( "Allowed processing pipelines and versions are those supported natively in Nipoppy. " "For a full list, see https://github.com/nipoppy/pipeline-catalog." @@ -78,7 +79,7 @@ def check_at_least_one_pipeline_version_is_recognized(status_df: pd.DataFrame): status_df[PROC_STATUS_COLS["pipeline_name"]].unique() ) - any_recognized_versions = 0 + any_recognized_versions = False unrecognized_pipeline_versions = {} for pipeline in recognized_pipelines: versions = status_df[ @@ -89,8 +90,10 @@ def check_at_least_one_pipeline_version_is_recognized(status_df: pd.DataFrame): validate_pipeline_versions(pipeline, versions) ) - any_recognized_versions += len(recognized_versions) - if len(unrecognized_versions) > 0: + if recognized_versions: + any_recognized_versions = True + + if unrecognized_versions: unrecognized_pipeline_versions[pipeline] = unrecognized_versions if not any_recognized_versions: From 789d49330ec68d6c56723d63da84abf6d6004cda Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Fri, 6 Dec 2024 16:00:54 -0500 Subject: [PATCH 20/22] make unrecognized pipeline versions clearer --- bagel/utilities/derivative_utils.py | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/bagel/utilities/derivative_utils.py b/bagel/utilities/derivative_utils.py index a01b9f7..8203f66 100644 --- a/bagel/utilities/derivative_utils.py +++ b/bagel/utilities/derivative_utils.py @@ -30,7 +30,7 @@ def get_recognized_pipelines(pipelines: Iterable[str]) -> list: set(pipelines).difference(mappings.KNOWN_PIPELINE_URIS) ) - unrecognized_pipelines_template = ( + unrecognized_pipelines_details = ( f"Unrecognized processing pipelines: {unrecognized_pipelines}\n" f"Supported pipelines are those in the Nipoppy pipeline catalog (https://github.com/nipoppy/pipeline-catalog):\n" f"{list(mappings.KNOWN_PIPELINE_URIS.keys())}" @@ -38,12 +38,12 @@ def get_recognized_pipelines(pipelines: Iterable[str]) -> list: if not recognized_pipelines: raise LookupError( f"The processing status file contains no recognized pipelines in the column: '{PROC_STATUS_COLS['pipeline_name']}'.\n" - f"{unrecognized_pipelines_template}" + f"{unrecognized_pipelines_details}" ) if unrecognized_pipelines: warnings.warn( f"The processing status file contains unrecognized pipelines in the column: '{PROC_STATUS_COLS['pipeline_name']}'. These will be ignored.\n" - f"{unrecognized_pipelines_template}" + f"{unrecognized_pipelines_details}" ) return recognized_pipelines @@ -69,12 +69,6 @@ def check_at_least_one_pipeline_version_is_recognized(status_df: pd.DataFrame): """ Check that at least one pipeline name and version combination found in the processing status file is supported by Nipoppy. """ - - more_info_message = ( - "Allowed processing pipelines and versions are those supported natively in Nipoppy. " - "For a full list, see https://github.com/nipoppy/pipeline-catalog." - ) - recognized_pipelines = get_recognized_pipelines( status_df[PROC_STATUS_COLS["pipeline_name"]].unique() ) @@ -89,24 +83,27 @@ def check_at_least_one_pipeline_version_is_recognized(status_df: pd.DataFrame): recognized_versions, unrecognized_versions = ( validate_pipeline_versions(pipeline, versions) ) - if recognized_versions: any_recognized_versions = True - if unrecognized_versions: unrecognized_pipeline_versions[pipeline] = unrecognized_versions + unrecognized_versions_details = ( + f"Unrecognized processing pipeline versions: {unrecognized_pipeline_versions}\n" + "Supported pipeline versions are those in the Nipoppy pipeline catalog. " + "For a full list, see https://github.com/nipoppy/pipeline-catalog." + ) if not any_recognized_versions: # TODO: Consider simply exiting with a message and no output instead? raise LookupError( - f"The processing status file contains no recognized versions of {recognized_pipelines} in the column '{PROC_STATUS_COLS['pipeline_version']}'.\n" - f"{more_info_message}" + f"The processing status file contains no recognized versions of any pipelines in the column '{PROC_STATUS_COLS['pipeline_version']}'.\n" + f"{unrecognized_versions_details}" ) if unrecognized_pipeline_versions: warnings.warn( - f"The processing status file contains unrecognized versions of the following pipelines in the column '{PROC_STATUS_COLS['pipeline_version']}': " - f"{unrecognized_pipeline_versions}. These will be ignored.\n" - f"{more_info_message}" + f"The processing status file contains unrecognized versions of pipelines in the column '{PROC_STATUS_COLS['pipeline_version']}'. " + "These will be ignored.\n" + f"{unrecognized_versions_details}" ) From ddedf4709783f90dee04c9e4395a97963ee7c2ef Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Mon, 9 Dec 2024 11:49:37 -0500 Subject: [PATCH 21/22] update test parameter names --- tests/integration/test_cli_derivatives.py | 14 +++++++------- tests/unit/test_derivative_utils.py | 8 ++++---- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/tests/integration/test_cli_derivatives.py b/tests/integration/test_cli_derivatives.py index 92bd74b..2549c12 100644 --- a/tests/integration/test_cli_derivatives.py +++ b/tests/integration/test_cli_derivatives.py @@ -238,13 +238,13 @@ def test_unrecognized_pipelines_and_versions_excluded_from_output( assert len(w) == 2 warnings = [warning.message.args[0] for warning in w] - for warn_substrings in [ - ("unrecognized pipelines", "unknown-pipeline"), - ("unrecognized versions", "{'fmriprep': ['unknown.version']}"), - ]: - assert any( - all(substr in warning for substr in warn_substrings) - for warning in warnings + for warning in warnings: + assert ( + "unrecognized pipelines" in warning + and "unknown-pipeline" in warning + ) or ( + "unrecognized versions" in warning + and "{'fmriprep': ['unknown.version']}" in warning ) output = load_test_json(default_derivatives_output_path) diff --git a/tests/unit/test_derivative_utils.py b/tests/unit/test_derivative_utils.py index 51035da..9519cb9 100644 --- a/tests/unit/test_derivative_utils.py +++ b/tests/unit/test_derivative_utils.py @@ -147,14 +147,14 @@ def test_error_raised_when_no_pipeline_names_recognized(): @pytest.mark.parametrize( - "fmriprep_versions, expctd_recog_versions, expctd_unrecog_versions", + "fmriprep_versions, expected_recog_versions, expected_unrecog_versions", [ (["20.2.7", "vA.B"], ["20.2.7"], ["vA.B"]), (["C.D.E", "F.G.H"], [], ["C.D.E", "F.G.H"]), ], ) def test_pipeline_versions_classified_correctly( - fmriprep_versions, expctd_recog_versions, expctd_unrecog_versions + fmriprep_versions, expected_recog_versions, expected_unrecog_versions ): """Test that versions of a pipeline are correctly classified as recognized or unrecognized according to the pipeline catalog.""" recog_versions, unrecog_versions = ( @@ -163,8 +163,8 @@ def test_pipeline_versions_classified_correctly( ) ) # The order of the versions in the lists is not guaranteed - assert set(recog_versions) == set(expctd_recog_versions) - assert set(unrecog_versions) == set(expctd_unrecog_versions) + assert set(recog_versions) == set(expected_recog_versions) + assert set(unrecog_versions) == set(expected_unrecog_versions) def test_create_completed_pipelines(): From 5174bc6a66c663e316d3814f6723a9c47a8456b3 Mon Sep 17 00:00:00 2001 From: Alyssa Dai Date: Mon, 9 Dec 2024 11:51:37 -0500 Subject: [PATCH 22/22] switch order of asserts --- tests/integration/test_cli_derivatives.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/test_cli_derivatives.py b/tests/integration/test_cli_derivatives.py index 2549c12..62ae551 100644 --- a/tests/integration/test_cli_derivatives.py +++ b/tests/integration/test_cli_derivatives.py @@ -261,8 +261,8 @@ def test_unrecognized_pipelines_and_versions_excluded_from_output( "hasCompletedPipeline" ] + ses01_completed_pipes = sessions_with_completed_pipes.get("ses-01") assert sessions_with_completed_pipes.keys() == {"ses-01"} - ses01_completed_pipes = sessions_with_completed_pipes["ses-01"] assert len(ses01_completed_pipes) == 1 assert ( ses01_completed_pipes[0]["hasPipelineName"]["identifier"]