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: Retain git-annex error messages & don't show them if operation successful #6070

Merged
merged 1 commit into from
Oct 15, 2021

Conversation

DisasterMo
Copy link
Contributor

Description

Instead of being mashed into a 'normal' message, git-annex reported error messages will now be retained in a separate dict entry.
This allows for more differentiated handling of them. Examples implemented in this PR:

  1. To avoid confusion, git-annex error messages will only be rendered if they are meaningful, i.e. when the operation actually failed.
  2. Error messages are now grouped separately from notes & coloured red.

Related Issues

  1. A successful datalad get can still look quite sad #5617
  2. Distinguish and support multiple messages per result #3939

@DisasterMo DisasterMo changed the base branch from master to maint October 12, 2021 16:56
Copy link
Member

@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.

Thx! I like the direction.

Looking at the test failures, I think they are legit. Some tests expect the error messages to be rendered/provided specifically as they were. Those need to be adjusted.

The PR also includes a number of unrelated formatting changes. I personally very much appreciate this type of cleanup. However, I would prefer to separate no-op formatting changes from topical changes in separate commits. Future forensics will be more straightforward. Not need to disentangle them for this one, just mentioning it.

datalad/interface/clean.py Show resolved Hide resolved
@DisasterMo
Copy link
Contributor Author

DisasterMo commented Oct 13, 2021

Some tests expect the error messages to be rendered/provided specifically as they were. Those need to be adjusted.

On it. I should probably run the tests myself before I submit a PR in the future, huh.

The PR also includes a number of unrelated formatting changes.

Yeah, I had PyCharm do it's magic a couple of times out of habit, sorry.

@@ -238,7 +240,7 @@ def annexjson2result(d, ds, **kwargs):
for k, v in d['fields'].items()
if not k.endswith('lastchanged')}
if d.get('error-messages', None):
messages.extend(d['error-messages'])
res['error_message'] = '\n'.join(m.strip() for m in d['error-messages'])
Copy link
Member

Choose a reason for hiding this comment

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

I wonder whether such a join should only happen in result renderer(s). When using the python interface it may be beneficial to have a structured record in error_message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I merely reproduced what was already being done a couple of lines below:

if messages: res['message'] = '\n'.join(m.strip() for m in messages)

Copy link
Member

Choose a reason for hiding this comment

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

Q is what are the semantics here? Do we know that this is one message with several lines, or several messages. So far the the message key had one purpose: carry a message to be displayed. If it shall become a message-building toolkit instead, we should work this out properly in #6054, and not introduce things in a single location, just for annex messages.

@codecov
Copy link

codecov bot commented Oct 13, 2021

Codecov Report

Merging #6070 (723949c) into maint (3269925) will decrease coverage by 1.82%.
The diff coverage is 83.33%.

❗ Current head 723949c differs from pull request most recent head fcc2a2d. Consider uploading reports for the commit fcc2a2d to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #6070      +/-   ##
==========================================
- Coverage   90.25%   88.43%   -1.83%     
==========================================
  Files         312      312              
  Lines       42214    42217       +3     
==========================================
- Hits        38099    37333     -766     
- Misses       4115     4884     +769     
Impacted Files Coverage Δ
datalad/core/local/tests/test_results.py 100.00% <ø> (ø)
datalad/distributed/tests/test_ria_git_remote.py 100.00% <ø> (ø)
datalad/interface/clean.py 83.33% <ø> (ø)
datalad/interface/results.py 96.94% <66.66%> (-0.73%) ⬇️
datalad/interface/utils.py 95.32% <100.00%> (+0.01%) ⬆️
datalad/version.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/wtf.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/addurls.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/no_annex.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/add_readme.py 0.00% <0.00%> (-100.00%) ⬇️
... and 60 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 3269925...fcc2a2d. Read the comment docs.

@DisasterMo
Copy link
Contributor Author

Regarding AppVeyor error:

AssertionError: '<action-unspecified>(<status-unspecified>):\r\n[funky]' not found in '<action-unspecified>(<status-unspecified>):\n\n[funky]\n'

I use os.linesep in both cases, how can one be \r\n while the other is \n\n?

@bpoldrack
Copy link
Member

@DisasterMo

I use os.linesep in both cases, how can one be \r\n while the other is \n\n?

That's strange indeed. I vaguely remember running into this bc of some meant-to-be-convenient- but-actually-confusing auto conversion python3 does when reading/writing files. Like reading \r\n and returning a string with \n.
The swallow_* helpers do involve writing the outputs to a file (or IO stream). Hence, I'd assume the effect happens within those, not your diff. However, note that the generic test would iterate over the to be matched output list. You could simply provide both lines separately. It would effectively not test for the line separation as assert_in would be true either way, but good enough, I think.

@yarikoptic
Copy link
Member

Windows?

@DisasterMo DisasterMo force-pushed the bf-5617-duplicate-fail-messages branch from 5246dfc to d223782 Compare October 13, 2021 13:28
@mih mih added the semver-patch Increment the patch version when merged label Oct 13, 2021
@DisasterMo DisasterMo force-pushed the bf-5617-duplicate-fail-messages branch from d223782 to 477d86e Compare October 14, 2021 10:36
Copy link
Member

@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.

OK, so with this PR I see:

{                                                                                                                           
  "action": "get",
  "annexkey": "MD5E-s32338--3cb12b68abd59160dffdf4830e1de8bb.nii.gz",
  "error_message": "download failed: Not Found\nfailed to download content\ndownload failed: Not Found\nfailed to download content\ndownload failed: Not Found\nfailed to download content",
  "path": "/tmp/studyforrest-data/derivative/aggregate_fmri_timeseries/sub-10/atlases/bold3Tp2/shen_fconn_atlas_150.nii.gz",
  "refds": "/tmp/studyforrest-data",
  "status": "ok",
  "type": "file"
}

rendered as

get(ok): derivative/aggregate_fmri_timeseries/sub-10/atlases/bold3Tp2/shen_fconn_atlas_150.nii.gz (file)                    
[download failed: Not Found
failed to download content
download failed: Not Found
failed to download content
download failed: Not Found
failed to download content]

which doesn't match the "& don't show them if operation successful" part of the PR title. Is that intentional?

If so, maybe a message prefix ala "intermediate errors occurred: " or something along these lines.

Also I would personally prefer no preemptive linebreaks in the reporting (i.e. before the [)

@@ -238,7 +240,7 @@ def annexjson2result(d, ds, **kwargs):
for k, v in d['fields'].items()
if not k.endswith('lastchanged')}
if d.get('error-messages', None):
messages.extend(d['error-messages'])
res['error_message'] = '\n'.join(m.strip() for m in d['error-messages'])
Copy link
Member

Choose a reason for hiding this comment

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

Q is what are the semantics here? Do we know that this is one message with several lines, or several messages. So far the the message key had one purpose: carry a message to be displayed. If it shall become a message-building toolkit instead, we should work this out properly in #6054, and not introduce things in a single location, just for annex messages.

@DisasterMo
Copy link
Contributor Author

which doesn't match the "& don't show them if operation successful" part of the PR title. Is that intentional?

Absolutely not. I had multiple versions of this code before submitting the PR, and somewhere in the middle the "test" I used (from #5617) stopped working for me. I must have forgotten to re-add the status check at some point after that & didn't notice.

@DisasterMo
Copy link
Contributor Author

Also I would personally prefer no preemptive linebreaks in the reporting (i.e. before the [)

Why? From what I can tell, it is pretty obvious that any message is supposed to be a multi-line block. Personally, I find it incredibly ugly to see a multi-line block not start on a new line.

@DisasterMo
Copy link
Contributor Author

DisasterMo commented Oct 14, 2021

what are the semantics here? Do we know that this is one message with several lines, or several messages.

The original error-messages contains, in general, multiple messages each with at least one line. We can distinguish lines, but not messages. So the general form of d['error-messages'] looks something like this:
["err1line1", "err2line2", "err2line1", ... , "errNlineM"]

Copy link
Member

@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.

OK, this now works as advertised, thx much!

Also I would personally prefer no preemptive linebreaks in the reporting (i.e. before the [)

Why? From what I can tell, it is pretty obvious that any message is supposed to be a multi-line block. Personally, I find it incredibly ugly to see a multi-line block not start on a new line.

Actually, no. Overall, most messages are quite concise single line statements. Now they are forced on a separate line. The [ ... ] format was originally chosen to have some kind of a line/result-continuation indicator.

If the format of the default result rendering is to be changed, I think there should be a dedicated issue about this, and a separate fix -- not piggy-backing on an unrelated change.

@DisasterMo DisasterMo force-pushed the bf-5617-duplicate-fail-messages branch 3 times, most recently from 70f3d53 to 6962030 Compare October 15, 2021 08:13
@DisasterMo DisasterMo force-pushed the bf-5617-duplicate-fail-messages branch from 6962030 to fcc2a2d Compare October 15, 2021 08:14
Copy link
Member

@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.

Great, thx for the update. Please feel free, and encouraged to propose a format change for the default renderer output!

@mih mih merged commit b33cf83 into datalad:maint Oct 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants