ENH: First cut at script to run singularity images in Docker#11
Conversation
yarikoptic
left a comment
There was a problem hiding this comment.
provided in separate comments
scripts/singularity_cmd_via_docker
Outdated
| # - cleansed environment variables | ||
| # while bind mounting (and starting from) current directory. | ||
| # | ||
| # COPYRIGHT: Yaroslav Halchenko 2019 |
There was a problem hiding this comment.
I don't think I am the one who did it ;)
scripts/singularity_cmd_via_docker
Outdated
|
|
||
| docker run -ti --privileged --rm \ | ||
| -v $PWD:/working-dir \ | ||
| -v $tmpdir:/working-temp \ |
There was a problem hiding this comment.
probably some inconsistent tabs and spaces use above so indentation jumps
scripts/singularity_cmd_via_docker
Outdated
| -w /working-dir \ | ||
| singularityware/singularity:latest \ | ||
| "$cmd" -e -c -W /working-temp -H "$updir/binds/HOME" \ | ||
| -B /working-dir --pwd /working-dir "$@" |
There was a problem hiding this comment.
to retain similarity to how singularity works natively and how we invoke it within our singularityware/singularity, $PWD should be a $PWD inside the container. So I guess, if it is mapped to /workdir-dir at docker level, it should be mapped back to $PWD within singularity call. but may be the best would be to bind-mount to $PWD within docker call, without changing to /working-dir
scripts/singularity_cmd_via_docker
Outdated
| # Create a dedicated temporary directory to be removed upon completion | ||
| tmpdir=$(mktemp -d --suffix=singtmp) | ||
| info "created temp dir $tmpdir" | ||
| trap "rm -fr '$tmpdir' && info 'removed temp dir $tmpdir'" exit |
There was a problem hiding this comment.
I think there is no need for a dedicated /tmp here for the docker sake
scripts/singularity_cmd_via_docker
Outdated
|
|
||
| if [ ! -z "${DATALAD_CONTAINER_NAME:-}" ]; then | ||
| export SINGULARITYENV_DATALAD_CONTAINER_NAME="$DATALAD_CONTAINER_NAME" | ||
| fi |
There was a problem hiding this comment.
I guess no need for any of the above analysis for cmd and the env var
scripts/singularity_cmd_via_docker
Outdated
| -v $tmpdir:/working-temp \ | ||
| -w /working-dir \ | ||
| singularityware/singularity:latest \ | ||
| "$cmd" -e -c -W /working-temp -H "$updir/binds/HOME" \ |
There was a problem hiding this comment.
I think that the singularity_cmd should get those $BHOME and $tmpdir exposed via some environment variable(s) (e.g. REPRONIM_CONTAINERS_BINDS) so this script would just bind mount them...
Overall think about "interaction" between that singularity_cmd and this script, or just moving this "docker run ... singularity" invocation instead of plain "singularity" call within that script, to be triggered via SINGULARITY_VIA_DOCKER env variable, which is if empty decided based on OS (non-Linux - use docker).
|
I moved the docker call into the singularity_cmd script and made the updates suggested above. To force the use of docker, the user can set a REPRONIM_USE_DOCKER env variable |
yarikoptic
left a comment
There was a problem hiding this comment.
Singularity environment should feel/behave identically regardless either ran through docker or directly, so bind mounts set and home should be the same
scripts/singularity_cmd
Outdated
| -v $PWD:$PWD \ | ||
| -w $PWD \ | ||
| singularityware/singularity:latest \ | ||
| "$cmd" -e -c -B $PWD --pwd $PWD "$@" |
There was a problem hiding this comment.
What about -W and -H in this call?
There was a problem hiding this comment.
@yarikoptic
Those two switches are not needed. For -W, the system /tmp and /var_tmp are isolated at 2 levels, first by the -c switch we pass and because we are running singularity in a docker container for additional isolation of those system directories from the host. The -H encounters the same isolation as the -W, plus we decided on only $PWD for binding within the singularity container.
Actually, why are we passing those two switches in the non-Docker singularity call? Will the user want to write output to a surrogate home directory?
There was a problem hiding this comment.
Surrogate home directory provides needed (by git and datalad) .gitconfig and uniform .bashrc . As for /tmp indeed not needed probably, if only for uniform operation
There was a problem hiding this comment.
In https://github.com/ReproNim/containers/pull/9/files I might also rely on being able to copy bash history for the interactive session from that temporary directory, so would be to have it visible on the host system
|
@yarikoptic |
|
@chaselgrove could you please test this script on your osx laptop? |
|
@mjtravers - have you approached the 2nd checkbox in #3 on user mapping? Currently it would run as root: I am not sure yet if we somehow could make use of |
|
This last commit addresses the 3 issues discussed at the last status meeting: testing: I added a test using the BATS test framework (https://github.com/bats-core/bats-core). We started using this framework for NITRC-CE testing and it is easy to use and run. I can rollback if it turns out to be not what we want for this repo. "$@" issues: This sorted out fairly easily when I gave up on "su" and implemented the singularity call using "sudo -u" instead. HEREDOC: Figured out a way to get a HEREDOC in a Dockerfile RUN statement |
|
awesome -- I will try it out a bit later. bats -- great! You made decision for us, so subsequent tests might as well be done in bats as well ;-) scripts/tests/arg-test.simg -- we shouldn't commit images directly here. 2.5M is not much but they would add up so "doesn't sale". Although a chicken-egg-problem of a kind, I would rename singularity file into |
| @test "verifying arguments passed to singularity_cmd Docker shim" { | ||
| export REPRONIM_USE_DOCKER=1 | ||
| run ../singularity_cmd \ | ||
| exec arg-test.simg /singularity "foo bar" blah 45.5 /dir |
There was a problem hiding this comment.
would you be kind to add one more (or just include to existing one) obscure one with & ; and '${something}' in it/them? ;-)
|
re .simg -- please amend/rewrite and existing commit so there is no commit to merge with this heavy object |
|
I thought about that but we need the .simg file to run the test. I could use the singularity_cmd itself to build the image temporarily, I suppose. The .img file is 2.7MB |
|
yeap, 2.7MB in this version... then if we change anything, would be +2.7MB and so on. I think it would be ok if you just build locally and for tests to assume that it is there for now. Whenever we get tagged Singularity build we could add it to reside under git-annex and make bats fetch it using "git annex get" from singularity hub or wherever it would be made available from. |
|
continuing on our brief hangout discussion -- it was the https://www.shellcheck.net/ . To avoid conflicts I guess I will not make this script fully compliant but will attend to |
singularityware contianer so that the user who launches the singularity_cmd script with have ownership of the files created.
Added BATS framework test for singularity-in-docker version of singularity_cmd Changed bash code in Dockerfile to a more readable HEREDOC Renamed the Singularity build file to meet the naming standard Quoted switch arguments to docker run command in singularity_cmd
|
you would've needed to squash 6a2844c and its parent cc77adb, or just rewrite both so there is no trace of the .simg file being added in git. Since you have merged master in the most recent commit... I did that and rebased on top of master, with the last commit being ENH: Renamed singularity-shim Dockerfile to match file naming convention is it ok to force push? |
|
Yes, force push is a-okay... BTW is that some sort of Star Wars reference? :) |
9a4cdc6 to
0a5a643
Compare
@yarikoptic
Fixes #3
Here's a first cut at the proxy Docker script.
Any directories that are bound into the container must be relative to the $PWD where the script is run.
I am still experimenting to verify that it works on Windows but have confirmed it works on Linux machines. Will need a volunteer to test on a Mac.