run: Use 'datalad add' in failure instructions #3080
Merged
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
Under Python 2, we pass a bytestring to commit(), but the intention was to pass a unicode string, like we do under Python 3. Passing a bytestring as the message doesn't seem to cause any issues, at least according to test_save_message_file, but we should still do so for consistency across Python versions.
When save is called from the command line with both --message and --message-file but without -d<dataset>, we get two error messages, the intentional error message as well as one about a key error: [ERROR ] Both a message and message file were specified [ERROR ] 'path' [utils.py:default_result_renderer:503] (KeyError) This is because we pass refds_path as the path to get_status_dict(), but refds_path is None in the above scenario. Instead of worrying about properly populating the status dict info, which isn't particularly relevant for the --message/--message-file conflict, just raise a plain exception. Doing so is in line with what's done in other interfaces (e.g., aggregate raises a ValueError when it receives an unknown update mode).
This will be used in `add` as well.
`save` learned to accept a --message-file argument in da8d235 (ENH: save: Add -F/--message-file option, 2018-03-28). Teach `add` to do the same.
If a run command has a non-zero exit, we don't save the outputs and instead instruct the user how to save the outputs given the failure is expected. The instructions say to use 'datalad save -r .', but there are cases where this is not sufficient to add an output file (e.g., if a file is added to subdataset). Update the instructions to use 'add' instead of 'save', which is what run actually uses (by default) underneath. Also add '-d .' to the instructions to match run's dataset.add(), reducing the possibility of discrepancies like the one that cropped up in dataladgh-2276. There are still cases where run's dataset.add() is not sufficient (a known one is dataladgh-2454, which is addressed in datalad-revolution), but at least the command in the instructions will match run's behavior. Closes datalad#3072.
kyleam
force-pushed
the
run-fail-msg-add
branch
from
ab6bd6a
to
67ce0e3
Compare
Codecov Report
@@ Coverage Diff @@
## master #3080 +/- ##
==========================================
+ Coverage 90.24% 90.24% +<.01%
==========================================
Files 248 248
Lines 32571 32589 +18
==========================================
+ Hits 29393 29411 +18
Misses 3178 3178
Continue to review full report at Codecov.
|
yarikoptic
reviewed
Dec 9, 2018
| @@ -611,7 +611,7 @@ def run_command(cmd, dataset=None, inputs=None, outputs=None, expand=None, | |||
| ofh.write(assure_bytes(msg)) | |||
| lgr.info("The command had a non-zero exit code. " | |||
| "If this is expected, you can save the changes with " | |||
| "'datalad save -r -F %s .'", | |||
| "'datalad add -d . -r -F %s .'", | |||
There was a problem hiding this comment.
Now thinking about it, this wouldn't be a correct instruction in case of --explicit run... Not sure what would be a sensible cross platform and concise way though, besides add (may be adding it as a feature for all datalad API functions getting a list of files) getting an option of taking a file with filenames to add, and then is preparing that file alongside with the message file to be given here.
I will leave it for you to decide to move that into a new issue/PR and here for explicit mode may be just say that it might add too much or listing all outputs?
There was a problem hiding this comment.
this wouldn't be a correct instruction in case of
--explicitrun
When I've thought about it in the past, I've decided it's not worth worrying about --explicit here. If you feel strongly that we should tailor the message, please open a separate PR tweaking the message as you see fit.
yarikoptic
added a commit
that referenced
this pull request
Feb 8, 2019
A variety of bugfixes and enhancements ### Major refactoring and deprecations - All extracted metadata is now placed under git-annex by default. Previously files smaller than 20 kb were stored in git. ([#3109]) - TODO: get_runner #3104 and pending #3131 ### Fixes - Improved handling of long commands: - The code that inspected `SC_ARG_MAX` didn't check that the reported value was a sensible, positive number. ([#3025]) - More commands that invoke `git` and `git-annex` with file arguments learned to split up the command calls when it is likely that the command would fail due to exceeding the maximum supported length. ([#3138]) - The `setup_yoda_dataset` procedure created a malformed .gitattributes line. ([#3057]) - [download-url] unnecessarily tried to infer the dataset when `--no-save` was given. ([#3029]) - [rerun] aborted too late and with a confusing message when a ref specified via `--onto` didn't exist. ([#3019]) - [run]: - `run` didn't preserve the current directory prefix ("./") on inputs and outputs, which is problematic if the caller relies on this representation when formatting the command. ([#3037]) - Fixed a number of unicode py2-compatibility issues. ([#3035]) ([#3046]) - To proceed with a failed command, the user was confusingly instructed to use `save` instead of `add` even though `run` uses `add` underneath. ([#3080]) - Fixed a case where the helper class for checking external modules incorrectly reported a module as unknown. ([#3051]) - [add-archive-content] mishandled the archive path when the leading path contained a symlink. ([#3058]) - Following denied access, the credential code failed to consider a scenario, leading to a type error rather than an appropriate error message. ([#3091]) - Some tests failed when executed from a `git worktree` checkout of the source repository. ([#3129]) - During metadata extraction, batched annex processes weren't properly terminated, leading to issues on Windows. ([#3137]) - [add] incorrectly handled an "invalid repository" exception when trying to add a submodule. ([#3141]) - Pass `GIT_SSH_VARIANT=ssh` to git processes to be able to specify alternative ports in SSH urls ### Enhancements and new features - [search] learned to suggest closely matching keys if there are no hits. ([#3089]) - [create-sibling] gained a `--group` option so that the caller can specify the file system group for the repository. ([#3098]) - Interface classes can now override the default renderer for summarizing results. ([#3061]) - [run]: - `--input` and `--output` can now be shortened to `-i` and `-o`. ([#3066]) - Placeholders such as "{inputs}" are now expanded in the command that is shown in the commit message subject. ([#3065]) - `interface.run.run_command` gained an `extra_inputs` argument so that wrappers like [datalad-container] can specify additional inputs that aren't considered when formatting the command string. ([#3038]) - "--" can now be used to separate options for `run` and those for the command in ambiguous cases. ([#3119]) - The utilities `create_tree` and `ok_file_has_content` now support ".gz" files. ([#3049]) - The Singularity container for 0.11.1 now uses [nd_freeze] to make its builds reproducible. - A [publications] page has been added to the documentation. ([#3099]) - `GitRepo.set_gitattributes` now accepts a `mode` argument that controls whether the .gitattributes file is appended to (default) or overwritten. ([#3115]) - `datalad --help` now avoids using `man` so that the list of subcommands is shown. ([#3124]) * tag '0.11.2': (124 commits) Changelog entry for GIT_SSH_VARIANT change ENH: Declare our GIT_SSH_COMMAND as GIT_SSH_VARIANT=ssh BF: sshconnector: Don't use ssh's port flag as scp's RF: sshconnector: Simplify shlex quote import CHANGELOG(0.11.2): Fix some typos [DATALAD RUNCMD] CHANGELOG: Linkify 0.11.2 entries CHANGELOG: Do first pass for 0.11.2 CHANGELOG: Add missing link target for download-url Start cooking the 0.11.2 release RF: appveyor - move test_install tests to be ran the last RF: text_type instead of str ENH(TST): provide my timing for the slow test BF(TST): adjust the test for the fact that AnnexRepo.add does not blow on nonexisting files ENH(TST): two tests which test quick or thorough for add failing with too long list of files BF: get stderr if present, otherwise just use str(e) BF: append out/err only if not empty/None Centrlize handling running commands with long files list in _run_command_files_split RF: remove minor duplication of -- handling, place all files handling closer to the call RF: move unrelated to try/except handling outside ENH+DOC: Report actual process handle, not just PID ...
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.
Minor improvments
saves--message-filehandlingUpdate
addto support--message-fileUpdate
runs failure instructions to usedatalad add(closes failed run: provided command to run is insufficient #3072)