Add `--extra-inputs` to `containers-add` #190

nobodyinperson · 2023-01-19T09:24:03Z

This PR adds the datalad containers-add --extra-inputs option to specify additional files that should be included in the run-record and thus auto-fetched upon datalad containers-run.

Furthermore the --call-fmt now also has a {img_dir} placeholder for easier specification of files next to the image.

I added a test for containers-add asserting that the correct values land in .datalad/config. There is currently no test for containers-run as I couldn't get this to work in CI. I tried a shell-script as 'container' and sourced files as 'overlays', works on my machine, weird errors in CI.

Full test script test/run.sh for reference, a comparable pytest is shipped with this PR:

#!/bin/sh
cd $(dirname "$(readlink -f "$0")")
(chmod -R +w ds;rm -rf ds) 2>/dev/null
set -x
datalad create ds
cd ds
mkdir containers
sudo apptainer build containers/alpine.sif docker://alpine
sudo apptainer overlay create --size 64 containers/overlay.img
sudo chown $USER:$USER containers/overlay.img
sudo apptainer overlay create --size 64 containers/overlay2.img
sudo chown $USER:$USER containers/overlay2.img
datalad $@ containers-add alpine -i containers/alpine.sif
datalad $@ containers-add alpine-with-overlay \
    --call-fmt 'apptainer exec --overlay {img_dir}/overlay.img {img} {cmd}' \
    --extra-inputs containers/overlay.img \
    -i containers/alpine.sif
datalad $@ containers-add alpine-with-two-overlays \
    --call-fmt 'apptainer exec --overlay {img_dir}/overlay.img --overlay {img_dir}/overlay2.img:ro {img} {cmd}' \
    --extra-inputs containers/overlay.img containers/overlay2.img \
    -i containers/alpine.sif
datalad save
echo 1 > file.txt
datalad save
cat .datalad/config
datalad $@ containers-run -n alpine -o alpine-uname-a.txt 'uname -a > {outputs[0]}'
cat alpine-uname-a.txt
datalad $@ containers-run -n alpine-with-overlay -o alpine-with-overlay-uname-a.txt 'uname -a > {outputs[0]}'
cat alpine-with-overlay-uname-a.txt
datalad $@ containers-run -n alpine-with-two-overlays -o alpine-with-two-overlays-uname-a.txt 'uname -a > {outputs[0]}'
cat alpine-with-two-overlays-uname-a.txt

closes #189

codeclimate · 2023-01-19T09:24:28Z

Code Climate has analyzed commit d46b443 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Duplication	2

View more on Code Climate.

bpoldrack · 2023-01-19T09:51:09Z

Thanks for approaching that, @nobodyinperson!

WRT to the CI failure: The new parameter needs to also become part of the actual signature:
https://github.com/datalad/datalad-container/pull/190/files#diff-74ae0e7052ad9775f337a738a2a3915e46a23f555f2acbf4c24c72f6ebbab4caR182

nobodyinperson · 2023-01-19T09:51:43Z

Thanks for approaching that, @nobodyinperson!

WRT to the CI failure: The new parameter needs to also become part of the actual signature: https://github.com/datalad/datalad-container/pull/190/files#diff-74ae0e7052ad9775f337a738a2a3915e46a23f555f2acbf4c24c72f6ebbab4caR182

Yep, working on it!

codecov · 2023-01-19T11:39:21Z

Codecov Report

Base: 93.92% // Head: 93.49% // Decreases project coverage by -0.43% ⚠️

Coverage data is based on head (d46b443) compared to base (66941a9).
Patch coverage: 85.71% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #190      +/-   ##
==========================================
- Coverage   93.92%   93.49%   -0.43%     
==========================================
  Files          19       19              
  Lines         889      938      +49     
==========================================
+ Hits          835      877      +42     
- Misses         54       61       +7

Impacted Files	Coverage Δ
datalad_container/containers_add.py	`89.67% <66.66%> (-1.94%)`	⬇️
datalad_container/containers_run.py	`84.37% <70.00%> (-2.67%)`	⬇️
datalad_container/tests/test_containers.py	`100.00% <100.00%> (ø)`
datalad_container/tests/test_run.py	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

nobodyinperson · 2023-01-19T16:11:34Z

I'm happy with this so far. I give up on implementing a test for containers-run as I wasn't able to get it working in CI. 🤷

bpoldrack · 2023-01-30T07:09:05Z

Thank you very much, @nobodyinperson . I'm looking into the test issues to finish this.

yarikoptic

brief comments on how to harmonize with other interfaces etc

datalad_container/containers_add.py

datalad_container/containers_run.py

nobodyinperson · 2023-01-31T11:47:49Z

@yarikoptic I implemented all of your suggestions. I also made img_dirpath and img_dspath available in --extra-input and added a check in containers-add for that (for --call-fmt invalid placeholers are silently ignored until containers-run, might want to add a similar check there as well).

I tried again to make a test for containers-run with extra-input but I couldn't get it work. Please one of you experts who are more familiar with the code base step in as I am hitting a dead end here and just burning time.

yarikoptic · 2023-01-31T14:41:22Z

just to make sure -- do you mean TODO at https://github.com/datalad/datalad-container/pull/190/files#diff-c9a6a0cde74f0c35ded765fc396d537f9485fe47645c4e00698ab951138042d6R218 ?

nobodyinperson · 2023-01-31T14:43:11Z

just to make sure -- do you mean TODO at https://github.com/datalad/datalad-container/pull/190/files#diff-c9a6a0cde74f0c35ded765fc396d537f9485fe47645c4e00698ab951138042d6R218 ?

Yes, in general a test for containers-run. The above linked comments are not enough to test the new --extra-input feature.

datalad_container/containers_add.py

yarikoptic · 2023-02-03T18:47:53Z

@nobodyinperson I sent a PR for your fork with suggestive changes/fixup to the test: nobodyinperson#1

nobodyinperson · 2023-02-05T19:26:43Z

Thanks @yarikoptic. How is the run-record accessed? The ´ds.repo apparently can't access the git log.

yarikoptic · 2023-02-07T03:40:17Z

probably look at rerun for somewhat ultimate referral on how "accessed": https://github.com/datalad/datalad/blob/HEAD/datalad/local/rerun.py#L282

nobodyinperson · 2023-02-07T12:35:14Z

Thanks @yarikoptic, I was now finally able to add a proper test for containers-run asserting the extra_inputs land in the run-record. ✅

yarikoptic

Looks good to me . Just nit picking to make a little more consistent with the rest of inconsistencies we have ;)

datalad_container/containers_run.py

datalad_container/tests/test_run.py

minor typo Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>

@yarikoptic

Thanks @yarikoptic for the hints

- {img_dspath} and {img_dirpath}

It now fails on the 2nd test with E datalad.runner.exception.CommandError: CommandError: 'datalad containers-run -n sub/mycontainer XXX' failed with exitcode 1 under /home/yoh/.tmp/datalad_temp_tree_test_extra_inputsratuqu5q [out: 'image=sub/containers/container.img cmd=XXX img_dspath=sub img_dirpath=sub/containers E run(error): /home/yoh/.tmp/datalad_temp_tree_test_extra_inputsratuqu5q (dataset) [Input did not match existing file: overlay1.img] E run(ok): /home/yoh/.tmp/datalad_temp_tree_test_extra_inputsratuqu5q (dataset) [echo image=sub/containers/container.img ...] E action summary: E get (notneeded: 4) E run (error: 1, ok: 1) E save (notneeded: 2)'] and not sure yet if legit since that overlay1.img extra_input is relative path, and intentionally or not is not present in super dataset: - we could have had it as a feature that somehow that extra_input is relative to current invocation path, while instructing people to use {img_dspath} whenever it needs to be relative from the img_ds. Then we would need to add one more overlay1.img in superdataet - but above would be a bit too of an obscure feature I think, so not sure if should be facilitated.

Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>

nobodyinperson · 2023-02-17T09:57:31Z

@yarikoptic I addressed all your remarks and rebased the branch onto master to get rid of the conflict around from datalad.interface.{utils,base} import eval_results.

Concerning the code style discussions I'd recommend using black as code formatter. Takes the discussion out of the formatting, it's one de-facto standard for Python code style.

datalad_container/tests/test_run.py

yarikoptic · 2023-02-18T02:10:32Z

@yarikoptic I addressed all your remarks and rebased the branch onto master to get rid of the conflict around from datalad.interface.{utils,base} import eval_results.

Concerning the code style discussions I'd recommend using black as code formatter. Takes the discussion out of the formatting, it's one de-facto standard for Python code style.

We do use black with pre-commit in dandi-cli and I quite like it. Given that there is no lineup of outstanding PRs we might as well add it here, may be even with pre-commit. Filed #196 to gather feedback ;)

yarikoptic · 2023-02-18T02:14:49Z

changelog.d/pr-190.md

@@ -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))


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

yarikoptic · 2023-02-18T02:21:57Z

datalad_container/containers_run.py

+                xi_kwargs = dict(
+                    img_dspath=image_dspath,
+                    img_dirpath=op.dirname(image_path) or ".",
+                )


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.

…mand errors On appveyor env we got Selecting previously unselected package singularity-ce. (Reading database ... 300710 files and directories currently installed.) Preparing to unpack /tmp/singularity-ce.deb ... Unpacking singularity-ce (3.11.0-focal) ... dpkg: dependency problems prevent configuration of singularity-ce: singularity-ce depends on uidmap; however: Package uidmap is not installed. dpkg: error processing package singularity-ce (--install): dependency problems - leaving unconfigured Processing triggers for man-db (2.9.1-1) ... Errors were encountered while processing: singularity-ce Reading package lists... Done Building dependency tree Reading state information... Done Correcting dependencies... Done The following additional packages will be installed: uidmap The following NEW packages will be installed: uidmap 0 upgraded, 1 newly installed, 0 to remove and 155 not upgraded. 1 not fully installed or removed. Need to get 26.4 kB of archives. After this operation, 172 kB of additional disk space will be used. Do you want to continue? [Y/n] Abort. Command exited with code 1

yarikoptic · 2023-02-18T02:29:20Z

pushed potential fix for appveyor failure to configure singularity. If doesn't work out -- can troubleshoot in a separate PR

yarikoptic · 2023-03-08T14:31:46Z

It did turn green -- let's proceed, thank you @nobodyinperson !

github-actions · 2023-05-25T19:29:50Z

PR released in 1.2.0

nobodyinperson force-pushed the 189-specifying-extra-inputs branch from 002e5b3 to 92c0989 Compare January 19, 2023 11:28

nobodyinperson marked this pull request as ready for review January 19, 2023 16:09

yarikoptic requested changes Jan 30, 2023

View reviewed changes

yarikoptic reviewed Feb 2, 2023

View reviewed changes

datalad_container/containers_add.py Show resolved Hide resolved

yarikoptic mentioned this pull request Feb 2, 2023

tuneup + test fix up (incomplete) nobodyinperson/datalad-container#1

Merged

nobodyinperson requested a review from yarikoptic February 15, 2023 06:48

yarikoptic requested changes Feb 17, 2023

View reviewed changes

nobodyinperson and others added 10 commits February 17, 2023 10:48

Add --extra-inputs option and {img_dir} placeholder

51f949a

Add multiple overlays to test script

574c9d6

Add basic test for containers-add --extra-inputs

14ef7f8

Remove test shell script

9793587

Add test for containers-run with --extra-inputs

90428eb

Change quoting in test

972f16e

Remove containers-run test, can't get it to work in CI

25c768f

Empty test

be5f29a

Update datalad_container/containers_add.py

a64ff9b

minor typo Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>

Don't use JSON for config storage

35cbd48

Thanks @yarikoptic for the hints

nobodyinperson and others added 12 commits February 17, 2023 10:48

Cleanup

4f0ecec

Make sensible placeholders available in --extra-input

9234493

- {img_dspath} and {img_dirpath}

Try (and fail) again to write a test for containers-run

1f118ec

Improve placeholder handling in --extra-input

9c124f2

ENH: define helper cfg variable to avoid duplication

3dc7366

(still not ready) Fix test_extra_inputs

d193298

Fix containers-run test

a8d0b91

Update datalad_container/containers_run.py

43c37ee

Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>

Update datalad_container/containers_run.py

f13ffd5

Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>

Update datalad_container/tests/test_run.py

738f1a1

Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>

🎨 Use ensure_iter for cleaner implementation

af1034b

nobodyinperson force-pushed the 189-specifying-extra-inputs branch from 70b4a1a to af1034b Compare February 17, 2023 09:53

yarikoptic reviewed Feb 18, 2023

View reviewed changes

datalad_container/tests/test_run.py Outdated Show resolved Hide resolved

Remove debug commented out print lines in a test

64ea256

yarikoptic added minor Increment the minor version when merged CHANGELOG-missing labels Feb 18, 2023

[release-action] Autogenerate changelog snippet for PR 190

00888ce

github-actions bot removed the CHANGELOG-missing label Feb 18, 2023

yarikoptic mentioned this pull request Feb 18, 2023

Add black and isort with pre-commit? #196

Open

yarikoptic reviewed Feb 18, 2023

View reviewed changes

yarikoptic merged commit afc5486 into datalad:master Mar 8, 2023

yarikoptic mentioned this pull request Mar 8, 2023

Add singularity inspect metalad extractor #200

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `--extra-inputs` to `containers-add` #190

Add `--extra-inputs` to `containers-add` #190

nobodyinperson commented Jan 19, 2023 •

edited

Loading

codeclimate bot commented Jan 19, 2023 •

edited

Loading

bpoldrack commented Jan 19, 2023

nobodyinperson commented Jan 19, 2023

codecov bot commented Jan 19, 2023 •

edited

Loading

nobodyinperson commented Jan 19, 2023

bpoldrack commented Jan 30, 2023

yarikoptic left a comment

nobodyinperson commented Jan 31, 2023

yarikoptic commented Jan 31, 2023

nobodyinperson commented Jan 31, 2023

yarikoptic commented Feb 3, 2023

nobodyinperson commented Feb 5, 2023

yarikoptic commented Feb 7, 2023

nobodyinperson commented Feb 7, 2023

yarikoptic left a comment

nobodyinperson commented Feb 17, 2023

yarikoptic commented Feb 18, 2023

yarikoptic Feb 18, 2023

yarikoptic Feb 18, 2023

yarikoptic commented Feb 18, 2023

yarikoptic commented Mar 8, 2023

github-actions bot commented May 25, 2023

		@@ -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))

Add --extra-inputs to containers-add #190

Add --extra-inputs to containers-add #190

Conversation

nobodyinperson commented Jan 19, 2023 • edited Loading

codeclimate bot commented Jan 19, 2023 • edited Loading

bpoldrack commented Jan 19, 2023

nobodyinperson commented Jan 19, 2023

codecov bot commented Jan 19, 2023 • edited Loading

Codecov Report

nobodyinperson commented Jan 19, 2023

bpoldrack commented Jan 30, 2023

yarikoptic left a comment

Choose a reason for hiding this comment

nobodyinperson commented Jan 31, 2023

yarikoptic commented Jan 31, 2023

nobodyinperson commented Jan 31, 2023

yarikoptic commented Feb 3, 2023

nobodyinperson commented Feb 5, 2023

yarikoptic commented Feb 7, 2023

nobodyinperson commented Feb 7, 2023

yarikoptic left a comment

Choose a reason for hiding this comment

nobodyinperson commented Feb 17, 2023

yarikoptic commented Feb 18, 2023

yarikoptic Feb 18, 2023

Choose a reason for hiding this comment

yarikoptic Feb 18, 2023

Choose a reason for hiding this comment

yarikoptic commented Feb 18, 2023

yarikoptic commented Mar 8, 2023

github-actions bot commented May 25, 2023

Add `--extra-inputs` to `containers-add` #190

Add `--extra-inputs` to `containers-add` #190

nobodyinperson commented Jan 19, 2023 •

edited

Loading

codeclimate bot commented Jan 19, 2023 •

edited

Loading

codecov bot commented Jan 19, 2023 •

edited

Loading