ENH: run_command: Support injecting a detached command #2937
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
|
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 |
|
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". |
|
@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. |
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)>
... for use in datalad-htcondor. 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 run_command().
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 _execute_command().
... to group code that will be moved under a conditional in the next commit.
This allows a record to be created for a command that is executed remotely. Note that most of the code change in run.py corresponds to pure indentation. Closes datalad#2934.
kyleam
force-pushed
the
enh-run-inject
branch
from
711cf4d
to
36394d1
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
yarikoptic
approved these changes
Oct 29, 2018
There was a problem hiding this comment.
Just left a minor comment, otherwise seems to be legitimate. If fits @mih needs, we should merge
datalad/interface/run.py
Outdated
| 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`. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Sounds like a good idea, yes.
mih
added a commit
to datalad/datalad-htcondor
that referenced
this pull request
Nov 1, 2018
datalad/datalad#2937 Full RF yet to come.
yarikoptic
added a commit
that referenced
this pull request
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](https://singularity-hub.org/containers/5663/view) 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 http://datasets.datalad.org - 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
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.
Main changes:
New function
prepare_inputsto help datalad-htcondor to avoid some duplicated code.format_commandnow handles custom substitutions. This fixesrerun --scriptfor commands that include substitutions and helps (a little bit) with code duplication in datalad-htcondor.run_commandaccepts a new argumentextra_infothat allows arbitrary information to be added to the run record.run_commandaccepts a new argumentinjectthat 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) (runhelper to fake the actual execution part? #2934).@mih, will
inject=Truework for you wrt datalad-htcondor?