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

ENH: run: Move the image from inputs to extra_inputs #60

Merged
merged 3 commits into from May 28, 2019

Conversation

kyleam
Copy link
Contributor

@kyleam kyleam commented Nov 30, 2018

containers-run builds on run, augmenting the input list with a
Singularity image. This means that if a user then puts "{inputs}" in
their command, the image file is included too. This is almost
certainly surprising to the user because the user has not explicitly
specified the image with --input and is probably not aware that
containers-run adds it. Avoid this by using run_command's recently
added extra_inputs argument.

This depends on datalad/datalad#3038.

Closes #38.

@yarikoptic
Copy link
Member

We shouldn't forget to boost version dependency on datalad before releasing this one

@yarikoptic
Copy link
Member

FWIW #3038 is merged. I thought we merged this one as well and thought to use it.

@codecov
Copy link

codecov bot commented Jan 15, 2019

Codecov Report

Merging #60 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #60   +/-   ##
=======================================
  Coverage   85.48%   85.48%           
=======================================
  Files          14       14           
  Lines         627      627           
=======================================
  Hits          536      536           
  Misses         91       91
Impacted Files Coverage Δ
datalad_container/containers_run.py 86.79% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 436219a...571c203. Read the comment docs.

@yarikoptic
Copy link
Member

I think I ran into this issue again, forgot that I probably installed from your PR branch, then upgraded to current master, and kaboom ... took some time to arrive back to this PR to recall details - downgraded to your PR branch and now it works again.
we should definitely finish up/merge this PR asap, restarted travis builds

@kyleam
Copy link
Contributor Author

kyleam commented Jan 15, 2019

we should definitely finish up/merge this PR asap, restarted travis builds

I'll leave merging up to you.

I'm hesitant to merge it because that means it might be released, along with the required datalad version bump, before there is a fix for datalad/datalad#3087 (fallout from datalad/datalad#3009), which breaks the "dhub://" prefix. It looks like the current plan for 3087 is to wait for revolution pr to be merged (datalad/datalad#3106), though IMO 3087 is a bigger issue than original issues addressed by 3009, and it'd be better to revert the changes from 3009 while we wait for the revolution pr.

@yarikoptic
Copy link
Member

oh right -- good analysis, thanks! I should get back to the datalad/datalad#3106 and see what could be done, especially given @mih 's feedback. Any help would be most welcome!

@yarikoptic
Copy link
Member

although i failed to fix that datalad/datalad#3087 yet (may be revolution would just fix it all!) is this change anyhow directly demands it? if not, I would say we should merge -- I keep needing this fix, and haven't used dhub:// yet -- all images I use are singularity.

@kyleam
Copy link
Contributor Author

kyleam commented Mar 19, 2019

is this change anyhow directly demands it?

I don't have anything to add to what I said above.

kyleam added a commit to kyleam/datalad-container that referenced this pull request Apr 23, 2019
It was a useful idea to explore, but it's unlikely that anyone uses
it.  When an image is only available from Docker, we now support the
"docker://" prefix for building Singularity images, so it's not clear
that this is worth maintaining.  (And, although not the adapter's
fault, its existence sadly holds up the merge of dataladgh-60.)  If there is
something that "docker://" won't work for, we can always resurrect
this.
@kyleam kyleam mentioned this pull request Apr 23, 2019
@kyleam
Copy link
Contributor Author

kyleam commented Apr 23, 2019

Rebased and bumped DataLad dependency in setup.py.

range-diff
1:  70fe020 ! 1:  d2dd8d7 RF: run: Use run_command()
    @@ -17,7 +17,7 @@
     +from datalad.interface.run import run_command
      from datalad.interface.run import get_command_pwds
      from datalad.interface.run import normalize_command
    - from datalad_container.containers_list import ContainersList
    + from datalad_container.find_container import find_container
     @@
              inputs = (inputs or []) + [image_path]
      
    @@ -25,7 +25,7 @@
     -        for r in Run.__call__(
     +        for r in run_command(
                      cmd=cmd,
    -                 dataset=ds,
    +                 dataset=dataset or (ds if ds.path == pwd else None),
                      inputs=inputs,
     @@
                      message=message,
2:  7e03c5e ! 2:  89e9f03 ENH: run: Move the image from inputs to extra_inputs
    @@ -26,7 +26,7 @@
              # fire!
              for r in run_command(
                      cmd=cmd,
    -                 dataset=ds,
    +                 dataset=dataset or (ds if ds.path == pwd else None),
                      inputs=inputs,
     +                extra_inputs=[image_path],
                      outputs=outputs,
-:  ------- > 3:  2036812 MNT: setup.py: Bump require DataLad version for extra_inputs

@yarikoptic
Copy link
Member

datalad/datalad#3106 is ready for review to address the issue with adding directories. Unless some show stopped is found, it should make it into datalad 0.11.5

@yarikoptic
Copy link
Member

I kicked 0.11.5 out -- so after conflicts resolved we could merge/release!

@kyleam
Copy link
Contributor Author

kyleam commented May 28, 2019

Didn't realize there was yet another round of conflicts. Will update.

Doing so gives access to the extra_inputs argument, which isn't
exposed to the user-facing interface.  run_command() lacks the --rerun
compatibility flag of Run.__call__, but that's not an issue because
containers-run doesn't expose --rerun.
`containers-run` builds on `run`, augmenting the input list with a
Singularity image.  This means that if a user then puts "{inputs}" in
their command, the image file is included too.  This is almost
certainly surprising to the user because the user has not explicitly
specified the image with --input and is probably not aware that
containers-run adds it.  Avoid this by using run_command's recently
added extra_inputs argument.

Re: datalad/datalad#3038
Closes datalad#38.
extra_inputs was introduced in v0.11.2, but use v0.11.5 because that
will include 514545a46 (datalad/datalad#3106) as a fix for a
datalad-save regression that broke the docker adapter.
@kyleam kyleam merged commit 571c203 into datalad:master May 28, 2019
kyleam added a commit that referenced this pull request May 28, 2019
@kyleam kyleam deleted the use-run-command branch May 28, 2019 18:37
@yarikoptic yarikoptic added this to the Release 0.4.0 milestone May 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding image to inputs may surprise users that use "{inputs}" placeholder
2 participants