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

NF: run, rerun: Add --assume-ready option #5431

Merged
merged 1 commit into from
Feb 24, 2021

Conversation

kyleam
Copy link
Contributor

@kyleam kyleam commented Feb 22, 2021

Provide a way to avoid the cost of preparing specified inputs and
outputs.

Closes #3005.
Closes #3206.

@codecov
Copy link

codecov bot commented Feb 22, 2021

Codecov Report

Merging #5431 (08f0a00) into master (bc8e364) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5431      +/-   ##
==========================================
+ Coverage   89.47%   89.48%   +0.01%     
==========================================
  Files         296      296              
  Lines       41794    41844      +50     
==========================================
+ Hits        37396    37446      +50     
  Misses       4398     4398              
Impacted Files Coverage Δ
datalad/core/local/run.py 99.11% <100.00%> (+<0.01%) ⬆️
datalad/core/local/tests/test_run.py 99.27% <100.00%> (+0.09%) ⬆️
datalad/interface/rerun.py 96.14% <100.00%> (+0.05%) ⬆️
datalad/interface/tests/test_rerun.py 98.49% <100.00%> (+0.04%) ⬆️

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 bc8e364...a38e126. Read the comment docs.

Copy link
Member

@bpoldrack bpoldrack left a comment

Choose a reason for hiding this comment

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

Nice! LGTM.

Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

I wonder if extra_inputs should be taken care of as well. Left related comments

@@ -72,6 +72,15 @@ def _format_cmd_shorty(cmd):
return cmd_shorty


assume_ready_opt = Parameter(
args=("--assume-ready",),
constraints=EnsureChoice(None, "inputs", "outputs", "both"),
Copy link
Member

Choose a reason for hiding this comment

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

ATM "inputs" seems to not "mask away" handling of "extra_inputs". So I wonder if "extra_inputs" should be exposed too and then "all" might be a better choice?

for res in prepare_inputs(ds_path, inputs, extra_inputs):
for res in prepare_inputs(
ds_path,
[] if assume_ready in ["inputs", "both"] else inputs,
Copy link
Member

Choose a reason for hiding this comment

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

so we will still take care about "extra_inputs"... is that intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so we will still take care about "extra_inputs"... is that intended?

Yes, from the line below:

[] if assume_ready in ["inputs", "both"] else inputs,
# Ignore --assume-ready for extra_inputs. It's an unexposed
# implementation detail that lets wrappers sneak in inputs.
extra_inputs):

Provide a way to avoid the cost of preparing specified inputs and
outputs.

Closes datalad#3005.
Closes datalad#3206.
@yarikoptic yarikoptic added the cmd-run/rerun Issues about the favorite command of ReproPeople label Feb 24, 2021
@yarikoptic
Copy link
Member

errors are common suspects... let's proceed. Thank you @kyleam !

@yarikoptic yarikoptic merged commit 11b38b2 into datalad:master Feb 24, 2021
@kyleam kyleam deleted the run-skip-io-prepare branch March 4, 2021 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd-run/rerun Issues about the favorite command of ReproPeople
Projects
None yet
Development

Successfully merging this pull request may close these issues.

run/rerun: an option to NOT unlock outputs run: add an option to disable checking of inputs
3 participants