diff --git a/snakebids/app.py b/snakebids/app.py index 3f313009..eb682ef3 100644 --- a/snakebids/app.py +++ b/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: diff --git a/snakebids/core/input_generation.py b/snakebids/core/input_generation.py index a6db389c..77c84f24 100644 --- a/snakebids/core/input_generation.py +++ b/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,18 +261,9 @@ 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 = ( @@ -274,8 +271,8 @@ def generate_inputs( 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()) diff --git a/snakebids/tests/test_app.py b/snakebids/tests/test_app.py index 41acf057..cfb52c71 100644 --- a/snakebids/tests/test_app.py +++ b/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 diff --git a/snakebids/tests/test_generate_inputs.py b/snakebids/tests/test_generate_inputs.py index f0cda0a9..c6cd033d 100644 --- a/snakebids/tests/test_generate_inputs.py +++ b/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( [ diff --git a/snakebids/utils/utils.py b/snakebids/utils/utils.py index 310394c1..4ee5d9cf 100644 --- a/snakebids/utils/utils.py +++ b/snakebids/utils/utils.py @@ -50,6 +50,10 @@ class BidsTag(TypedDict): BidsTags: TypeAlias = "dict[str, BidsTag]" +DEPRECATION_FLAG = "" +"""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.