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_command: Support injecting a detached command #2937

merged 10 commits into from Nov 1, 2018


Copy link

@kyleam kyleam commented Oct 22, 2018

Main changes:

  • New function prepare_inputs to help datalad-htcondor to avoid some duplicated code.

  • format_command now handles custom substitutions. This fixes rerun --script for commands that include substitutions and helps (a little bit) with code duplication in datalad-htcondor.

  • run_command accepts a new argument extra_info that allows arbitrary information to be added to the run record.

  • run_command accepts a new argument inject that means "take the current working tree state as the result of the command" (i.e., don't prepare inputs or outputs before the command and don't execute the command) (gh-2934).

@mih, will inject=True work for you wrt datalad-htcondor?

Copy link
Contributor Author

@kyleam kyleam commented Oct 22, 2018

Note for @yarikoptic, who I know likes to review PRs from github. This is the sort of PR that is much easier to view from your terminal, making use of options like --color-moved and --ignore-space-change.

Copy link
Contributor Author

@kyleam kyleam commented Oct 23, 2018

The buildbot failure is the previously seen certs import error, and one of the appveyor builds failed with "failed to assign job to 'gce' build environment".

Copy link

@mih mih commented Oct 23, 2018

@kyleam FTR: I saw your PR, but I will need a sec or two before I can work the changes in and test them. Still chewing on the symlink issues, but making good progress.

kyleam added 9 commits Oct 24, 2018
gpaths is a single GlobbedPaths instance not a list of GlobbedPaths.
Otherwise 'rerun --script' will fail if it encounters a command that
uses custom placeholders.

This also somewhat addresses the code duplication in datalad-htcondor.
Getting rid of the "KeyError to result" duplication is trickier.

Re: <datalad/datalad-htcondor#6 (comment)>
Document how run_command differs from Run.__call__(), especially
because upcoming commits add more parameters that are unique to
This should help outside callers fit run_command to their needs.  For
example, datalad-htcondor may want to store extra information about
where the command was actually executed.
This probably won't be used elsewhere, but it makes run_command() a
bit more digestible, especially because this code would be indented
one more level when we add the 'inject' parameter.
run_command() doesn't rely on detecting whether cmd_exitcode is None,
so we might as well handle the None->0 conversion inside
... to group code that will be moved under a conditional in the next
This allows a record to be created for a command that is executed

Note that most of the code change in corresponds to pure

Closes datalad#2934.
@kyleam kyleam force-pushed the enh-run-inject branch from 711cf4d to 36394d1 Oct 24, 2018
Copy link

@codecov codecov bot commented Oct 24, 2018

Codecov Report

Merging #2937 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2937      +/-   ##
+ Coverage   90.32%   90.34%   +0.02%     
  Files         246      246              
  Lines       32029    32060      +31     
+ Hits        28930    28966      +36     
+ Misses       3099     3094       -5
Impacted Files Coverage Δ
datalad/interface/ 96.26% <ø> (ø) ⬆️
datalad/interface/ 100% <100%> (ø) ⬆️
datalad/interface/tests/ 99.83% <100%> (ø) ⬆️
datalad/support/ 96.89% <0%> (ø) ⬆️
datalad/support/ 88.84% <0%> (+0.03%) ⬆️
datalad/cmdline/ 78.63% <0%> (+0.09%) ⬆️
datalad/downloaders/ 88.81% <0%> (+3.49%) ⬆️

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 f935535...869d26b. Read the comment docs.

Copy link

@yarikoptic yarikoptic left a comment

Just left a minor comment, otherwise seems to be legitimate. If fits @mih needs, we should merge

Additional information to dump with the json run record. Any value
given here will take precedence over the standard run key. Warning:
Callers should try to use fairly specific key names to avoid collisions
with future keys added by `run`.
Copy link

@yarikoptic yarikoptic Oct 29, 2018

minor: I wonder if we should even encourage adding additional information in subsections (e.g. niceman to contain any relevant/added field, ...) instead of flooding this record at the top level?

Copy link
Contributor Author

@kyleam kyleam Oct 29, 2018

Sounds like a good idea, yes.

@yarikoptic yarikoptic merged commit 7a74773 into datalad:master Nov 1, 2018
7 of 10 checks passed
mih added a commit to datalad/datalad-htcondor that referenced this issue Nov 1, 2018
@kyleam kyleam deleted the enh-run-inject branch Nov 1, 2018
@yarikoptic yarikoptic added this to the Release 0.11.1 milestone Nov 24, 2018
yarikoptic added a commit that referenced this issue Nov 27, 2018
	## 0.11.1 (Nov 25, 2018) -- v7-better-than-v6

	Rushed out bugfix release to stay fully compatible with recent
	[git-annex] which introduced v7 to replace v6.

	### Fixes

	- [install]: be able to install recursively into a dataset ([#2982])
	- [save]: be able to commit/save changes whenever files potentially
	  could have swapped their storage between git and annex
	  ([#1651]) ([#2752]) ([#3009])
	- [aggregate-metadata]:
	  - dataset's itself is now not "aggregated" if specific paths are
		provided for aggregation ([#3002]). That resolves the issue of
		`-r` invocation aggregating all subdatasets of the specified dataset
		as well
	  - also compare/verify the actual content checksum of aggregated metadata
		while considering subdataset metadata for re-aggregation ([#3007])
	- `annex` commands are now chunked assuming 50% "safety margin" on the
	  maximal command line length. Should resolve crashes while operating
	  ot too many files at ones ([#3001])
	- `run` sidecar config processing ([#2991])
	- no double trailing period in docs ([#2984])
	- correct identification of the repository with symlinks in the paths
	  in the tests ([#2972])
	- re-evaluation of dataset properties in case of dataset changes ([#2946])
	- [text2git] procedure to use `ds.repo.set_gitattributes`
	  ([#2974]) ([#2954])
	- Switch to use plain `os.getcwd()` if inconsistency with env var
	  `$PWD` is detected ([#2914])
	- Make sure that credential defined in env var takes precedence
	  ([#2960]) ([#2950])

	### Enhancements and new features

	- [shub://datalad/datalad:git-annex-dev](
	  provides a Debian buster Singularity image with build environment for
	  [git-annex]. [tools/bisect-git-annex]() provides a helper for running
	  `git bisect` on git-annex using that Singularity container ([#2995])
	- Added [.zenodo.json]() for better integration with Zenodo for citation
	- [run-procedure] now provides names and help messages with a custom
	  renderer for ([#2993])
	- Documentation: point to [datalad-revolution] extension (prototype of
	  the greater DataLad future)
	- [run]
	  - support injecting of a detached command ([#2937])
	- `annex` metadata extractor now extracts `annex.key` metadata record.
	  Should allow now to identify uses of specific files etc ([#2952])
	- Test that we can install from
	- Proper rendering of `CommandError` (e.g. in case of "out of space"
	  error) ([#2958])

* tag '0.11.1':
  Adjust the date -- 25th fell through due to __version__ fiasco
  BF+ENH(TST): boost hardcoded version + provide a test to guarantee consistency in the future
  This (expensive) approach is not needed in v6+
  small tuneup to changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants