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

WIP: ENH: Implement command run-bidsapp #28

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions datalad_neuroimaging/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,12 @@
'BIDS2Scidata',
'bids2scidata',
),
(
'datalad_neuroimaging.bidsapp',
'BidsApp',
'run-bidsapp',
'run_bidsapp',
)

]
)
98 changes: 98 additions & 0 deletions datalad_neuroimaging/bidsapp.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
import logging
from os.path import join as opj
from datalad.interface.base import build_doc, Interface
from datalad.support.param import Parameter
from datalad.distribution.dataset import datasetmethod, EnsureDataset, \
require_dataset, resolve_path, Dataset
from datalad.interface.utils import eval_results
from datalad.support.constraints import EnsureStr
from datalad.support.constraints import EnsureNone
from datalad.support.exceptions import InsufficientArgumentsError
from datalad.coreapi import run
from datalad.interface.results import get_status_dict


lgr = logging.getLogger("datalad.cbbsimaging.bidsapp")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely needs to change to neuroimaging.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed



@build_doc
class BidsApp(Interface):
"""Run a BIDSApp on the dataset
"""

_params_ = dict(
dataset=Parameter(
args=("-d", "--dataset"),
doc="""dataset to run the BIDSApp on""",
constraints=EnsureDataset() | EnsureNone()),
cmd=Parameter(
args=("cmd",),
doc="""Preconfigured BIDSApp to run. The name corresponds to
datalad.neuroimaging.bidsapp.NAME configuration.""",
metavar="CMD",
constraints=EnsureStr() | EnsureNone())
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be very useful to expose all the aspects of preconfiguration also as arguments, so we can support ad-hoc use.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. I also assume, while making other BIDSApps available that way, we'll discover the need for slightly different configuration.


@staticmethod
@datasetmethod(name='run_bidsapp')
@eval_results
def __call__(cmd, dataset=None):

dataset = require_dataset(dataset, check_installed=True,
purpose="run BIDS App")

container_name = \
dataset.config.get("datalad.neuroimaging.bidsapp.%s.container-name"
% cmd)
if not container_name:
raise ValueError("Missing configuration for "
"datalad.neuroimaging.bidsapp.%s.container-name"
% cmd)

container_ds = Dataset(opj(dataset.path, ".git",
"environment", container_name))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is better to put it under .git/datalad/environment (or some other name, but inside a datalad namespace), otherwise we are in potential conflict with other git-based tools.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right.

if not container_ds.is_installed():
source = \
dataset.config.get("datalad.neuroimaging.bidsapp.%s.container-url"
% cmd)
if not source:
raise ValueError("Missing configuration for "
"datalad.neuroimaging.bidsapp.%s.container-url"
% cmd)

lgr.info("Installing BIDSApp %s from %s", container_name, source)
for r in dataset.install(path=container_ds.path, source=source, result_xfm=None, return_type='generator'):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the container get installed/registered in the dataset? Now it would end up under .git, but in this case the run record further down would not see it as a precondition or input, and a rerun would not be possible, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Forgot to do that.


if r['type'] == 'dataset':
container_ds = Dataset(r['path'])
yield r

image_name = \
container_ds.config.get("datalad.neuroimaging.bidsapp.container")
if image_name is None:
image_name = ""
else:
if not container_ds.repo.file_has_content(image_name):
container_ds.get(image_name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what 70-76 do. A comment would be nice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.


image_exec = \
container_ds.config.get("datalad.neuroimaging.bidsapp.exec")
if not image_exec:
raise ValueError("Missing configuration for "
"datalad.neuroimaging.bidsapp.exec in %s" %
container_ds.path)

cmd_call = \
dataset.config.get("datalad.neuroimaging.bidsapp.%s.exec" % cmd)
if not cmd_call:
raise ValueError("Missing configuration for "
"datalad.neuroimaging.bidsapp.%s.exec" % cmd)

run_cmd = "{exec_} {img} {call}".format(exec_=image_exec,
img="" if image_name is None
else opj(container_ds.path,
image_name),
call=cmd_call)

for r in dataset.run(run_cmd, message="Run of bidsapp %s" % cmd):
yield r