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

BF: Perform result rendering prior error handling #3863

Merged
merged 1 commit into from Dec 9, 2019

Conversation

mih
Copy link
Member

@mih mih commented Nov 12, 2019

Otherwise we might not be able to act on error results (depending on 'on-failure' setting), and also will not get them to a result renderer.

This is a surprisingly old problem.

@mih mih added severity-normal UX labels Nov 12, 2019
@mih
Copy link
Member Author

@mih mih commented Nov 12, 2019

Test failures are unrelated.

@kyleam
Copy link
Contributor

@kyleam kyleam commented Nov 12, 2019

Otherwise we might not be able to act on error results (depending on 'on-failure' setting),

Could you provide a snippet or test to demonstrate a scenario this describes? Is "act on" restricted to callers that are inspecting the rendered output?

and also will not get them to a result renderer.

Before this change, this snippet

cd $(mktemp -d --tmpdir dl-XXXXXXX)
datalad create ds >/dev/null 2>&1
datalad -f json_pp --on-failure=stop status -d ds not-in-ds

produces

[ERROR  ] path not underneath this dataset [status(/tmp/dl-kiUevIC/not-in-ds)] 

After it produces

[ERROR  ] path not underneath this dataset [status(/tmp/dl-axRCRFa/not-in-ds)] 
{
  "action": "status",
  "path": "/tmp/dl-axRCRFa/not-in-ds",
  "refds": "/tmp/dl-axRCRFa/ds",
  "status": "error"
}

Makes sense.

@mih
Copy link
Member Author

@mih mih commented Nov 12, 2019

@kyleam thanks for answering this question. This illustrates the core of the matter. I actually stumbled upon this issue, because I have result logging disabled locally, and ran a create in a dirty directory, which came back with no error message and no dataset. I also had the impression that there was (is?) a chance that it came back with exit code zero under some conditions, but I could not figure out how it happened (maybe I was mistaken).

@kyleam
Copy link
Contributor

@kyleam kyleam commented Nov 12, 2019

@kyleam thanks for answering this question. This illustrates the core of the matter.

Oh, I actually wasn't intending to answer my own question :) With that snippet, I was just trying to work through the other part of the commit message, the quoted bit right above.

The main thing I wasn't sure about and was trying to clarify with the question is whether "not being able to act on" is about a caller not getting the rendered result on stdout, or whether the ordering here somehow affects Python callers working with the returned results (I don't think that's the case).

I actually stumbled upon this issue, because I have result logging disabled locally, and ran a create in a dirty directory, which came back with no error message and no dataset.

Got it.

The change in this PR looks right, but I think for create more is needed (on top of an otherwise vanilla datalad config) to make create show the result because of creates default result filter:

$ git config datalad.log.result-level debug
$ datalad --on-failure=stop create  # existing repo; nothing shown
$ datalad --on-failure=stop --report-status=error create
create(error): /tmp/dlc-Zs1BWVL (dataset) [will not create a dataset in a non-empty directory, use `force` option to ignore]

Anyway, assuming nothing that I wrote above reveals a misunderstanding on my part, this code movement looks correct to me.

kyleam
kyleam approved these changes Nov 12, 2019
@mih
Copy link
Member Author

@mih mih commented Nov 12, 2019

Arrgh, I missed the filter... thx! Will have a closer look.

@mih
Copy link
Member Author

@mih mih commented Nov 18, 2019

Changes here are broken. They will actually cause dataset to not give a non-zero exit code on create error. Not yet sure how exactly.

@mih mih added the do not merge label Nov 18, 2019
@kyleam
Copy link
Contributor

@kyleam kyleam commented Nov 18, 2019

Changes here are broken. They will actually cause dataset to not give a non-zero exit code on create error. Not yet sure how exactly.

It comes down to creates result filter again. Because of the result filter, _process_results will abort early:

if result_filter:
try:
if not result_filter(res):
raise ValueError('excluded by filter')
except ValueError as e:
lgr.debug('not reporting result (%s)', exc_str(e))
continue

And that means it never appends to incomplete_results:

incomplete_results.append(res)

Otherwise a user might not become aware of the specific nature of error
results (depending on 'on-failure' setting), because we will not get them
to a result renderer.
@codecov
Copy link

@codecov codecov bot commented Dec 9, 2019

Codecov Report

Merging #3863 into master will increase coverage by 9.14%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3863      +/-   ##
==========================================
+ Coverage   71.01%   80.15%   +9.14%     
==========================================
  Files         272      273       +1     
  Lines       36065    36012      -53     
==========================================
+ Hits        25611    28867    +3256     
+ Misses      10454     7145    -3309
Impacted Files Coverage Δ
datalad/interface/utils.py 88.32% <90.9%> (+18.67%) ⬆️
datalad/support/archives.py 61.32% <0%> (-22.66%) ⬇️
datalad/log.py 46.15% <0%> (-19.24%) ⬇️
datalad/support/cookies.py 59.7% <0%> (-16.42%) ⬇️
datalad/support/tests/test_cookies.py 85.71% <0%> (-14.29%) ⬇️
datalad/customremotes/base.py 67.31% <0%> (-11.01%) ⬇️
datalad/customremotes/tests/__init__.py 91.66% <0%> (-8.34%) ⬇️
datalad/tests/test_base.py 96.55% <0%> (-3.45%) ⬇️
datalad/customremotes/tests/test_archives.py 86.27% <0%> (-3.27%) ⬇️
datalad/support/locking.py 95.12% <0%> (-2.44%) ⬇️
... and 88 more

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 6031944...9eac224. Read the comment docs.

@mih
Copy link
Member Author

@mih mih commented Dec 9, 2019

I have force-pushed on update that maintains the sequence of error handling and result filtering in this code move. This should address the remaining issue. Moreover, I have edited the commit message to no longer talk about a vague "act on":

Otherwise a user might not become aware of the specific nature of error
results (depending on 'on-failure' setting), because we will not get them
to a result renderer.

@mih mih removed the do not merge label Dec 9, 2019
@mih
Copy link
Member Author

@mih mih commented Dec 9, 2019

Windows test failure looks familiar and unrelated. appveyor died on coverage submission, but passes otherwise.

@mih mih merged commit 534f763 into datalad:master Dec 9, 2019
13 of 15 checks passed
@mih mih deleted the bf-errorrendering branch Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
severity-normal UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants