Add deprecation mechanism to old pybidsdb vals

Pybidsdb values encoded in the config file had their key names changed
without warning in snakebids==0.9.0. This caused apps relying on those
configuration names to silently fail to read the database

With this fix, once again set the keys with parsable sentinal values so
that reliant workflows will still work but maintainers will be alerted
to the deprecation

Resolves #333

snakebids/app.py

@@ -28,7 +28,7 @@
 from snakebids.types import OptionalFilter
 from snakebids.utils.output import write_config_file  # type: ignore
 from snakebids.utils.output import prepare_bidsapp_output, write_output_mode
-from snakebids.utils.utils import to_resolved_path
+from snakebids.utils.utils import DEPRECATION_FLAG, to_resolved_path
 
 logger = logging.Logger(__name__)
 
@@ -184,6 +184,12 @@ def run_snakemake(self) -> None:
         # Update config with pybids settings
         self.config["pybidsdb_dir"] = self.args.pybidsdb_dir
         self.config["pybidsdb_reset"] = self.args.pybidsdb_reset
+        self.config[
+            "pybids_db_dir"
+        ] = f"{DEPRECATION_FLAG}{self.args.pybidsdb_dir}{DEPRECATION_FLAG}"
+        self.config[
+            "pybids_db_reset"
+        ] = f"{DEPRECATION_FLAG}{int(self.args.pybidsdb_reset)}{DEPRECATION_FLAG}"
 
         # First, handle outputs in snakebids_root or results folder
         try:

snakebids/core/input_generation.py

@@ -6,6 +6,7 @@
 import os
 import re
 import sys
+import warnings
 from collections import defaultdict
 from pathlib import Path
 from typing import Any, Generator, Iterable, Mapping, Optional, Sequence, overload
@@ -27,7 +28,12 @@
 )
 from snakebids.types import InputsConfig, ZipList
 from snakebids.utils.snakemake_io import glob_wildcards
-from snakebids.utils.utils import BidsEntity, BidsParseError, get_first_dir
+from snakebids.utils.utils import (
+    DEPRECATION_FLAG,
+    BidsEntity,
+    BidsParseError,
+    get_first_dir,
+)
 
 _logger = logging.getLogger(__name__)
 
@@ -74,7 +80,7 @@ def generate_inputs(
     bids_dir: Path | str,
     pybids_inputs: InputsConfig,
     pybidsdb_dir: Path | str | None = None,
-    pybidsdb_reset: bool = False,
+    pybidsdb_reset: bool | None = None,
     derivatives: bool | Path | str = False,
     pybids_config: str | None = None,
     limit_to: Iterable[str] | None = None,
@@ -83,7 +89,7 @@ def generate_inputs(
     use_bids_inputs: bool | None = None,
     validate: bool = False,
     pybids_database_dir: Path | str | None = None,
-    pybids_reset_database: bool = False,
+    pybids_reset_database: bool | None = None,
 ) -> BidsDataset | BidsDatasetDict:
     """Dynamically generate snakemake inputs using pybids_inputs
 
@@ -152,7 +158,7 @@ def generate_inputs(
         the default behaviour
 
     validate
-        If True performs validation of BIDS directory using pybids, otherwise 
+        If True performs validation of BIDS directory using pybids, otherwise
         skips validation.
 
     Returns
@@ -255,27 +261,18 @@ def generate_inputs(
         participant_label, exclude_participant_label
     )
 
-    if pybids_database_dir:
-        _logger.warning(
-            "The parameter `pybids_database_dir` in generate_inputs() is deprecated "
-            "and will be removed in the next release. To set the pybids database, use "
-            "the `pybidsdb_dir` parameter instead."
-        )
-    if pybids_reset_database:
-        _logger.warning(
-            "The parameter `pybids_reset_database` in generate_inputs() is deprecated "
-            "and will be removed in the next release. To reset the pybids database, "
-            "use the `pybidsdb_reset` parameter instead."
-        )
+    pybidsdb_dir, pybidsdb_reset = _normalize_database_args(
+        pybidsdb_dir, pybidsdb_reset, pybids_database_dir, pybids_reset_database
+    )
 
     # Generates a BIDSLayout
     layout = (
         _gen_bids_layout(
             bids_dir=bids_dir,
             derivatives=derivatives,
             pybids_config=pybids_config,
-            pybidsdb_dir=pybidsdb_dir or pybids_database_dir,
-            pybidsdb_reset=pybidsdb_reset or pybids_reset_database,
+            pybidsdb_dir=pybidsdb_dir,
+            pybidsdb_reset=pybidsdb_reset,
             validate=validate,
         )
         if not _all_custom_paths(pybids_inputs)
@@ -312,6 +309,68 @@ def generate_inputs(
     return dataset.as_dict
 
 
+def _normalize_database_args(
+    pybidsdb_dir: Path | str | None,
+    pybidsdb_reset: bool | str | None,
+    pybids_database_dir: Path | str | None,
+    pybids_reset_database: str | bool | None,
+) -> tuple[Path | str | None, bool]:
+    """This function exists to help handle deprecation for pybidsdb"""
+    if pybids_database_dir is not None:
+        warnings.warn(
+            "The parameter `pybids_database_dir` in generate_inputs() is deprecated "
+            "and will be removed in the next release. To set the pybids database, use "
+            "the `pybidsdb_dir` parameter instead."
+        )
+    if pybids_reset_database is not None:
+        warnings.warn(
+            "The parameter `pybids_reset_database` in generate_inputs() is deprecated "
+            "and will be removed in the next release. To reset the pybids database, "
+            "use the `pybidsdb_reset` parameter instead."
+        )
+
+    pybidsdb_dir = pybidsdb_dir or pybids_database_dir
+    pybidsdb_reset = (
+        pybidsdb_reset
+        if pybidsdb_reset is not None
+        else pybids_reset_database
+        if pybids_reset_database is not None
+        else False
+    )
+
+    depr_len = len(DEPRECATION_FLAG)
+    if (
+        pybidsdb_dir is not None
+        and str(pybidsdb_dir).startswith(DEPRECATION_FLAG)
+        and str(pybidsdb_dir).endswith(DEPRECATION_FLAG)
+    ):
+        warnings.warn(
+            "The config value `pybids_db_dir` is deprecated and will be removed in a "
+            "future release. To access a CLI-specified pybids database directory, use "
+            '`config.get("pybidsdb_dir")` instead.'
+        )
+        pybidsdb_dir = str(pybidsdb_dir)[depr_len:-depr_len]
+    try:
+        if (
+            isinstance(pybidsdb_reset, str)
+            and pybidsdb_reset.startswith(DEPRECATION_FLAG)
+            and pybidsdb_reset.endswith(DEPRECATION_FLAG)
+        ):
+            pybidsdb_reset = bool(int(pybidsdb_reset[depr_len:-depr_len]))
+            warnings.warn(
+                "The config value `pybids_db_reset` is deprecated and will be removed "
+                "in a future release. To access CLI-specified pybids database reset "
+                'instructions, use `config.get("pybidsdb_reset")` instead.'
+            )
+    except ValueError:
+        raise TypeError("pybidsdb_reset must be a boolean")
+
+    if not isinstance(pybidsdb_reset, bool):
+        raise TypeError("pybidsdb_reset must be a boolean")
+
+    return pybidsdb_dir, pybidsdb_reset
+
+
 def _all_custom_paths(config: InputsConfig):
     """Check that all input components have a custom path"""
     return all(comp.get("custom_path") for comp in config.values())

snakebids/tests/test_app.py

@@ -16,6 +16,7 @@
 from snakebids.cli import SnakebidsArgs
 from snakebids.tests import strategies as sb_st
 from snakebids.types import InputConfig, InputsConfig, OptionalFilter
+from snakebids.utils.utils import DEPRECATION_FLAG
 
 from .. import app as sn_app
 from ..app import SnakeBidsApp
@@ -181,6 +182,8 @@ def test_runs_in_correct_mode(
                 "snakemake_dir": Path("app").resolve(),
                 "pybidsdb_dir": Path("/tmp/output/.db"),
                 "pybidsdb_reset": True,
+                "pybids_db_dir": f"{DEPRECATION_FLAG}/tmp/output/.db{DEPRECATION_FLAG}",
+                "pybids_db_reset": f"{DEPRECATION_FLAG}1{DEPRECATION_FLAG}",
                 "snakefile": Path("Snakefile"),
                 "output_dir": outputdir.resolve(),
                 "snakemake_version": metadata.version("snakemake"),
@@ -274,6 +277,36 @@ def test_pybidsdb_path_resolved(self, mocker: MockerFixture):
 
         assert app.config["pybidsdb_dir"] == Path(".pybids").resolve()
 
+    def test_old_db_args_deprecated(self, mocker: MockerFixture):
+        self.io_mocks(mocker)
+        mocker.patch.object(
+            sys,
+            "argv",
+            ["./run.sh", "input", "output", "participant", "--pybidsdb-dir", ".pybids"],
+        )
+
+        # Prepare app and initial config values
+        app = SnakeBidsApp(
+            Path("app"),
+            skip_parse_args=False,
+            snakefile_path=Path("Snakefile"),
+            configfile_path=Path("mock/config.yaml"),
+            config=copy.deepcopy(config),
+        )
+
+        # Prepare expected config
+        try:
+            app.run_snakemake()
+        except SystemExit as e:
+            print("System exited prematurely")
+            print(e)
+
+        assert (
+            app.config["pybids_db_dir"]
+            == f"{DEPRECATION_FLAG}{Path('.pybids').resolve()}{DEPRECATION_FLAG}"
+        )
+        assert app.config["pybids_db_reset"] == f"{DEPRECATION_FLAG}0{DEPRECATION_FLAG}"
+
     def test_plugin_args(self, mocker: MockerFixture, app: SnakeBidsApp):
         """Test that plugins have access to args parsed from the CLI."""
         # Get mocks for all the io functions

snakebids/tests/test_generate_inputs.py

@@ -10,6 +10,7 @@
 import shutil
 import sys
 import tempfile
+import warnings
 from collections import defaultdict
 from pathlib import Path
 from typing import (
@@ -40,6 +41,7 @@
     _gen_bids_layout,
     _generate_filters,
     _get_lists_from_bids,
+    _normalize_database_args,
     _parse_bids_path,
     _parse_custom_path,
     generate_inputs,
@@ -56,11 +58,117 @@
     reindex_dataset,
 )
 from snakebids.types import InputsConfig
-from snakebids.utils.utils import BidsEntity, BidsParseError, MultiSelectDict
+from snakebids.utils.utils import (
+    DEPRECATION_FLAG,
+    BidsEntity,
+    BidsParseError,
+    MultiSelectDict,
+)
 
 T = TypeVar("T")
 
 
+def _not_deprecated(s: str):
+    return not (s.startswith(DEPRECATION_FLAG) and s.endswith(DEPRECATION_FLAG))
+
+
+class TestNormalizeDatabaseArgs:
+    @given(
+        pybidsdb_dir=st.text().filter(_not_deprecated) | st.none(),
+        pybidsdb_reset=st.booleans(),
+    )
+    def test_normal_calls_give_no_warnings(
+        self, pybidsdb_dir: str | None, pybidsdb_reset: bool
+    ):
+        with warnings.catch_warnings():
+            warnings.simplefilter("error")
+            _normalize_database_args(pybidsdb_dir, pybidsdb_reset, None, None)
+
+    @given(
+        pybidsdb_dir=st.text().filter(_not_deprecated) | st.none(),
+        pybidsdb_reset=st.booleans(),
+        pybids_database_dir=st.text().filter(_not_deprecated),
+    )
+    def test_old_dir_param_gives_warning(
+        self, pybidsdb_dir: str | None, pybidsdb_reset: bool, pybids_database_dir: str
+    ):
+        with pytest.warns(UserWarning, match="`pybids_database_dir`"):
+            _normalize_database_args(
+                pybidsdb_dir, pybidsdb_reset, pybids_database_dir, None
+            )
+
+    @given(
+        pybidsdb_dir=st.text().filter(_not_deprecated) | st.none(),
+        pybidsdb_reset=st.booleans(),
+        pybids_database_reset=st.booleans(),
+    )
+    def test_old_reset_param_gives_warning(
+        self,
+        pybidsdb_dir: str | None,
+        pybidsdb_reset: bool,
+        pybids_database_reset: bool,
+    ):
+        with pytest.warns(UserWarning, match="`pybids_reset_database`"):
+            _normalize_database_args(
+                pybidsdb_dir, pybidsdb_reset, None, pybids_database_reset
+            )
+
+    @given(
+        pybids_database_dir=st.text().filter(_not_deprecated),
+        pybids_database_reset=st.booleans(),
+    )
+    def test_old_params_passed_on_to_new(
+        self,
+        pybids_database_dir: str,
+        pybids_database_reset: bool,
+    ):
+        assert (pybids_database_dir, pybids_database_reset) == _normalize_database_args(
+            None, None, pybids_database_dir, pybids_database_reset
+        )
+
+    @given(
+        pybidsdb_reset=st.booleans() | st.none(),
+        pybids_database_reset=st.booleans() | st.none(),
+    )
+    def test_second_return_never_none(
+        self,
+        pybidsdb_reset: bool | None,
+        pybids_database_reset: bool | None,
+    ):
+        assert (
+            _normalize_database_args(
+                None,
+                pybidsdb_reset,
+                None,
+                pybids_database_reset,
+            )[1]
+            is not None
+        )
+
+    @given(pybidsdb_dir=st.text().filter(_not_deprecated))
+    def test_deprecated_dir_raises_warning(self, pybidsdb_dir: str):
+        pybidsdb_dir_ = DEPRECATION_FLAG + pybidsdb_dir + DEPRECATION_FLAG
+        with pytest.warns(UserWarning, match="`pybids_db_dir`"):
+            assert (
+                _normalize_database_args(pybidsdb_dir_, None, None, None)[0]
+                == pybidsdb_dir
+            )
+
+    @given(pybidsdb_reset=st.booleans())
+    def test_deprecated_reset_raises_warning(self, pybidsdb_reset: bool):
+        pybidsdb_reset_ = DEPRECATION_FLAG + str(int(pybidsdb_reset)) + DEPRECATION_FLAG
+        with pytest.warns(UserWarning, match="`pybids_db_reset`"):
+            assert (
+                _normalize_database_args(None, pybidsdb_reset_, None, None)[1]
+                == pybidsdb_reset
+            )
+
+    @given(pybidsdb_reset=st.text().filter(_not_deprecated))
+    def test_non_deprecated_text_in_reset_raises_error(self, pybidsdb_reset: bool):
+        with pytest.raises(TypeError):
+            _normalize_database_args(None, pybidsdb_reset, None, None)
+
+
 class TestFilterBools:
     @pytest.fixture(autouse=True)
     def bids_fs(self, bids_fs: Optional[FakeFilesystem]):
@@ -284,6 +392,11 @@ def add_entity(component: BidsComponent, entity: str, value: str):
         target = data.draw(sb_st.bids_value(entity.match))
         decoy = data.draw(sb_st.bids_value(entity.match))
         assume(target != decoy)
+        # This is a brute-force way of dealing with the fact that values for `run` are
+        # converted into int by pybids before querying. So just prevent the decoy from
+        # ever looking like the same number as target
+        if target.isdecimal() and decoy.isdecimal():
+            assume(int(target) != int(decoy))
         dataset = BidsDataset.from_iterable(
             [
                 attrs.evolve(
@@ -368,6 +481,8 @@ def add_entity(component: BidsComponent, entity: str, value: str):
         target = data.draw(sb_st.bids_value(entity.match))
         decoy = data.draw(sb_st.bids_value(entity.match))
         assume(target != decoy)
+        if target.isdecimal() and decoy.isdecimal():
+            assume(int(target) != int(decoy))
         root = tempfile.mkdtemp(dir=tmpdir)
         dataset = BidsDataset.from_iterable(
             [

snakebids/utils/utils.py

@@ -50,6 +50,10 @@ class BidsTag(TypedDict):
 BidsTags: TypeAlias = "dict[str, BidsTag]"
 
 
+DEPRECATION_FLAG = "<!DEPRECATED!>"
+"""Sentinel string to mark deprecated config features"""
+
+
 @ft.lru_cache(None)
 def read_bids_tags(bids_json: Path | None = None) -> BidsTags:
     """Read the bids tags we are aware of from a JSON file.