Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port forward 0.9.x changes into 0.10.x #345

Merged
merged 9 commits into from
Nov 30, 2023
Prev Previous commit
Next Next commit
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
pvandyken committed Sep 15, 2023

Verified

This commit was signed with the committer’s verified signature.
pauldambra Paul D'Ambra
commit 3a25e9974d5ef13d63b9b84c84155d3023b40ce4
8 changes: 7 additions & 1 deletion snakebids/app.py
Original file line number Diff line number Diff line change
@@ -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:
95 changes: 77 additions & 18 deletions snakebids/core/input_generation.py
Original file line number Diff line number Diff line change
@@ -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())
33 changes: 33 additions & 0 deletions snakebids/tests/test_app.py
Original file line number Diff line number Diff line change
@@ -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
117 changes: 116 additions & 1 deletion snakebids/tests/test_generate_inputs.py
Original file line number Diff line number Diff line change
@@ -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(
[
4 changes: 4 additions & 0 deletions snakebids/utils/utils.py
Original file line number Diff line number Diff line change
@@ -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.