ENH: set DATALAD_CONTAINER_NAME env var while running a container #84
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yarikoptic
added a commit
to ReproNim/containers
that referenced
this pull request
May 15, 2019
DATALAD_CONTAINER_NAME is yet to be supported/set by datalad-container. PR: datalad/datalad-container#84 But decided also to improve the PS1 altogether so we could make PS1 less cryptic and set explicitly to point to being under singularity and also shorten the long annex key if using old singularity
kyleam
reviewed
May 15, 2019
datalad_container/containers_run.py
Outdated
| @@ -47,8 +52,15 @@ class ContainersRun(Interface): | |||
| A command is generated based on the input arguments such that the | |||
| container image itself will be recorded as an input dependency of | |||
| the command execution in the `run` record in the git history. | |||
|
|
|||
| During execution the environment variable {name_envvar} set to the | |||
datalad_container/containers_run.py
Outdated
| @@ -112,6 +124,9 @@ def __call__(cmd, container_name=None, dataset=None, | |||
| # with amend inputs to also include the container image | |||
| inputs = (inputs or []) + [image_path] | |||
|
|
|||
| old_envvar = os.environ.get(CONTAINER_NAME_ENVVAR, None) | |||
| os.environ[CONTAINER_NAME_ENVVAR] = container['name'] | |||
There was a problem hiding this comment.
What about using patch.dict instead? Right now the value is cleaned up only if something was already set and wouldn't be restored properly for python callers if there is an exception raised within Run.__call__().
There was a problem hiding this comment.
Thank you @kyleam for review/suggestions. I am not sure why I was lazy to not use patch.dict. Did it (with delayed import, takes too long to import, at least while I am also Zoom'ing). Committed via --amend, pushed.
The main reason was due to the fact that older (prior 3.0?) singularity
followed the symlinked for the annexed file to set
SINGULARITY_NAME and SINGULARITY_CONTAINER env vars:
$> singularity --version ; singularity exec images/repronim/repronim-reproin--0.5.4.sing bash -c 'export | grep SING'
2.6.1-dist
declare -x SINGULARITY_CONTAINER="MD5E-s301797407--a50bf88c0fb53809154e42e8d08a482e.4.sing"
declare -x SINGULARITY_NAME="MD5E-s301797407--a50bf88c0fb53809154e42e8d08a482e.4.sing"
That seems was fixed in 3.x;
$> singularity --version ; singularity exec images/repronim/repronim-reproin--0.5.4.sing bash -c 'export | grep SING'
singularity version 3.0.3+ds
declare -x SINGULARITY_APPNAME=""
declare -x SINGULARITY_CONTAINER="/home/yoh/proj/repronim/containers/images/repronim/repronim-reproin--0.5.4.sing"
declare -x SINGULARITY_NAME="repronim-reproin--0.5.4.sing"
but
- in many places 2.x is still be used
- our container name but default does not even match container
name and just called "image"
- even when it is set to some reasonable named file, name of the
container could still be different
So I thought that it should only be advantageous to expose our container name
to underlying process, possibly a shim, to take advantage of it, e.g. to set
PS1 env variable for interactive shell etc.
yarikoptic
force-pushed
the
enh-pass-envvar
branch
from
4ea814b to
dfc5c91
Compare
Codecov Report
@@ Coverage Diff @@
## master #84 +/- ##
==========================================
+ Coverage 85.2% 85.32% +0.11%
==========================================
Files 14 14
Lines 615 620 +5
==========================================
+ Hits 524 529 +5
Misses 91 91
Continue to review full report at Codecov.
|
kyleam
added a commit
that referenced
this pull request
May 15, 2019
kyleam
added a commit
to kyleam/datalad-container
that referenced
this pull request
May 15, 2019
yarikoptic
added a commit
that referenced
this pull request
May 30, 2019
0.4.0 (May 29, 2019) -- run-baby-run
The minimum required DataLad version is now 0.11.5.
New features
- The call format gained the "{img_dspath}" placeholder, which expands
to the relative path of the dataset that contains the image. This
is useful for pointing to a wrapper script that is bundled in the
same subdataset as a container.
- `containers-run` now passes the container image to `run` via its
`extra_inputs` argument so that a run command's "{inputs}" field is
restricted to inputs that the caller explicitly specified.
- During execution, `containers-run` now sets the environment variable
`DATALAD_CONTAINER_NAME` to the name of the container.
Fixes
- `containers-run` mishandled paths when called from a subdirectory.
- `containers-run` didn't provide an informative error message when
`cmdexec` contained an unknown placeholder.
- `containers-add` ignores the `--update` flag when the container
doesn't yet exist, but it confusingly still used the word "update"
in the commit message.
* tag '0.4.0': (28 commits)
Finalizing 0.4.0 changelog/version
ENH: Kyle to CONTRIBUTORS
MNT: setup.py: Bump require DataLad version for extra_inputs
ENH: run: Move the image from inputs to extra_inputs
RF: run: Use run_command()
TST: Don't check content of /singularity
CHANGELOG.md: Add entry for gh-86
BF(TST): specify really bogus URL to containers_add
BF: report "Update " in commit message only if an existing image was actually updated
CHANGELOG.md: Add entry for gh-84
ENH: set DATALAD_CONTAINER_NAME env var while running a container
CHANGELOG.md: Prepare entries for 0.4.0
ENH: run: Provide more informative message for cmdexec key error
ENH: add: Log start of 'singularity build' call
TST: Update test for dspath => parentds rename (e60b06b)
ENH(DOC): mention {img_dspath} in the --call-fmt doc
RF: dspath -> parentds, use resolved dataset (ds) not dataset variable
BF: run: Preserve subdirectory call context
Revert "RF(OPT): do not require any real container"
RF(OPT): do not require any real container
...
adswa
pushed a commit
to adswa/datalad-container
that referenced
this pull request
Nov 8, 2023
Enh workflow
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The main reason was due to the fact that older (prior 3.0?) singularity
followed the symlinked for the annexed file to set
SINGULARITY_NAME and SINGULARITY_CONTAINER env vars:
That seems was fixed in 3.x;
but
name and just called "image"
container could still be different
So I thought that it should only be advantageous to expose our container name
to underlying process, possibly a shim, to take advantage of it, e.g. to set
PS1 env variable for interactive shell etc.