From 3a25e9974d5ef13d63b9b84c84155d3023b40ce4 Mon Sep 17 00:00:00 2001 From: Peter Van Dyken Date: Thu, 24 Aug 2023 16:53:27 -0400 Subject: [PATCH] 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 | 8 +- snakebids/core/input_generation.py | 95 +++++++++++++++---- snakebids/tests/test_app.py | 33 +++++++ snakebids/tests/test_generate_inputs.py | 117 +++++++++++++++++++++++- snakebids/utils/utils.py | 4 + 5 files changed, 237 insertions(+), 20 deletions(-) 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.