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

Result hooks #304

Merged
merged 9 commits into from
Dec 18, 2019
Merged

Result hooks #304

merged 9 commits into from
Dec 18, 2019

Conversation

adswa
Copy link
Contributor

@adswa adswa commented Dec 10, 2019

This adds a section on the new feature of result hooks to the hand book. Currently, I placed this section in the last chapter of the Basics, "Further options". It would be cool if the contents of the sections could be checked for accuracy, and if someone could point me to where I can find all possible keys and values of the result evaluation.

@adswa adswa changed the title [WIP] Result hooks Result hooks Dec 12, 2019
@mih mih self-requested a review December 15, 2019 19:17
Copy link
Collaborator

@mih mih left a comment

Choose a reason for hiding this comment

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

Cool, thanks much! I left one critical comment and a few minor suggestions.

docs/basics/101-145-hooks.rst Show resolved Hide resolved
docs/basics/101-145-hooks.rst Show resolved Hide resolved
docs/basics/101-145-hooks.rst Outdated Show resolved Hide resolved
This translates to "unlock the path the previous command operated on, in the
dataset the previous command operated on". Another example is this run command::

$ run {{"cmd": "touch {path}_annoyed", "dataset": "{dsarg}", "explicit": true}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this was me, but it is a stupid example ;-)

What about replacing the command with cp ~/templates/standard-readme.txt {path}/README and sell it as automatically populate a dataset with a default README.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Personally, I found the example hilarious ;-) Will change it nevertheless)

Copy link
Contributor Author

@adswa adswa Dec 16, 2019

Choose a reason for hiding this comment

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

I fail to get such a hook to run because of an inconveniently coinciding mismatching formatting options and command requirements: I am trying to execute "datalad run --output "README" "cp ~/Templates/standard-readme.txt {path}/README" --explicit" via a hook after successful installation of a dataset. Here are my git config calls:

For matching:

git config --global --add datalad.result-hook.readme.match-json '{"type": "dataset","action":"create","status":"ok"}'

Hook definition:

git config --global --add datalad.result-hook.readme.call-json 'run {{"cmd":"cp ~/Templates/standard-readme.txt {path}/README", "outputs":"["README"]", "dataset":"{path}","explicit":true}}'

The important part here is "outputs":"["README"]". I need to give the output definition as a list with the string. I thought I once PR'ed an assure_list(), but could only find this for create...

This fails with (Debug output):

[WARNING] Invalid argument specification for hook readme (after parameter substitutions): {"cmd":"cp ~/Templates/standard-readme.txt /tmp/ads18/README", "outputs":"["README"]", "dataset":"/tmp/ads18","explicit":true} [Expecting ',' delimiter: line 1 column 77 (char 76) [decoder.py:raw_decode:353]], hook will be skipped 

Because in "outputs":"["README"]", only the "[" part is considered. Switching to single quotes ("outputs":"['README']") or leaving quotation marks completely (both lead to no quotes in the config file) leads to README being split into its components:

[DEBUG  ] Resolved dataset for saving: /tmp/ads21 
[DEBUG  ] Determined class of decorated function: <class 'datalad.core.local.status.Status'> 
[DEBUG  ] Resolved dataset for status reporting: /tmp/ads21 
[DEBUG  ] Resolved dataset for path resolution: /tmp/ads21 
[ERROR  ] path not underneath this dataset [status(/tmp/A)] 
[DEBUG  ] Resolved dataset for path resolution: /tmp/ads21 
[ERROR  ] path not underneath this dataset [status(/tmp/D)] 
[DEBUG  ] Resolved dataset for path resolution: /tmp/ads21 
[ERROR  ] path not underneath this dataset [status(/tmp/E)] 
[DEBUG  ] Resolved dataset for path resolution: /tmp/ads21 
[ERROR  ] path not underneath this dataset [status(/tmp/E)] 
[DEBUG  ] Resolved dataset for path resolution: /tmp/ads21 
[ERROR  ] path not underneath this dataset [status(/tmp/M)] 
[DEBUG  ] Resolved dataset for path resolution: /tmp/ads21 
[ERROR  ] path not underneath this dataset [status(/tmp/R)] 
[DEBUG  ] Resolved dataset for path resolution: /tmp/ads21 
[ERROR  ] path not underneath this dataset [status(/tmp/[)] 
[DEBUG  ] Resolved dataset for path resolution: /tmp/ads21 
[ERROR  ] path not underneath this dataset [status(/tmp/])] 
[DEBUG  ] Determined 0 datasets for saving from input arguments 
[DEBUG  ] chdir '/tmp' -> '/tmp' (coming back) 
[DEBUG  ] could not perform all requested actions: Command did not complete successfully [{'action': 'status', 'path': PosixPath('/tmp/A'), 'refds': '/tmp/ads21', 'status': 'error', 'message': 'path not underneath this dataset'}, {'action': 'status', 'path': PosixPath('/tmp/D'), 'refds': '/tmp/ads21', 'status': 'error', 'message': 'path not underneath this dataset'}, {'action': 'status', 'path': PosixPath('/tmp/E'), 'refds': '/tmp/ads21', 'status': 'error', 'message': 'path not underneath this dataset'}, {'action': 'status', 'path': PosixPath('/tmp/E'), 'refds': '/tmp/ads21', 'status': 'error', 'message': 'path not underneath this dataset'}, {'action': 'status', 'path': PosixPath('/tmp/M'), 'refds': '/tmp/ads21', 'status': 'error', 'message': 'path not underneath this dataset'}, {'action': 'status', 'path': PosixPath('/tmp/R'), 'refds': '/tmp/ads21', 'status': 'error', 'message': 'path not underneath this dataset'}, {'action': 'status', 'path': PosixPath('/tmp/['), 'refds': '/tmp/ads21', 'status': 'error', 'message': 'path not underneath this dataset'}, {'action': 'status', 'path': PosixPath('/tmp/]'), 'refds': '/tmp/ads21', 'status': 'error', 'message': 'path not underneath this dataset'}] [utils.py:generator_func:495] 

I've unsucessfully tried escaping single or double quotes in the config call as well (e.g., git config --global --replace-all datalad.result-hook.readme.call-json 'run {{"cmd":"cp ~/Templates/standard-readme.txt {path}/README", "outputs":"[\'README\']", "dataset":"{path}","explicit":true}}'. Do you see a way of giving a list with a string to the hook definition @mih?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I will try it myself, but why "["README"]" instead of ["README"]. This is valid JSON for a list.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems to work fine without the outer quotes.

% git config --add datalad.result-hook.readme.match-json '{"type": "dataset","action":"create","status":"ok"}'
% git config --add datalad.result-hook.readme.call-json 'run {{"cmd":"cp ~/Templates/standard-readme.txt {path}/README", "outputs":["README"], "dataset":"{path}","explicit":true}}'
% datalad create -d . subds1
[INFO   ] Creating a new annex repo at /tmp/testhook/subds1 
add(ok): subds1 (file)                                                                                                       
add(ok): .gitmodules (file)
save(ok): . (dataset)
create(ok): subds1 (dataset)
[INFO   ] == Command start (output follows) ===== 
[INFO   ] == Command exit (modification check follows) ===== 
(datalad3-dev) 1 mih@meiner /tmp/testhook (git)-[master] % ls subds1
README
(datalad3-dev) mih@meiner /tmp/testhook (git)-[master] % cat subds1/README
dummy

docs/basics/101-145-hooks.rst Outdated Show resolved Hide resolved
@adswa
Copy link
Contributor Author

adswa commented Dec 16, 2019

FTR: I've addressed everything but the command example that I could not yet get to work.

docs/basics/101-145-hooks.rst Outdated Show resolved Hide resolved
@mih mih self-requested a review December 17, 2019 08:11
Copy link
Collaborator

@mih mih left a comment

Choose a reason for hiding this comment

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

With the last two minor bits changed, this is good to go! Thx!

@adswa
Copy link
Contributor Author

adswa commented Dec 18, 2019

This is done, will merge.

@adswa adswa merged commit 01d842e into master Dec 18, 2019
@adswa adswa deleted the resulthooks branch December 18, 2019 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants