[FIX] Fix doughnut path and other bugs (nipoppy#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

@@ -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

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

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,21 +138,44 @@ 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")
     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:

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.

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,12 +29,29 @@ 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=(
             "Path to file containing a list of regex patterns (strings) to ignore "
             "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")

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
                 }
             ]
         }

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
                 }
             ]
         }

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": {

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)

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

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",
 }