Fix/datalad cache #199
4 changed files with 108 additions and 25 deletions
|
|
@ -71,6 +71,8 @@ Bugs
|
||||||
|
|
||||||
- Fix a bug in which :func:`junifer.stats.count` will not be correctly applied across an axis (:gh:`195` by `Fede Raimondo`_).
|
- Fix a bug in which :func:`junifer.stats.count` will not be correctly applied across an axis (:gh:`195` by `Fede Raimondo`_).
|
||||||
|
|
||||||
|
- Fix an issue with datalad cache and locks in which the overriden settings in Junifer were not propagated to subprocesses, resulting in using the default settings (:gh:`199` by `Fede Raimondo`_).
|
||||||
|
|
||||||
API changes
|
API changes
|
||||||
~~~~~~~~~~~
|
~~~~~~~~~~~
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -74,6 +74,45 @@ def _parse_elements(element: str, config: Dict) -> Union[List, None]:
|
||||||
return elements
|
return elements
|
||||||
|
It's just an invalid string value that is not a valid option and can't be casted to int. We need to continue and then raise the BadParameter. There's no reason to log/warn here. It's a dead-end. It's just an invalid string value that is not a valid option and can't be casted to int. We need to continue and then raise the BadParameter. There's no reason to log/warn here. It's a dead-end.
I would put the I would put the `return` in an `else` block then and put a comment so that we know why it's like that.
I'll add the comment, but I do not like the I'll add the comment, but I do not like the `else` block to contain just the return as it complicates reading.
Fair enough. It's how Python docs explain one to write it which makes sense to me. Fair enough. It's how Python docs explain one to write it which makes sense to me.
|
|||||||
|
|
||||||
|
|
||||||
|
def _validate_verbose(
|
||||||
|
ctx: click.Context, param: str, value: str
|
||||||
|
) -> Union[str, int]:
|
||||||
|
"""Validate verbose option.
|
||||||
|
|
||||||
|
Parameters
|
||||||
|
----------
|
||||||
|
ctx : click.Context
|
||||||
|
The context of the command.
|
||||||
|
param : str
|
||||||
|
The parameter to validate.
|
||||||
|
value : str
|
||||||
|
The value to validate.
|
||||||
|
|
||||||
|
Returns
|
||||||
|
-------
|
||||||
|
str or int
|
||||||
|
The validated value.
|
||||||
|
"""
|
||||||
|
if isinstance(value, int):
|
||||||
|
return value
|
||||||
|
|
||||||
|
valid_strings = ["error", "warning", "info", "debug"]
|
||||||
|
if isinstance(value, str) and value.lower() in valid_strings:
|
||||||
|
return value.upper()
|
||||||
|
|
||||||
|
try:
|
||||||
|
value = int(value) # type: ignore
|
||||||
|
return value
|
||||||
|
except ValueError:
|
||||||
|
# If we get here, the value is not a valid integer.
|
||||||
|
pass
|
||||||
|
|
||||||
|
# If we get here, the value is not valid.
|
||||||
|
raise click.BadParameter(
|
||||||
|
f"verbose must be one of {valid_strings} or an integer"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
@click.group()
|
@click.group()
|
||||||
def cli() -> None: # pragma: no cover
|
def cli() -> None: # pragma: no cover
|
||||||
"""CLI for JUelich NeuroImaging FEature extractoR."""
|
"""CLI for JUelich NeuroImaging FEature extractoR."""
|
||||||
|
|
@ -90,10 +129,11 @@ def cli() -> None: # pragma: no cover
|
||||||
@click.option(
|
@click.option(
|
||||||
"-v",
|
"-v",
|
||||||
"--verbose",
|
"--verbose",
|
||||||
type=click.Choice(["warning", "info", "debug"], case_sensitive=False),
|
type=click.UNPROCESSED,
|
||||||
|
callback=_validate_verbose,
|
||||||
default="info",
|
default="info",
|
||||||
)
|
)
|
||||||
def run(filepath: click.Path, element: str, verbose: click.Choice) -> None:
|
def run(filepath: click.Path, element: str, verbose: Union[str, int]) -> None:
|
||||||
"""Run command for CLI.
|
"""Run command for CLI.
|
||||||
\f
|
\f
|
||||||
Parameters
|
Parameters
|
||||||
|
|
@ -106,7 +146,7 @@ def run(filepath: click.Path, element: str, verbose: click.Choice) -> None:
|
||||||
The verbosity level: warning, info or debug (default "info").
|
The verbosity level: warning, info or debug (default "info").
|
||||||
|
|
||||||
"""
|
"""
|
||||||
configure_logging(level=str(verbose).upper())
|
configure_logging(level=verbose)
|
||||||
# TODO: add validation
|
# TODO: add validation
|
||||||
config = parse_yaml(filepath) # type: ignore
|
config = parse_yaml(filepath) # type: ignore
|
||||||
workdir = config["workdir"]
|
workdir = config["workdir"]
|
||||||
|
|
@ -136,10 +176,11 @@ def run(filepath: click.Path, element: str, verbose: click.Choice) -> None:
|
||||||
@click.option(
|
@click.option(
|
||||||
"-v",
|
"-v",
|
||||||
"--verbose",
|
"--verbose",
|
||||||
type=click.Choice(["warning", "info", "debug"], case_sensitive=False),
|
type=click.UNPROCESSED,
|
||||||
|
callback=_validate_verbose,
|
||||||
default="info",
|
default="info",
|
||||||
)
|
)
|
||||||
def collect(filepath: click.Path, verbose: click.Choice) -> None:
|
def collect(filepath: click.Path, verbose: Union[str, int]) -> None:
|
||||||
"""Collect command for CLI.
|
"""Collect command for CLI.
|
||||||
\f
|
\f
|
||||||
Parameters
|
Parameters
|
||||||
|
|
@ -150,7 +191,7 @@ def collect(filepath: click.Path, verbose: click.Choice) -> None:
|
||||||
The verbosity level: warning, info or debug (default "info").
|
The verbosity level: warning, info or debug (default "info").
|
||||||
|
|
||||||
"""
|
"""
|
||||||
configure_logging(level=str(verbose).upper())
|
configure_logging(level=verbose)
|
||||||
# TODO: add validation
|
# TODO: add validation
|
||||||
config = parse_yaml(filepath) # type: ignore
|
config = parse_yaml(filepath) # type: ignore
|
||||||
storage = config["storage"]
|
storage = config["storage"]
|
||||||
|
|
@ -171,7 +212,8 @@ def collect(filepath: click.Path, verbose: click.Choice) -> None:
|
||||||
@click.option(
|
@click.option(
|
||||||
"-v",
|
"-v",
|
||||||
"--verbose",
|
"--verbose",
|
||||||
type=click.Choice(["warning", "info", "debug"], case_sensitive=False),
|
type=click.UNPROCESSED,
|
||||||
|
callback=_validate_verbose,
|
||||||
default="info",
|
default="info",
|
||||||
)
|
)
|
||||||
def queue(
|
def queue(
|
||||||
|
|
@ -179,7 +221,7 @@ def queue(
|
||||||
element: str,
|
element: str,
|
||||||
overwrite: bool,
|
overwrite: bool,
|
||||||
submit: bool,
|
submit: bool,
|
||||||
verbose: click.Choice,
|
verbose: Union[str, int],
|
||||||
) -> None:
|
) -> None:
|
||||||
"""Queue command for CLI.
|
"""Queue command for CLI.
|
||||||
\f
|
\f
|
||||||
|
|
@ -197,7 +239,7 @@ def queue(
|
||||||
The verbosity level: warning, info or debug (default "info").
|
The verbosity level: warning, info or debug (default "info").
|
||||||
|
|
||||||
"""
|
"""
|
||||||
configure_logging(level=str(verbose).upper())
|
configure_logging(level=verbose)
|
||||||
# TODO: add validation
|
# TODO: add validation
|
||||||
config = parse_yaml(filepath) # type: ignore
|
config = parse_yaml(filepath) # type: ignore
|
||||||
elements = _parse_elements(element, config)
|
elements = _parse_elements(element, config)
|
||||||
|
|
|
||||||
|
|
@ -6,6 +6,7 @@
|
||||||
# License: AGPL
|
# License: AGPL
|
||||||
|
|
||||||
import atexit
|
import atexit
|
||||||
|
import os
|
||||||
import shutil
|
import shutil
|
||||||
import tempfile
|
import tempfile
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
|
|
@ -13,6 +14,7 @@ from typing import Dict, Optional, Tuple, Union
|
||||||
|
|
||||||
import datalad
|
import datalad
|
||||||
import datalad.api as dl
|
import datalad.api as dl
|
||||||
|
from datalad.support.exceptions import IncompleteResultsError
|
||||||
from datalad.support.gitrepo import GitRepo
|
from datalad.support.gitrepo import GitRepo
|
||||||
|
|
||||||
from ..api.decorators import register_datagrabber
|
from ..api.decorators import register_datagrabber
|
||||||
|
|
@ -82,21 +84,10 @@ class DataladDataGrabber(BaseDataGrabber):
|
||||||
sockets_dir.mkdir(parents=True, exist_ok=False)
|
sockets_dir.mkdir(parents=True, exist_ok=False)
|
||||||
locks_dir.mkdir(parents=True, exist_ok=False)
|
locks_dir.mkdir(parents=True, exist_ok=False)
|
||||||
logger.debug(f"Setting datalad cache to {cache_dir}")
|
logger.debug(f"Setting datalad cache to {cache_dir}")
|
||||||
datalad.cfg.set(
|
os.environ["DATALAD_LOCATIONS_CACHE"] = cache_dir.as_posix()
|
||||||
"datalad.locations.cache",
|
os.environ["DATALAD_LOCATIONS_SOCKETS"] = sockets_dir.as_posix()
|
||||||
cache_dir.as_posix(),
|
os.environ["DATALAD_LOCATIONS_LOCKS"] = locks_dir.as_posix()
|
||||||
scope="override",
|
datalad.cfg.reload()
|
||||||
)
|
|
||||||
datalad.cfg.set(
|
|
||||||
"datalad.locations.sockets",
|
|
||||||
sockets_dir.as_posix(),
|
|
||||||
scope="override",
|
|
||||||
)
|
|
||||||
datalad.cfg.set(
|
|
||||||
"datalad.locations.locks",
|
|
||||||
locks_dir.as_posix(),
|
|
||||||
scope="override",
|
|
||||||
)
|
|
||||||
logger.debug(
|
logger.debug(
|
||||||
"Datalad cache set to "
|
"Datalad cache set to "
|
||||||
f"{datalad.cfg.get('datalad.locations.cache')}"
|
f"{datalad.cfg.get('datalad.locations.cache')}"
|
||||||
|
|
@ -184,7 +175,12 @@ class DataladDataGrabber(BaseDataGrabber):
|
||||||
for fname in to_get:
|
for fname in to_get:
|
||||||
logger.debug(f"\t: {fname}")
|
logger.debug(f"\t: {fname}")
|
||||||
|
|
||||||
dl_out = self._dataset.get(to_get, result_renderer="disabled")
|
try:
|
||||||
|
dl_out = self._dataset.get(to_get, result_renderer="disabled")
|
||||||
|
except IncompleteResultsError as e:
|
||||||
|
raise_error(
|
||||||
|
f"Failed to get from dataset: {e.failed}"
|
||||||
|
)
|
||||||
if not self._was_cloned:
|
if not self._was_cloned:
|
||||||
# If the dataset was already installed, check that the
|
# If the dataset was already installed, check that the
|
||||||
# file was actually downloaded to avoid removing a
|
# file was actually downloaded to avoid removing a
|
||||||
|
|
|
||||||
|
|
@ -160,6 +160,49 @@ def test_datalad_clone_cleanup(
|
||||||
assert len(list(datadir.glob("*"))) == 0
|
assert len(list(datadir.glob("*"))) == 0
|
||||||
|
|
||||||
|
|
||||||
|
def test_datalad_clone_create_cleanup(concrete_datagrabber: Type) -> None:
|
||||||
|
Maybe have the type as the base class for datalad stuff? Maybe have the type as the base class for datalad stuff?
I don't get it. This is a copy/paste from the other tests I don't get it. This is a copy/paste from the other tests
Ah yes I remember why it's like that. My bad, you can keep it like that. Ah yes I remember why it's like that. My bad, you can keep it like that.
|
|||||||
|
"""Test datalad base tempdir clone and remove.
|
||||||
|
|
||||||
|
Parameters
|
||||||
|
----------
|
||||||
|
concrete_datagrabber : DataladDataGrabber
|
||||||
|
A concrete datagrabber class to use.
|
||||||
|
"""
|
||||||
|
|
||||||
|
# Clone whole dataset
|
||||||
|
uri = _testing_dataset["example_bids"]["uri"]
|
||||||
|
with concrete_datagrabber(datadir=None, uri=uri) as dg:
|
||||||
|
datadir = dg._tmpdir / "datadir"
|
||||||
|
elem1_bold = (
|
||||||
|
datadir / "example_bids/sub-01/func/sub-01_task-rest_bold.nii.gz"
|
||||||
|
)
|
||||||
|
elem1_t1w = datadir / "example_bids/sub-01/anat/sub-01_T1w.nii.gz"
|
||||||
|
|
||||||
|
assert elem1_bold.is_file() is False
|
||||||
|
assert elem1_t1w.is_file() is False
|
||||||
|
assert datadir.exists() is True
|
||||||
|
assert dg._was_cloned is True
|
||||||
|
assert elem1_bold.is_file() is False
|
||||||
|
assert elem1_bold.is_symlink() is True
|
||||||
|
assert elem1_t1w.is_file() is False
|
||||||
|
assert elem1_t1w.is_symlink() is True
|
||||||
|
elem1 = dg["sub-01"]
|
||||||
|
assert "meta" in elem1["BOLD"]
|
||||||
|
meta = elem1["BOLD"]["meta"]
|
||||||
|
assert "datagrabber" in meta
|
||||||
|
assert "datalad_dirty" in meta["datagrabber"]
|
||||||
|
assert meta["datagrabber"]["datalad_dirty"] is False
|
||||||
|
assert hasattr(dg, "_got_files") is False
|
||||||
|
assert datadir.exists() is True
|
||||||
|
assert elem1_bold.is_file() is True
|
||||||
|
assert elem1_bold.is_symlink() is True
|
||||||
|
assert elem1_t1w.is_file() is True
|
||||||
|
assert elem1_t1w.is_symlink() is True
|
||||||
|
|
||||||
|
assert datadir.exists() is False
|
||||||
|
assert len(list(datadir.glob("*"))) == 0
|
||||||
|
|
||||||
|
|
||||||
def test_datalad_previously_cloned(
|
def test_datalad_previously_cloned(
|
||||||
tmp_path: Path, concrete_datagrabber: Type
|
tmp_path: Path, concrete_datagrabber: Type
|
||||||
) -> None:
|
) -> None:
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue
Missing return type annotation.
We can also not have the
elsehere as we have early returns for the previous cases.I wouldn't go with a
passas it might have unforeseen cases. Better to log it or warn?