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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: run: Provide result record for command execution #6447

Merged
merged 5 commits into from
Feb 14, 2022

Conversation

mih
Copy link
Member

@mih mih commented Feb 11, 2022

Note that this contains a breaking change:

  • it changes a CommandError to a result record with status="error"

As pointed out in #3071 (comment) we could continue to raise a CommandError for Python-API users. But @mih is not convinced that this is worth it.

This is the last piece from the closed #5695

Demo:

% datalad -f json_pp run "echo 12"
[INFO   ] == Command start (output follows) ===== 
12
[INFO   ] == Command exit (modification check follows) ===== 
{
  "action": "run",
  "exitcode": 0,
  "explicit_outputs": null,
  "message": "Executed command",
  "msg_path": null,
  "path": "/tmp/runny",
  "record_id": "49f818a6e4ab1d9c70610db52da25daf",
  "run_info": {
    "chain": [],
    "cmd": "echo 12",
    "dsid": "2f0b7d17-f5d2-4659-8da8-8bd9dd781e14",
    "exit": 0,
    "extra_inputs": [],
    "inputs": [],
    "outputs": [],
    "pwd": "."
  },
  "status": "ok",
  "type": "dataset"
}
{                                                                                                                           
  "action": "save",
  ...

TODO:

  • Add sidecar info to result record
  • Avoid duplicate exit_code record in the error case (use exit_code)

Changelog

馃挮 Enhancements and new features

馃獡 Deprecations and removals

  • run no longer raises a CommandError exception for failed commands, but yields an error result that includes a superset of the information provided by the exception. This change impacts command line usage insofar as the exit code of the underlying command is no longer relayed as the exit code of the run command call -- although run continues to exit with a non-zero exit code in case of an error. For Python API users, the nature of the raised exception changes from CommandError to IncompleteResultsError, and the exception handling is now configurable using the standard on_failure command argument. The original CommandError exception remains available via the exception property of the newly introduced result record for the command execution, and this result record is available via IncompleteResultsError.failed, if such an exception is raised.

After executing a command, yield a result record.  This allows callers
to control whether the save happens via the standard --on-failure
switch.

Note that this contains a breaking change:

  * it changes a CommandError to a result record with status="error"

Suggested by @mih in dataladgh-3071.
@mih mih added the semver-minor Increment the minor version when merged label Feb 11, 2022
@bpoldrack
Copy link
Member

As pointed out in #3071 (comment) we could continue to raise a CommandError for Python-API users. But @mih is not convience that this is worth it.

I agree. Especially since a python caller should somehow deal with possible error results anyway.

@codecov
Copy link

codecov bot commented Feb 11, 2022

Codecov Report

Merging #6447 (c9518d1) into master (aebb5ce) will increase coverage by 0.00%.
The diff coverage is 97.67%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6447   +/-   ##
=======================================
  Coverage   89.97%   89.98%           
=======================================
  Files         348      348           
  Lines       43859    43867    +8     
=======================================
+ Hits        39464    39474   +10     
+ Misses       4395     4393    -2     
Impacted Files Coverage 螖
datalad/core/local/run.py 97.89% <95.00%> (+0.05%) 猬嗭笍
datalad/core/local/tests/test_run.py 100.00% <100.00%> (酶)
datalad/local/tests/test_rerun.py 100.00% <100.00%> (酶)
datalad/local/tests/test_run_procedure.py 100.00% <100.00%> (酶)
datalad/local/run_procedure.py 94.30% <0.00%> (+0.63%) 猬嗭笍
datalad/core/local/create.py 98.62% <0.00%> (+0.68%) 猬嗭笍

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 aebb5ce...c9518d1. Read the comment docs.

@yarikoptic
Copy link
Member

FTR

here how it looks in master with CommandError
(git)lena:~datalad/datalad-master[master]git
$> python -c 'import datalad.api as dl; dl.Dataset("/tmp/testds").run("dokaboom"); print("Life is good")'
[INFO   ] == Command start (output follows) ===== 
/bin/sh: 1: dokaboom: not found
[INFO   ] == Command exit (modification check follows) ===== 
[INFO   ] The command had a non-zero exit code. If this is expected, you can save the changes with "Dataset('/tmp/testds').save(path='.', recursive=True, message_file='.git/COMMIT_EDITMSG')" 
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/yoh/proj/datalad/datalad-master/datalad/distribution/dataset.py", line 502, in apply_func
    return f(*args, **kwargs)
  File "/home/yoh/proj/datalad/datalad-master/datalad/interface/utils.py", line 436, in eval_func
    return return_func(*args, **kwargs)
  File "/home/yoh/proj/datalad/datalad-master/datalad/interface/utils.py", line 428, in return_func
    results = list(results)
  File "/home/yoh/proj/datalad/datalad-master/datalad/interface/utils.py", line 346, in generator_func
    for r in _process_results(
  File "/home/yoh/proj/datalad/datalad-master/datalad/interface/utils.py", line 543, in _process_results
    for res in results:
  File "/home/yoh/proj/datalad/datalad-master/datalad/core/local/run.py", line 299, in __call__
    for r in run_command(cmd, dataset=dataset,
  File "/home/yoh/proj/datalad/datalad-master/datalad/core/local/run.py", line 850, in run_command
    raise exc
  File "/home/yoh/proj/datalad/datalad-master/datalad/core/local/run.py", line 591, in _execute_command
    runner.run(
  File "/home/yoh/proj/datalad/datalad-master/datalad/runner/runner.py", line 201, in run
    raise CommandError(
datalad.runner.exception.CommandError: CommandError: 'dokaboom' failed with exitcode 127 under /tmp/testds
and here with IncompleteResultsError
(git)lena:~datalad/datalad-master[nf-runresult]git
$> python -c 'import datalad.api as dl; dl.Dataset("/tmp/testds").run("dokaboom"); print("Life is good")'
[INFO   ] == Command start (output follows) ===== 
/bin/sh: 1: dokaboom: not found
[INFO   ] == Command exit (modification check follows) ===== 
[INFO   ] The command had a non-zero exit code. If this is expected, you can save the changes with "Dataset('/tmp/testds').save(path='.', recursive=True, message_file='.git/COMMIT_EDITMSG')" 
run(error): /tmp/testds (dataset) [Executed command]
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/yoh/proj/datalad/datalad-master/datalad/distribution/dataset.py", line 502, in apply_func
    return f(*args, **kwargs)
  File "/home/yoh/proj/datalad/datalad-master/datalad/interface/utils.py", line 437, in eval_func
    return return_func(*args, **kwargs)
  File "/home/yoh/proj/datalad/datalad-master/datalad/interface/utils.py", line 429, in return_func
    results = list(results)
  File "/home/yoh/proj/datalad/datalad-master/datalad/interface/utils.py", line 414, in generator_func
    raise IncompleteResultsError(
datalad.support.exceptions.IncompleteResultsError: Command did not complete successfully. 1 failed:
[{'action': 'run',
  'exception': CommandError(''),
  'exception_traceback': '[run.py:_execute_command:610,runner.py:run:201]',
  'exit_code': 127,
  'explicit_outputs': None,
  'message': 'Executed command',
  'msg_path': '/tmp/testds/.git/COMMIT_EDITMSG',
  'path': '/tmp/testds',
  'record_id': None,
  'run_info': {'chain': [],
               'cmd': 'dokaboom',
               'dsid': 'c8e4f9b8-ad5e-450f-b956-d3796ce54edb',
               'exit': 127,
               'extra_inputs': [],
               'inputs': [],
               'outputs': [],
               'pwd': '.'},
  'status': 'error',
  'type': 'dataset'}]
so looks even more informative (exit code) somewhat. As long as such invocation is errorring out by default (instead of silently returning some error record and expecting user to inspect it), good with me.

@codeclimate
Copy link

codeclimate bot commented Feb 12, 2022

Code Climate has analyzed commit c9518d1 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Security 1

View more on Code Climate.

@mih
Copy link
Member Author

mih commented Feb 14, 2022

I added another changelog item with more details on the nature of the breaking change.

@mih mih merged commit 7e1bf0b into datalad:master Feb 14, 2022
@mih mih deleted the nf-runresult branch February 14, 2022 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor Increment the minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to force run (or containers-run) to save even if {cmd} exits with non-0
4 participants