From 3dfdcb197a17d73b2818c574d83c9a3131165c84 Mon Sep 17 00:00:00 2001 From: Michelle Wang Date: Thu, 11 Jul 2024 15:37:06 +0800 Subject: [PATCH] [FIX] Fix doughnut path and other bugs (#274) * fix doughnut path to be in correct directory * do not update doughnut in BidsConversionWorkflow if simulate is True * refactor PipelineConfig and PipelineStepConfig to add UPDATE_DOUGHNUT field for BIDS pipelines * use Union instead of | in Pydantic annotation * add comments for clarity * remove unused property * fix docs warnings --- nipoppy_cli/docs/source/schemas/index.rst | 2 +- nipoppy_cli/nipoppy/config/__init__.py | 8 +- nipoppy_cli/nipoppy/config/main.py | 44 +++++-- nipoppy_cli/nipoppy/config/pipeline.py | 60 +++++++--- nipoppy_cli/nipoppy/config/pipeline_step.py | 21 +++- .../sample_global_config-all_pipelines.json | 6 +- ...sample_global_config-latest_pipelines.json | 10 +- .../nipoppy/data/layouts/layout-default.json | 2 +- .../nipoppy/workflows/bids_conversion.py | 10 +- nipoppy_cli/nipoppy/workflows/pipeline.py | 4 +- nipoppy_cli/tests/conftest.py | 2 +- nipoppy_cli/tests/test_config_main.py | 6 +- nipoppy_cli/tests/test_config_pipeline.py | 107 ++++++++++-------- .../tests/test_config_pipeline_step.py | 60 +++++++--- nipoppy_cli/tests/test_layout.py | 5 + .../tests/test_workflow_bids_conversion.py | 52 ++++++++- nipoppy_cli/tests/test_workflow_pipeline.py | 14 +-- 17 files changed, 292 insertions(+), 121 deletions(-) diff --git a/nipoppy_cli/docs/source/schemas/index.rst b/nipoppy_cli/docs/source/schemas/index.rst index 6fe2825f..ed638f39 100644 --- a/nipoppy_cli/docs/source/schemas/index.rst +++ b/nipoppy_cli/docs/source/schemas/index.rst @@ -25,7 +25,7 @@ Below is the schema used for the global configuration :term:`JSON` file. Tracker configuration file ~~~~~~~~~~~~~~~~~~~~~~~~~~ -The tracker configuration file specified in the `PipelineConfig`_ should be a :term:`JSON` file that contains **a list** of tracker configurations. +The tracker configuration file specified in the `ProcPipelineConfig`_ should be a :term:`JSON` file that contains **a list** of tracker configurations. The schema for each individual tracker configuration is shown below. .. jsonschema:: tracker.json diff --git a/nipoppy_cli/nipoppy/config/__init__.py b/nipoppy_cli/nipoppy/config/__init__.py index 4b947f94..1afe062e 100644 --- a/nipoppy_cli/nipoppy/config/__init__.py +++ b/nipoppy_cli/nipoppy/config/__init__.py @@ -3,6 +3,10 @@ from .boutiques import BoutiquesConfig from .container import ContainerConfig from .main import Config -from .pipeline import PipelineConfig -from .pipeline_step import PipelineStepConfig +from .pipeline import BasePipelineConfig, BidsPipelineConfig, ProcPipelineConfig +from .pipeline_step import ( + BasePipelineStepConfig, + BidsPipelineStepConfig, + ProcPipelineStepConfig, +) from .tracker import TrackerConfig diff --git a/nipoppy_cli/nipoppy/config/main.py b/nipoppy_cli/nipoppy/config/main.py index 540bd180..e2e89418 100644 --- a/nipoppy_cli/nipoppy/config/main.py +++ b/nipoppy_cli/nipoppy/config/main.py @@ -9,7 +9,12 @@ from typing_extensions import Self from nipoppy.config.container import SchemaWithContainerConfig -from nipoppy.config.pipeline import PipelineConfig +from nipoppy.config.pipeline import ( + BasePipelineConfig, + BidsPipelineConfig, + ProcPipelineConfig, +) +from nipoppy.config.pipeline_step import BidsPipelineStepConfig, ProcPipelineStepConfig from nipoppy.layout import DEFAULT_LAYOUT_INFO from nipoppy.tabular.dicom_dir_map import DicomDirMap from nipoppy.utils import ( @@ -66,10 +71,10 @@ class Config(SchemaWithContainerConfig): "is loaded from a file with :func:`nipoppy.config.main.Config.load`" ), ) - BIDS_PIPELINES: list[PipelineConfig] = Field( + BIDS_PIPELINES: list[BidsPipelineConfig] = Field( default=[], description="Configurations for BIDS conversion, if applicable" ) - PROC_PIPELINES: list[PipelineConfig] = Field( + PROC_PIPELINES: list[ProcPipelineConfig] = Field( description="Configurations for processing pipelines" ) CUSTOM: dict = Field( @@ -111,7 +116,7 @@ def _check_no_duplicate_pipeline(self) -> Self: def propagate_container_config(self) -> Self: """Propagate the container config to all pipelines.""" - def _propagate(pipeline_configs: list[PipelineConfig]): + def _propagate(pipeline_configs: list[BasePipelineConfig]): for pipeline_config in pipeline_configs: pipeline_container_config = pipeline_config.get_container_config() if pipeline_container_config.INHERIT: @@ -133,14 +138,17 @@ def _propagate(pipeline_configs: list[PipelineConfig]): @model_validator(mode="before") @classmethod def check_input(cls, data: Any): - """Validate the raw input.""" + """ + Validate the raw input. + + Specifically: + - If session_ids is not given, set to be the same as visit_ids + """ key_session_ids = "SESSION_IDS" key_visit_ids = "VISIT_IDS" if isinstance(data, dict): - # if session_ids is not given, set to be the same as visit_ids if key_session_ids not in data: data[key_session_ids] = data[key_visit_ids] - return data @model_validator(mode="after") @@ -148,6 +156,26 @@ def validate_and_process(self) -> Self: """Validate and process the configuration.""" self._check_dicom_dir_options() self._check_no_duplicate_pipeline() + + # make sure BIDS/processing pipelines are the right type + for pipeline_configs, step_class in [ + (self.BIDS_PIPELINES, BidsPipelineStepConfig), + (self.PROC_PIPELINES, ProcPipelineStepConfig), + ]: + for pipeline_config in pipeline_configs: + # type annotation to make IDE smarter + pipeline_config: BasePipelineConfig + steps = pipeline_config.STEPS + for i_step in range(len(steps)): + # extract fields used to create (possibly incorrect) step object + # and use them to create a new (correct) step object + # (this is needed because BidsPipelineStepConfig and + # ProcPipelineStepConfig share some fields, and the fields + # that are different are optional, so the default Pydantic + # parsing can create the wrong type of step object) + steps[i_step] = step_class( + **steps[i_step].model_dump(exclude_unset=True) + ) return self def get_pipeline_version(self, pipeline_name: str) -> str: @@ -176,7 +204,7 @@ def get_pipeline_config( self, pipeline_name: str, pipeline_version: str, - ) -> PipelineConfig: + ) -> ProcPipelineConfig: """Get the config for a BIDS or processing pipeline.""" # pooling them together since there should not be any duplicates for pipeline_config in self.PROC_PIPELINES + self.BIDS_PIPELINES: diff --git a/nipoppy_cli/nipoppy/config/pipeline.py b/nipoppy_cli/nipoppy/config/pipeline.py index 23ae1a4b..26e90270 100644 --- a/nipoppy_cli/nipoppy/config/pipeline.py +++ b/nipoppy_cli/nipoppy/config/pipeline.py @@ -2,17 +2,22 @@ from __future__ import annotations +from abc import ABC from pathlib import Path -from typing import Optional +from typing import Optional, Union from pydantic import ConfigDict, Field, model_validator from nipoppy.config.container import ContainerInfo, SchemaWithContainerConfig -from nipoppy.config.pipeline_step import PipelineStepConfig +from nipoppy.config.pipeline_step import ( + BasePipelineStepConfig, + BidsPipelineStepConfig, + ProcPipelineStepConfig, +) -class PipelineConfig(SchemaWithContainerConfig): - """Schema for processing pipeline configuration.""" +class BasePipelineConfig(SchemaWithContainerConfig, ABC): + """Base schema for processing/BIDS pipeline configuration.""" NAME: str = Field(description="Name of the pipeline") VERSION: str = Field(description="Version of the pipeline") @@ -23,21 +28,10 @@ class PipelineConfig(SchemaWithContainerConfig): default=ContainerInfo(), description="Information about the container image file", ) - STEPS: list[PipelineStepConfig] = Field( + STEPS: list[Union[ProcPipelineStepConfig, BidsPipelineStepConfig]] = Field( default=[], description="List of pipeline step configurations", ) - TRACKER_CONFIG_FILE: Optional[Path] = Field( - default=None, - description=( - "Path to the tracker configuration file associated with the pipeline" - ". This file must contain a list of tracker configurations" - ", each of which must be a dictionary with a NAME field (string)" - " and a PATHS field (non-empty list of strings)" - ), - ) - - model_config = ConfigDict(extra="forbid") @model_validator(mode="after") def validate_after(self): @@ -71,7 +65,9 @@ def get_fpath_container(self) -> Path: """Return the path to the pipeline's container.""" return self.CONTAINER_INFO.FILE - def get_step_config(self, step_name: Optional[str] = None) -> PipelineStepConfig: + def get_step_config( + self, step_name: Optional[str] = None + ) -> BasePipelineStepConfig: """ Return the configuration for the given step. @@ -106,6 +102,36 @@ def get_descriptor_file(self, step_name: Optional[str] = None) -> Path | None: """ return self.get_step_config(step_name).DESCRIPTOR_FILE + +class BidsPipelineConfig(BasePipelineConfig): + """Schema for BIDS pipeline configuration.""" + + model_config = ConfigDict(extra="forbid") + + def get_update_doughnut(self, step_name: Optional[str] = None) -> Path | None: + """ + Return the update doughnut flag for the given step. + + If step is None, return the flag for the first step. + """ + return self.get_step_config(step_name).UPDATE_DOUGHNUT + + +class ProcPipelineConfig(BasePipelineConfig): + """Schema for processing pipeline configuration.""" + + TRACKER_CONFIG_FILE: Optional[Path] = Field( + default=None, + description=( + "Path to the tracker configuration file associated with the pipeline" + ". This file must contain a list of tracker configurations" + ", each of which must be a dictionary with a NAME field (string)" + " and a PATHS field (non-empty list of strings)" + ), + ) + + model_config = ConfigDict(extra="forbid") + def get_pybids_ignore_file(self, step_name: Optional[str] = None) -> Path | None: """ Return the list of regex patterns to ignore when building the PyBIDS layout. diff --git a/nipoppy_cli/nipoppy/config/pipeline_step.py b/nipoppy_cli/nipoppy/config/pipeline_step.py index d1aca986..9e924270 100644 --- a/nipoppy_cli/nipoppy/config/pipeline_step.py +++ b/nipoppy_cli/nipoppy/config/pipeline_step.py @@ -2,15 +2,17 @@ from __future__ import annotations +from abc import ABC from pathlib import Path from typing import Optional from pydantic import ConfigDict, Field from nipoppy.config.container import SchemaWithContainerConfig +from nipoppy.tabular.doughnut import Doughnut -class PipelineStepConfig(SchemaWithContainerConfig): +class BasePipelineStepConfig(SchemaWithContainerConfig, ABC): """Schema for processing pipeline step configuration.""" NAME: Optional[str] = Field( @@ -27,6 +29,11 @@ class PipelineStepConfig(SchemaWithContainerConfig): default=None, description=("Path to the JSON invocation file"), ) + + +class ProcPipelineStepConfig(BasePipelineStepConfig): + """Schema for processing pipeline step configuration.""" + PYBIDS_IGNORE_FILE: Optional[Path] = Field( default=None, description=( @@ -34,5 +41,17 @@ class PipelineStepConfig(SchemaWithContainerConfig): "when building the PyBIDS layout" ), ) + model_config = ConfigDict(extra="forbid") + +class BidsPipelineStepConfig(BasePipelineStepConfig): + """Schema for BIDS pipeline step configuration.""" + + UPDATE_DOUGHNUT: Optional[bool] = Field( + default=False, + description=( + f"Whether or not the {Doughnut.col_in_bids} column " + "in the doughnut file should be updated" + ), + ) model_config = ConfigDict(extra="forbid") diff --git a/nipoppy_cli/nipoppy/data/examples/sample_global_config-all_pipelines.json b/nipoppy_cli/nipoppy/data/examples/sample_global_config-all_pipelines.json index fc94f7f8..f49574a4 100644 --- a/nipoppy_cli/nipoppy/data/examples/sample_global_config-all_pipelines.json +++ b/nipoppy_cli/nipoppy/data/examples/sample_global_config-all_pipelines.json @@ -44,7 +44,8 @@ "--bind", "[[HEUDICONV_HEURISTIC_FILE]]" ] - } + }, + "UPDATE_DOUGHNUT": true } ] }, @@ -70,7 +71,8 @@ "--bind", "[[DCM2BIDS_CONFIG_FILE]]" ] - } + }, + "UPDATE_DOUGHNUT": true } ] } diff --git a/nipoppy_cli/nipoppy/data/examples/sample_global_config-latest_pipelines.json b/nipoppy_cli/nipoppy/data/examples/sample_global_config-latest_pipelines.json index ffea28ab..95689247 100644 --- a/nipoppy_cli/nipoppy/data/examples/sample_global_config-latest_pipelines.json +++ b/nipoppy_cli/nipoppy/data/examples/sample_global_config-latest_pipelines.json @@ -44,7 +44,8 @@ "--bind", "[[HEUDICONV_HEURISTIC_FILE]]" ] - } + }, + "UPDATE_DOUGHNUT": true } ] }, @@ -59,18 +60,19 @@ { "NAME": "prepare", "INVOCATION_FILE": "[[NIPOPPY_DPATH_INVOCATIONS]]/[[NIPOPPY_PIPELINE_NAME]]-[[NIPOPPY_PIPELINE_VERSION]]-prepare.json", - "DESCRIPTOR_FILE": "[[NIPOPPY_DPATH_DESCRIPTORS]]/[[NIPOPPY_PIPELINE_NAME]]-[[NIPOPPY_PIPELINE_VERSION]]-prepare.json" + "DESCRIPTOR_FILE": "[[NIPOPPY_DPATH_DESCRIPTORS]]/dcm2bids_helper-[[NIPOPPY_PIPELINE_VERSION]].json" }, { "NAME": "convert", "INVOCATION_FILE": "[[NIPOPPY_DPATH_INVOCATIONS]]/[[NIPOPPY_PIPELINE_NAME]]-[[NIPOPPY_PIPELINE_VERSION]]-convert.json", - "DESCRIPTOR_FILE": "[[NIPOPPY_DPATH_DESCRIPTORS]]/[[NIPOPPY_PIPELINE_NAME]]-[[NIPOPPY_PIPELINE_VERSION]]-convert.json", + "DESCRIPTOR_FILE": "[[NIPOPPY_DPATH_DESCRIPTORS]]/dcm2bids-[[NIPOPPY_PIPELINE_VERSION]].json", "CONTAINER_CONFIG": { "ARGS": [ "--bind", "[[DCM2BIDS_CONFIG_FILE]]" ] - } + }, + "UPDATE_DOUGHNUT": true } ] } diff --git a/nipoppy_cli/nipoppy/data/layouts/layout-default.json b/nipoppy_cli/nipoppy/data/layouts/layout-default.json index a4ccbc53..98689342 100644 --- a/nipoppy_cli/nipoppy/data/layouts/layout-default.json +++ b/nipoppy_cli/nipoppy/data/layouts/layout-default.json @@ -80,7 +80,7 @@ "description": "The configuration file for a dataset is where all data processing is configured." }, "fpath_doughnut": { - "path": "scratch/raw_dicom/doughnut.csv", + "path": "scratch/raw_imaging/doughnut.csv", "description": "The doughnut file is generated/updated internally and is used to keep track of the raw data organization and BIDS conversion process." }, "fpath_manifest": { diff --git a/nipoppy_cli/nipoppy/workflows/bids_conversion.py b/nipoppy_cli/nipoppy/workflows/bids_conversion.py index ef00818c..ee62722d 100644 --- a/nipoppy_cli/nipoppy/workflows/bids_conversion.py +++ b/nipoppy_cli/nipoppy/workflows/bids_conversion.py @@ -7,7 +7,7 @@ from pathlib import Path from typing import Optional -from nipoppy.config.pipeline import PipelineConfig +from nipoppy.config.pipeline import BidsPipelineConfig from nipoppy.utils import StrOrPathLike from nipoppy.workflows.runner import PipelineRunner @@ -49,7 +49,7 @@ def dpaths_to_check(self) -> list[Path]: return [] @cached_property - def pipeline_config(self) -> PipelineConfig: + def pipeline_config(self) -> BidsPipelineConfig: """Get the user config for the BIDS conversion software.""" return self.config.get_pipeline_config( self.pipeline_name, @@ -105,5 +105,9 @@ def run_cleanup(self, **kwargs): Specifically: - Write updated doughnut file """ - self.save_tabular_file(self.doughnut, self.layout.fpath_doughnut) + update_doughnut = self.pipeline_config.get_update_doughnut( + step_name=self.pipeline_step + ) + if update_doughnut and not self.simulate: + self.save_tabular_file(self.doughnut, self.layout.fpath_doughnut) return super().run_cleanup(**kwargs) diff --git a/nipoppy_cli/nipoppy/workflows/pipeline.py b/nipoppy_cli/nipoppy/workflows/pipeline.py index c92863d5..1b334abe 100644 --- a/nipoppy_cli/nipoppy/workflows/pipeline.py +++ b/nipoppy_cli/nipoppy/workflows/pipeline.py @@ -17,7 +17,7 @@ BoutiquesConfig, get_boutiques_config_from_descriptor, ) -from nipoppy.config.pipeline import PipelineConfig +from nipoppy.config.pipeline import ProcPipelineConfig from nipoppy.utils import ( BIDS_SESSION_PREFIX, BIDS_SUBJECT_PREFIX, @@ -105,7 +105,7 @@ def dpath_pipeline_bids_db(self) -> Path: ) @cached_property - def pipeline_config(self) -> PipelineConfig: + def pipeline_config(self) -> ProcPipelineConfig: """Get the user config for the pipeline.""" return self.config.get_pipeline_config( self.pipeline_name, self.pipeline_version diff --git a/nipoppy_cli/tests/conftest.py b/nipoppy_cli/tests/conftest.py index 2e1d3634..e2879355 100644 --- a/nipoppy_cli/tests/conftest.py +++ b/nipoppy_cli/tests/conftest.py @@ -54,7 +54,7 @@ ATTR_TO_FPATH_MAP = { **ATTR_TO_REQUIRED_FPATH_MAP, - "fpath_doughnut": "scratch/raw_dicom/doughnut.csv", + "fpath_doughnut": "scratch/raw_imaging/doughnut.csv", "fpath_imaging_bagel": "derivatives/bagel.csv", } diff --git a/nipoppy_cli/tests/test_config_main.py b/nipoppy_cli/tests/test_config_main.py index 64c53fba..f9154cce 100644 --- a/nipoppy_cli/tests/test_config_main.py +++ b/nipoppy_cli/tests/test_config_main.py @@ -9,7 +9,7 @@ from nipoppy.config.container import ContainerConfig from nipoppy.config.main import Config -from nipoppy.config.pipeline import PipelineConfig +from nipoppy.config.pipeline import BasePipelineConfig, ProcPipelineConfig from nipoppy.utils import FPATH_SAMPLE_CONFIG from .conftest import DPATH_TEST_DATA @@ -246,7 +246,7 @@ def test_get_pipeline_version_invalid_name(valid_config_data): def test_get_pipeline_config(pipeline, version, valid_config_data): assert isinstance( Config(**valid_config_data).get_pipeline_config(pipeline, version), - PipelineConfig, + BasePipelineConfig, ) @@ -347,7 +347,7 @@ def test_load_apply_substitutions(valid_config_data, tmp_path: Path): fpath = tmp_path / "config.json" Config(**valid_config_data).save(fpath) config_to_check = Config.load(fpath, apply_substitutions=True) - assert config_to_check.PROC_PIPELINES[0] == PipelineConfig( + assert config_to_check.PROC_PIPELINES[0] == ProcPipelineConfig( **{ "NAME": "fmriprep", "VERSION": "23.1.3", diff --git a/nipoppy_cli/tests/test_config_pipeline.py b/nipoppy_cli/tests/test_config_pipeline.py index fdc3f8ee..d93c03cf 100644 --- a/nipoppy_cli/tests/test_config_pipeline.py +++ b/nipoppy_cli/tests/test_config_pipeline.py @@ -5,18 +5,20 @@ import pytest from pydantic import ValidationError -from nipoppy.config.pipeline import PipelineConfig, PipelineStepConfig +from nipoppy.config.pipeline import BasePipelineConfig, ProcPipelineConfig +from nipoppy.config.pipeline_step import ProcPipelineStepConfig -FIELDS_PIPELINE = [ +FIELDS_PIPELINE_BASE = [ "NAME", "VERSION", "DESCRIPTION", "CONTAINER_INFO", "CONTAINER_CONFIG", "STEPS", - "TRACKER_CONFIG_FILE", ] +FIELDS_PIPELINE_PROC = FIELDS_PIPELINE_BASE + ["TRACKER_CONFIG_FILE"] + @pytest.fixture(scope="function") def valid_data() -> dict: @@ -27,21 +29,32 @@ def valid_data() -> dict: @pytest.mark.parametrize( - "additional_data", + "pipeline_config_class,fields,additional_data_list", [ - {}, - {"DESCRIPTION": "My pipeline"}, - {"CONTAINER_CONFIG": {"ARGS": ["--cleanenv"]}}, - {"STEPS": []}, - {"TRACKER_CONFIG_FILE": "path/to/tracker/config/file"}, + ( + BasePipelineConfig, + FIELDS_PIPELINE_BASE, + [ + {}, + {"DESCRIPTION": "My pipeline"}, + {"CONTAINER_CONFIG": {"ARGS": ["--cleanenv"]}}, + {"STEPS": []}, + ], + ), + ( + ProcPipelineConfig, + FIELDS_PIPELINE_PROC, + [{"TRACKER_CONFIG_FILE": "path/to/tracker/config/file"}], + ), ], ) -def test_fields(valid_data, additional_data): - pipeline_config = PipelineConfig(**valid_data, **additional_data) - for field in FIELDS_PIPELINE: - assert hasattr(pipeline_config, field) +def test_fields(pipeline_config_class, fields, valid_data, additional_data_list): + for additional_data in additional_data_list: + pipeline_config = pipeline_config_class(**valid_data, **additional_data) + for field in fields: + assert hasattr(pipeline_config, field) - assert len(set(pipeline_config.model_fields.keys())) == len(FIELDS_PIPELINE) + assert len(set(pipeline_config.model_fields.keys())) == len(fields) @pytest.mark.parametrize( @@ -50,24 +63,24 @@ def test_fields(valid_data, additional_data): ) def test_fields_missing_required(data): with pytest.raises(ValidationError): - PipelineConfig(**data) + ProcPipelineConfig(**data) def test_fields_no_extra(valid_data): with pytest.raises(ValidationError): - PipelineConfig(**valid_data, not_a_field="a") + ProcPipelineConfig(**valid_data, not_a_field="a") def test_step_names_error_none(valid_data): with pytest.raises( ValidationError, match="Found at least one step with undefined NAME field" ): - PipelineConfig(**valid_data, STEPS=[{}, {}]) + ProcPipelineConfig(**valid_data, STEPS=[{}, {}]) def test_step_names_error_duplicate(valid_data): with pytest.raises(ValidationError, match="Found at least two steps with NAME"): - PipelineConfig( + ProcPipelineConfig( **valid_data, STEPS=[ {"NAME": "step1"}, @@ -78,7 +91,9 @@ def test_step_names_error_duplicate(valid_data): @pytest.mark.parametrize("container", ["my_container.sif", "my_other_container.sif"]) def test_get_fpath_container(valid_data, container): - pipeline_config = PipelineConfig(**valid_data, CONTAINER_INFO={"FILE": container}) + pipeline_config = ProcPipelineConfig( + **valid_data, CONTAINER_INFO={"FILE": container} + ) assert pipeline_config.get_fpath_container() == Path(container) @@ -91,11 +106,11 @@ def test_get_fpath_container(valid_data, container): ], ) def test_get_step_config(valid_data, step_name, expected_name): - pipeling_config = PipelineConfig( + pipeling_config = ProcPipelineConfig( **valid_data, STEPS=[ - PipelineStepConfig(NAME="step1", INVOCATION_FILE="step1.json"), - PipelineStepConfig(NAME="step2", INVOCATION_FILE="step2.json"), + ProcPipelineStepConfig(NAME="step1", INVOCATION_FILE="step1.json"), + ProcPipelineStepConfig(NAME="step2", INVOCATION_FILE="step2.json"), ], ) @@ -103,17 +118,17 @@ def test_get_step_config(valid_data, step_name, expected_name): def test_get_step_config_no_steps(valid_data): - pipeline_config = PipelineConfig(**valid_data, STEPS=[]) + pipeline_config = ProcPipelineConfig(**valid_data, STEPS=[]) with pytest.raises(ValueError, match="No steps specified for pipeline"): pipeline_config.get_step_config() def test_get_step_config_invalid(valid_data): - pipeline_config = PipelineConfig( + pipeline_config = ProcPipelineConfig( **valid_data, STEPS=[ - PipelineStepConfig(NAME="step1", INVOCATION_FILE="step1.json"), - PipelineStepConfig(NAME="step2", INVOCATION_FILE="step2.json"), + ProcPipelineStepConfig(NAME="step1", INVOCATION_FILE="step1.json"), + ProcPipelineStepConfig(NAME="step2", INVOCATION_FILE="step2.json"), ], ) with pytest.raises(ValueError, match="not found in pipeline"): @@ -125,11 +140,11 @@ def test_get_step_config_invalid(valid_data): [("step1", Path("step1.json")), ("step2", Path("step2.json"))], ) def test_get_invocation_file(valid_data, step_name, invocation_file): - pipeline_config = PipelineConfig( + pipeline_config = ProcPipelineConfig( **valid_data, STEPS=[ - PipelineStepConfig(NAME="step1", INVOCATION_FILE="step1.json"), - PipelineStepConfig(NAME="step2", INVOCATION_FILE="step2.json"), + ProcPipelineStepConfig(NAME="step1", INVOCATION_FILE="step1.json"), + ProcPipelineStepConfig(NAME="step2", INVOCATION_FILE="step2.json"), ], ) @@ -141,28 +156,28 @@ def test_get_invocation_file(valid_data, step_name, invocation_file): [("step1", Path("step1.json")), ("step2", Path("step2.json"))], ) def test_get_descriptor_file(valid_data, step_name, descriptor_file): - pipeline_config = PipelineConfig( + pipeline_config = ProcPipelineConfig( **valid_data, STEPS=[ - PipelineStepConfig(NAME="step1", DESCRIPTOR_FILE="step1.json"), - PipelineStepConfig(NAME="step2", DESCRIPTOR_FILE="step2.json"), + ProcPipelineStepConfig(NAME="step1", DESCRIPTOR_FILE="step1.json"), + ProcPipelineStepConfig(NAME="step2", DESCRIPTOR_FILE="step2.json"), ], ) assert pipeline_config.get_descriptor_file(step_name) == descriptor_file -@pytest.mark.parametrize( - "step_name,pybids_ignore_file", - [("step1", Path("patterns1.json")), ("step2", Path("patterns2.json"))], -) -def test_get_pybids_ignore(valid_data, step_name, pybids_ignore_file): - pipeline_config = PipelineConfig( - **valid_data, - STEPS=[ - PipelineStepConfig(NAME="step1", PYBIDS_IGNORE_FILE="patterns1.json"), - PipelineStepConfig(NAME="step2", PYBIDS_IGNORE_FILE="patterns2.json"), - ], - ) - - assert pipeline_config.get_pybids_ignore_file(step_name) == pybids_ignore_file +# @pytest.mark.parametrize( +# "step_name,pybids_ignore_file", +# [("step1", Path("patterns1.json")), ("step2", Path("patterns2.json"))], +# ) +# def test_get_pybids_ignore(valid_data, step_name, pybids_ignore_file): +# pipeline_config = ProcPipelineConfig( +# **valid_data, +# STEPS=[ +# ProcPipelineStepConfig(NAME="step1", PYBIDS_IGNORE_FILE="patterns1.json"), +# ProcPipelineStepConfig(NAME="step2", PYBIDS_IGNORE_FILE="patterns2.json"), +# ], +# ) + +# assert pipeline_config.get_pybids_ignore_file(step_name) == pybids_ignore_file diff --git a/nipoppy_cli/tests/test_config_pipeline_step.py b/nipoppy_cli/tests/test_config_pipeline_step.py index fa52fb4f..f00691d1 100644 --- a/nipoppy_cli/tests/test_config_pipeline_step.py +++ b/nipoppy_cli/tests/test_config_pipeline_step.py @@ -1,37 +1,63 @@ """Tests for the pipeline step configuration class.""" import pytest -from pydantic import ValidationError +from pydantic import BaseModel, ValidationError -from nipoppy.config.pipeline import PipelineStepConfig +from nipoppy.config.pipeline_step import ( + BasePipelineStepConfig, + BidsPipelineStepConfig, + ProcPipelineStepConfig, +) -FIELDS_STEP = [ +FIELDS_STEP_BASE = [ "NAME", "DESCRIPTOR_FILE", "INVOCATION_FILE", - "PYBIDS_IGNORE_FILE", "CONTAINER_CONFIG", ] +FIELDS_STEP_PROC = FIELDS_STEP_BASE + ["PYBIDS_IGNORE_FILE"] +FIELDS_STEP_BIDS = FIELDS_STEP_BASE + ["UPDATE_DOUGHNUT"] + @pytest.mark.parametrize( - "data", + "step_class,fields,data_list", [ - {"NAME": "step_name"}, - {"DESCRIPTOR_FILE": "PATH_TO_DESCRIPTOR_FILE"}, - {"INVOCATION_FILE": "PATH_TO_INVOCATION_FILE"}, - {"PYBIDS_IGNORE_FILE": "PATH_TO_PYBIDS_IGNORE_FILE"}, - {"CONTAINER_CONFIG": {}}, + ( + BasePipelineStepConfig, + FIELDS_STEP_BASE, + [ + {"NAME": "step_name"}, + {"DESCRIPTOR_FILE": "PATH_TO_DESCRIPTOR_FILE"}, + {"INVOCATION_FILE": "PATH_TO_INVOCATION_FILE"}, + {"CONTAINER_CONFIG": {}}, + ], + ), + ( + BidsPipelineStepConfig, + FIELDS_STEP_BIDS, + [{"UPDATE_DOUGHNUT": True}], + ), + ( + ProcPipelineStepConfig, + FIELDS_STEP_PROC, + [{"PYBIDS_IGNORE_FILE": "PATH_TO_PYBIDS_IGNORE_FILE"}], + ), ], ) -def test_field(data): - pipeline_step_config = PipelineStepConfig(**data) - for field in FIELDS_STEP: - assert hasattr(pipeline_step_config, field) +def test_field_base(step_class: type[BaseModel], fields, data_list): + for data in data_list: + pipeline_step_config = step_class(**data) + for field in fields: + assert hasattr(pipeline_step_config, field) - assert len(set(pipeline_step_config.model_fields.keys())) == len(FIELDS_STEP) + assert len(set(pipeline_step_config.model_fields.keys())) == len(fields) -def test_no_extra_field(): +@pytest.mark.parametrize( + "model_class", + [ProcPipelineStepConfig, BidsPipelineStepConfig], +) +def test_no_extra_field(model_class): with pytest.raises(ValidationError, match="Extra inputs are not permitted"): - PipelineStepConfig(not_a_field="a") + model_class(not_a_field="a") diff --git a/nipoppy_cli/tests/test_layout.py b/nipoppy_cli/tests/test_layout.py index e58463ca..3e0c2e24 100644 --- a/nipoppy_cli/tests/test_layout.py +++ b/nipoppy_cli/tests/test_layout.py @@ -295,3 +295,8 @@ def test_get_dpath_bids_db( ) == dpath_root / expected ) + + +def test_doughnut_parent_directory(dpath_root: Path): + layout = DatasetLayout(dpath_root=dpath_root) + assert layout.fpath_doughnut.parent == layout.dpath_raw_imaging diff --git a/nipoppy_cli/tests/test_workflow_bids_conversion.py b/nipoppy_cli/tests/test_workflow_bids_conversion.py index 80938a63..56578e41 100644 --- a/nipoppy_cli/tests/test_workflow_bids_conversion.py +++ b/nipoppy_cli/tests/test_workflow_bids_conversion.py @@ -18,12 +18,18 @@ def config() -> Config: { "NAME": "heudiconv", "VERSION": "0.12.2", - "STEPS": [{"NAME": "prepare"}, {"NAME": "convert"}], + "STEPS": [ + {"NAME": "prepare"}, + {"NAME": "convert", "UPDATE_DOUGHNUT": True}, + ], }, { "NAME": "dcm2bids", "VERSION": "3.1.0", - "STEPS": [{"NAME": "prepare"}, {"NAME": "convert"}], + "STEPS": [ + {"NAME": "prepare"}, + {"NAME": "convert", "UPDATE_DOUGHNUT": True}, + ], }, ] ) @@ -60,14 +66,15 @@ def test_setup(config: Config, tmp_path: Path): ).validate(), ], ) -def test_cleanup(doughnut: Doughnut, tmp_path: Path): +def test_cleanup(doughnut: Doughnut, config: Config, tmp_path: Path): workflow = BidsConversionRunner( dpath_root=tmp_path / "my_dataset", - pipeline_name="", - pipeline_version="", - pipeline_step="", + pipeline_name="heudiconv", + pipeline_version="0.12.2", + pipeline_step="convert", ) workflow.doughnut = doughnut + config.save(workflow.layout.fpath_config) workflow.run_cleanup() @@ -75,6 +82,39 @@ def test_cleanup(doughnut: Doughnut, tmp_path: Path): assert Doughnut.load(workflow.layout.fpath_doughnut).equals(doughnut) +def test_cleanup_simulate(tmp_path: Path, config: Config): + workflow = BidsConversionRunner( + dpath_root=tmp_path / "my_dataset", + pipeline_name="heudiconv", + pipeline_version="0.12.2", + pipeline_step="convert", + simulate=True, + ) + workflow.doughnut = Doughnut() + config.save(workflow.layout.fpath_config) + + workflow.run_cleanup() + + assert not workflow.layout.fpath_doughnut.exists() + + +def test_cleanup_no_doughnut_update(config: Config, tmp_path: Path): + workflow = BidsConversionRunner( + dpath_root=tmp_path / "my_dataset", + pipeline_name="heudiconv", + pipeline_version="0.12.2", + pipeline_step="prepare", + ) + workflow.doughnut = Doughnut() + config.save(workflow.layout.fpath_config) + + print(config.get_pipeline_config("heudiconv", "0.12.2")) + + workflow.run_cleanup() + + assert not workflow.layout.fpath_doughnut.exists() + + @pytest.mark.parametrize( "doughnut_data,participant_id,session_id,expected", [ diff --git a/nipoppy_cli/tests/test_workflow_pipeline.py b/nipoppy_cli/tests/test_workflow_pipeline.py index 5db46381..5f0535df 100644 --- a/nipoppy_cli/tests/test_workflow_pipeline.py +++ b/nipoppy_cli/tests/test_workflow_pipeline.py @@ -10,8 +10,8 @@ from fids import fids from nipoppy.config.boutiques import BoutiquesConfig -from nipoppy.config.pipeline import PipelineConfig -from nipoppy.config.pipeline_step import PipelineStepConfig +from nipoppy.config.pipeline import ProcPipelineConfig +from nipoppy.config.pipeline_step import ProcPipelineStepConfig from nipoppy.utils import StrOrPathLike from nipoppy.workflows.pipeline import BasePipelineWorkflow @@ -168,7 +168,7 @@ def test_pipeline_version_optional(): def test_pipeline_config(workflow: PipelineWorkflow): - assert isinstance(workflow.pipeline_config, PipelineConfig) + assert isinstance(workflow.pipeline_config, ProcPipelineConfig) def test_fpath_container(workflow: PipelineWorkflow): @@ -243,10 +243,10 @@ def test_descriptor_substitutions( # set descriptor file and write descriptor content fpath_descriptor = tmp_path / "custom_pipeline.json" - workflow.pipeline_config = PipelineConfig( + workflow.pipeline_config = ProcPipelineConfig( NAME=workflow.pipeline_name, VERSION=workflow.pipeline_version, - STEPS=[PipelineStepConfig(DESCRIPTOR_FILE=fpath_descriptor)], + STEPS=[ProcPipelineStepConfig(DESCRIPTOR_FILE=fpath_descriptor)], ) fpath_descriptor.write_text(json.dumps({"key1": "[[TO_REPLACE1]]"})) @@ -298,10 +298,10 @@ def test_invocation_substitutions( # set invocation file and write invocation content fpath_invocation = tmp_path / "invocation.json" - workflow.pipeline_config = PipelineConfig( + workflow.pipeline_config = ProcPipelineConfig( NAME=workflow.pipeline_name, VERSION=workflow.pipeline_version, - STEPS=[PipelineStepConfig(INVOCATION_FILE=fpath_invocation)], + STEPS=[ProcPipelineStepConfig(INVOCATION_FILE=fpath_invocation)], ) fpath_invocation.write_text(json.dumps({"key1": "[[TO_REPLACE1]]"}))