Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
51f949a
Add --extra-inputs option and {img_dir} placeholder
nobodyinperson Jan 19, 2023
574c9d6
Add multiple overlays to test script
nobodyinperson Jan 19, 2023
14ef7f8
Add basic test for `containers-add --extra-inputs`
nobodyinperson Jan 19, 2023
9793587
Remove test shell script
nobodyinperson Jan 19, 2023
90428eb
Add test for `containers-run` with `--extra-inputs`
nobodyinperson Jan 19, 2023
972f16e
Change quoting in test
nobodyinperson Jan 19, 2023
25c768f
Remove `containers-run` test, can't get it to work in CI
nobodyinperson Jan 19, 2023
be5f29a
Empty test
bpoldrack Jan 30, 2023
a64ff9b
Update datalad_container/containers_add.py
nobodyinperson Jan 31, 2023
35cbd48
Don't use JSON for config storage
nobodyinperson Jan 31, 2023
d35dd4e
Rename img_dir to img_dirpath for --call-fmt
nobodyinperson Jan 31, 2023
9be3434
Fixup for removing JSON
nobodyinperson Jan 31, 2023
fa15b81
Make it --extra-input, not --extra-inputs for consistency
nobodyinperson Jan 31, 2023
4f0ecec
Cleanup
nobodyinperson Jan 31, 2023
9234493
Make sensible placeholders available in --extra-input
nobodyinperson Jan 31, 2023
1f118ec
Try (and fail) again to write a test for containers-run
nobodyinperson Jan 31, 2023
9c124f2
Improve placeholder handling in --extra-input
nobodyinperson Jan 31, 2023
3dc7366
ENH: define helper cfg variable to avoid duplication
yarikoptic Feb 2, 2023
88b52c7
BF+RF(TST): do not bother with url for container,
yarikoptic Feb 2, 2023
d193298
(still not ready) Fix `test_extra_inputs`
nobodyinperson Feb 5, 2023
a8d0b91
Fix containers-run test
nobodyinperson Feb 7, 2023
43c37ee
Update datalad_container/containers_run.py
nobodyinperson Feb 17, 2023
f13ffd5
Update datalad_container/containers_run.py
nobodyinperson Feb 17, 2023
738f1a1
Update datalad_container/tests/test_run.py
nobodyinperson Feb 17, 2023
af1034b
🎨 Use ensure_iter for cleaner implementation
nobodyinperson Feb 17, 2023
64ea256
Remove debug commented out print lines in a test
yarikoptic Feb 18, 2023
00888ce
[release-action] Autogenerate changelog snippet for PR 190
Feb 18, 2023
d46b443
Explicitly install uidmap and run it all with set -x to see which com…
yarikoptic Feb 18, 2023
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
3 changes: 3 additions & 0 deletions changelog.d/pr-190.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
### 🚀 Enhancements and New Features

- Add `--extra-inputs` to `containers-add`. Fixes [#189](https://github.com/datalad/datalad-container/issues/189) via [PR #190](https://github.com/datalad/datalad-container/pull/190) (by [@nobodyinperson](https://github.com/nobodyinperson))
Copy link
Member

Choose a reason for hiding this comment

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

feel welcome to expand here on how/when to use this feature, e.g. close to your use case.

44 changes: 42 additions & 2 deletions datalad_container/containers_add.py
Original file line number Diff line number Diff line change
Expand Up @@ -140,11 +140,29 @@ class ContainersAdd(Interface):
this container, e.g. "singularity exec {img} {cmd}". Where '{img}'
is a placeholder for the path to the container image and '{cmd}' is
replaced with the desired command. Additional placeholders:
'{img_dspath}' is relative path to the dataset containing the image.
'{img_dspath}' is relative path to the dataset containing the image,
'{img_dirpath}' is the directory containing the '{img}'.
""",
metavar="FORMAT",
constraints=EnsureStr() | EnsureNone(),
),
extra_input=Parameter(
args=("--extra-input",),
doc="""Additional file the container invocation depends on (e.g.
overlays used in --call-fmt). Can be specified multiple times.
Similar to --call-fmt, the placeholders {img_dspath} and
{img_dirpath} are available. Will be stored in the dataset config and
later added alongside the container image to the `extra_inputs`
field in the run-record and thus automatically be fetched when
needed.
""",
action="append",
default=[],
metavar="FILE",
# Can't use EnsureListOf(str) yet as it handles strings as iterables...
# See this PR: https://github.com/datalad/datalad/pull/7267
# constraints=EnsureListOf(str) | EnsureNone(),
),
image=Parameter(
args=("-i", "--image"),
doc="""Relative path of the container image within the dataset. If not
Expand All @@ -168,7 +186,7 @@ class ContainersAdd(Interface):
@datasetmethod(name='containers_add')
@eval_results
def __call__(name, url=None, dataset=None, call_fmt=None, image=None,
update=False):
update=False, extra_input=None):
if not name:
raise InsufficientArgumentsError("`name` argument is required")

Expand Down Expand Up @@ -321,6 +339,28 @@ def __call__(name, url=None, dataset=None, call_fmt=None, image=None,
"{}.cmdexec".format(cfgbasevar),
call_fmt,
force=True)
# --extra-input sanity check
# TODO: might also want to do that for --call-fmt above?
extra_input_placeholders = dict(img_dirpath="", img_dspath="")
for xi in (extra_input or []):
try:
xi.format(**extra_input_placeholders)
except KeyError as exc:
yield get_status_dict(
action="containers_add", ds=ds, logger=lgr,
status="error",
message=("--extra-input %r contains unknown placeholder %s. "
"Available placeholders: %s",
repr(xi), exc, ', '.join(extra_input_placeholders)))
return

# actually setting --extra-input config
cfgextravar = "{}.extra-input".format(cfgbasevar)
if ds.config.get(cfgextravar) is not None:
ds.config.unset(cfgextravar)
for xi in (extra_input or []):
ds.config.add(cfgextravar, xi)

# store changes
to_save.append(op.join(".datalad", "config"))
for r in ds.save(
Expand Down
26 changes: 25 additions & 1 deletion datalad_container/containers_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from datalad.distribution.dataset import datasetmethod
from datalad.distribution.dataset import require_dataset
from datalad.interface.base import eval_results
from datalad.utils import ensure_iter

from datalad.interface.results import get_status_dict
from datalad.core.local.run import (
Expand Down Expand Up @@ -114,6 +115,7 @@ def __call__(cmd, container_name=None, dataset=None,
img=image_path,
cmd=cmd,
img_dspath=image_dspath,
img_dirpath=op.dirname(image_path) or ".",
)
cmd = callspec.format(**cmd_kwargs)
except KeyError as exc:
Expand All @@ -131,14 +133,36 @@ def __call__(cmd, container_name=None, dataset=None,
# just prepend and pray
cmd = container['path'] + ' ' + cmd

extra_inputs = []
for extra_input in ensure_iter(container.get("extra-input",[]), set):
try:
xi_kwargs = dict(
img_dspath=image_dspath,
img_dirpath=op.dirname(image_path) or ".",
)
Copy link
Member

Choose a reason for hiding this comment

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

codeclimate btw warned about code duplication here. I think it is ok, although indeed duplicates with cmd_kwargs definition above so we could have some common_kwargs to be this structure and defined before any use and then only enhanced for cmd_kwargs. But really might be not worth the hassle this far, just wanted to leave a note that I am ok to suppress codeclimate for this.

extra_inputs.append(extra_input.format(**xi_kwargs))
except KeyError as exc:
yield get_status_dict(
'run',
ds=ds,
status='error',
message=(
'Unrecognized extra_input placeholder: %s. '
'See containers-add for information on known ones: %s',
exc,
", ".join(xi_kwargs)))
return

lgr.debug("extra_inputs = %r", extra_inputs)

with patch.dict('os.environ',
{CONTAINER_NAME_ENVVAR: container['name']}):
# fire!
for r in run_command(
cmd=cmd,
dataset=dataset or (ds if ds.path == pwd else None),
inputs=inputs,
extra_inputs=[image_path],
extra_inputs=[image_path] + extra_inputs,
outputs=outputs,
message=message,
expand=expand,
Expand Down
40 changes: 40 additions & 0 deletions datalad_container/tests/test_containers.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,46 @@ def test_container_files(ds_path=None, local_file=None, url=None):
assert(not op.lexists(target_path))


@with_tree(tree={
"container.img": "container",
"overlay1.img": "overlay 1",
"overlay2.img": "overlay 2",
})
def test_extra_inputs(ds_path=None):
container_file = 'container.img'
overlay1_file = 'overlay1.img'
overlay2_file = 'overlay2.img'

# prepare dataset:
ds = Dataset(ds_path).create(force=True)
ds.save()

ds.containers_add(
name="container",
image=container_file,
call_fmt="apptainer exec {img} {cmd}",
)
ds.containers_add(
name="container-with-overlay",
image=container_file,
call_fmt="apptainer exec --overlay {img_dirpath}/overlay1.img {img} {cmd}",
extra_input=[overlay1_file]
)
ds.containers_add(
name="container-with-two-overlays",
image=container_file,
call_fmt="apptainer exec --overlay {img_dirpath}/overlay1.img --overlay {img_dirpath}/overlay2.img:ro {img} {cmd}",
extra_input=[overlay1_file, overlay2_file]
)

res = ds.containers_list(**RAW_KWDS)
assert_result_count(res, 3)

assert_equal(ds.config.get("datalad.containers.container.extra-input"), None)
assert_equal(ds.config.get("datalad.containers.container-with-overlay.extra-input",get_all=True), "overlay1.img")
assert_equal(ds.config.get("datalad.containers.container-with-two-overlays.extra-input",get_all=True), ("overlay1.img", "overlay2.img"))


@with_tempfile
@with_tree(tree={'foo.img': "foo",
'bar.img': "bar"})
Expand Down
42 changes: 42 additions & 0 deletions datalad_container/tests/test_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
containers_run,
create,
)
from datalad.local.rerun import get_run_info
from datalad.cmd import (
StdOutCapture,
WitlessRunner,
Expand Down Expand Up @@ -191,6 +192,47 @@ def test_custom_call_fmt(path=None, local_file=None):
assert_in('image=../sub/righthere cmd=XXX img_dspath=../sub', out['stdout'])


@with_tree(
tree={
"overlay1.img": "overlay1",
"sub": {
"containers": {"container.img": "image file"},
"overlays": {"overlay2.img": "overlay2", "overlay3.img": "overlay3"},
},
}
)
def test_extra_inputs(path=None):
ds = Dataset(path).create(force=True)
subds = ds.create("sub", force=True)
subds.containers_add(
"mycontainer",
image="containers/container.img",
call_fmt="echo image={img} cmd={cmd} img_dspath={img_dspath} img_dirpath={img_dirpath} > out.log",
extra_input=[
"overlay1.img",
"{img_dirpath}/../overlays/overlay2.img",
"{img_dspath}/overlays/overlay3.img",
],
)
ds.save(recursive=True) # record the entire tree of files etc
ds.containers_run("XXX", container_name="sub/mycontainer")
ok_file_has_content(
os.path.join(ds.repo.path, "out.log"),
"image=sub/containers/container.img",
re_=True,
)
commit_msg = ds.repo.call_git(["show", "--format=%B"])
cmd, runinfo = get_run_info(ds, commit_msg)
assert set(
[
"sub/containers/container.img",
"overlay1.img",
"sub/containers/../overlays/overlay2.img",
"sub/overlays/overlay3.img",
]
) == set(runinfo.get("extra_inputs", set()))


@skip_if_no_network
@with_tree(tree={"subdir": {"in": "innards"}})
def test_run_no_explicit_dataset(path=None):
Expand Down
2 changes: 2 additions & 0 deletions tools/ci/install-singularity.sh
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,7 @@ release="$(curl -fsSL https://api.github.com/repos/sylabs/singularity/releases/l
codename="$(lsb_release -cs)"
arch="$(dpkg --print-architecture)"
wget -O /tmp/singularity-ce.deb https://github.com/sylabs/singularity/releases/download/$release/singularity-ce_${release#v}-${codename}_$arch.deb
set -x
sudo apt-get install uidmap
sudo dpkg -i /tmp/singularity-ce.deb
sudo apt-get install -f