BF: guard download_generator to not propagate errors #1008

yarikoptic · 2022-05-11T18:10:18Z

This way instead of dumping all tracebacks at the end, some times for benign
FileExistsError -- they will be displayed neatly in pyout and also output
in detail in the log.

Closes #1007
Closes #1030 (thanks @jwodder for pointing that out below)

added explicit on_error option for download_generator so we could still regularly raise in "-f debug"
includes commit to RF imports of type checks for older pythons so it is easier to find and later RF away for new minimal support versions of python.

dandi/download.py

codecov · 2022-05-11T18:18:40Z

Codecov Report

Attention: Patch coverage is 95.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.56%. Comparing base (84f3ead) to head (f72c2a8).
Report is 956 commits behind head on master.

Files	Patch %	Lines
dandi/download.py	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1008      +/-   ##
==========================================
+ Coverage   88.53%   88.56%   +0.03%     
==========================================
  Files          73       74       +1     
  Lines        9295     9307      +12     
==========================================
+ Hits         8229     8243      +14     
+ Misses       1066     1064       -2

Flag	Coverage Δ
unittests	`88.56% <95.00%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yarikoptic · 2022-05-11T18:24:57Z

apparently there is a test which ensured that individual calls would raise exception

dandi/tests/test_download.py:53: in test_download_000027
    download(url, tmp_path)
E   Failed: DID NOT RAISE <class 'FileExistsError'>

so may be this PR "wouldn't work as is" since it would change semantic of download python call... later then, converting to draft

…rts for type checking

This way instead of dumping all tracebacks at the end, some times for benign FileExistsError -- they will be displayed neatly in pyout and also output in detail in the log by default in the case of pyout output handling. But in "debug" mode we will not do anything like that. Closes #1007

dandi/tests/fixtures.py

lgtm-com · 2022-08-03T15:04:19Z

This pull request introduces 1 alert when merging 4001ad5 into 84f3ead - view on LGTM.com

new alerts:

1 for Unused import

jwodder · 2022-08-03T15:09:50Z

I believe this PR also addresses #1030.

jwodder · 2022-08-03T15:14:30Z

dandi/download.py

+def _download_generator_guard(path: str, generator: Iterator[dict]) -> Iterator[dict]:
+    try:
+        for resp in generator:
+            yield dict(resp, path=path)


We're already adding path to the dict on lines 323 and 326. There's no reason to do it again here, and there's a possibility that doing so would mess with pyout.

jwodder · 2022-08-03T15:16:14Z

dandi/download.py

+        yield {
+            "path": path,
+            "status": "error",
+            "message": str(exc.__class__.__name__),


Are you sure you don't want to include the error message via str(exc)?

ok, let's do str

Suggested change

"message": str(exc.__class__.__name__),

"message": str(exc),

I went back on this since then it becomes "too verbose" with each message just repeating the known path and summary duplicating all those

$> dandi download DANDI:000029 PATH SIZE DONE DONE% CHECKSUM STATUS MESSAGE dandiset.yaml skipped no change ...64/sub-anm369964_behavior+ecephys.nwb error File './000029/sub... ...63/sub-anm369963_behavior+ecephys.nwb error File './000029/sub... ...62/sub-anm369962_behavior+ecephys.nwb error File './000029/sub... sub-RAT123/sub-RAT123.nwb error File './000029/sub... sample.txt error File './000029/sam... sample.json error File './000029/sam... Summary: 0 Bytes 0 Bytes 1 skipped 1 no change +20.7 MB 0.00% 6 error 1 File './000029/s... 1 File './000029/s... 1 File './000029/s... 1 File './000029/s... 1 File './000029/s... 1 File './000029/s... 2022-08-03 11:38:10,873 [ INFO] Logs saved in /home/yoh/.cache/dandi-cli/log/20220803153807Z-1022747.log (dev3) 1 41191.....................................:Wed 03 Aug 2022 11:38:12 AM EDT:. lena:/tmp $> dandi download DANDI:000029 PATH SIZE DONE DONE% CHECKSUM STATUS MESSAGE dandiset.yaml skipped no change sub-anm369964/sub-anm369964_behavior+ecephys.nwb error File './000029/sub-anm369964/sub-anm369964_behavior+ecephys.nwb' already exists sub-anm369963/sub-anm369963_behavior+ecephys.nwb error File './000029/sub-anm369963/sub-anm369963_behavior+ecephys.nwb' already exists sub-anm369962/sub-anm369962_behavior+ecephys.nwb error File './000029/sub-anm369962/sub-anm369962_behavior+ecephys.nwb' already exists sub-RAT123/sub-RAT123.nwb error File './000029/sub-RAT123/sub-RAT123.nwb' already exists sample.txt error File './000029/sample.txt' already exists sample.json error File './000029/sample.json' already exists Summary: 0 Bytes 0 Bytes 1 skipped 1 no change +20.7 MB 0.00% 6 error 1 File './000029/sub-anm369964/sub-anm369964_behavior+ecephys.nwb' already exists 1 File './000029/sub-anm369963/sub-anm369963_behavior+ecephys.nwb' already exists 1 File './000029/sub-anm369962/sub-anm369962_behavior+ecephys.nwb' already exists 1 File './000029/sub-RAT123/sub-RAT123.nwb' already exists 1 File './000029/sample.txt' already exists 1 File './000029/sample.json' already exists

with only reporting the class we have better depiction with details of exception present in the log:

$> dandi download DANDI:000029 PATH SIZE DONE DONE% CHECKSUM STATUS MESSAGE dandiset.yaml skipped no change sub-anm369964/sub-anm369964_behavior+ecephys.nwb error FileExistsError sub-anm369963/sub-anm369963_behavior+ecephys.nwb error FileExistsError sub-anm369962/sub-anm369962_behavior+ecephys.nwb error FileExistsError sub-RAT123/sub-RAT123.nwb error FileExistsError sample.txt error FileExistsError sample.json error FileExistsError Summary: 0 Bytes 0 Bytes 1 skipped 1 no change +20.7 MB 0.00% 6 error 6 FileExistsError 2022-08-03 11:41:08,828 [ INFO] Logs saved in /home/yoh/.cache/dandi-cli/log/20220803154105Z-1024172.log

dandi/download.py

Co-authored-by: John T. Wodder II <jwodder@users.noreply.github.com>

lgtm-com · 2022-08-03T15:47:29Z

This pull request introduces 1 alert when merging f72c2a8 into 84f3ead - view on LGTM.com

new alerts:

1 for Unused import

yarikoptic requested a review from jwodder May 11, 2022 18:10

jwodder reviewed May 11, 2022

View reviewed changes

dandi/download.py Outdated Show resolved Hide resolved

yarikoptic marked this pull request as draft May 11, 2022 18:25

yarikoptic force-pushed the bf-download-no-exception branch from debdf1f to 8db314c Compare August 3, 2022 14:24

yarikoptic added 2 commits August 3, 2022 10:58

RF: establish .support.typing for all needed version conditional impo…

fc2035a

…rts for type checking

yarikoptic force-pushed the bf-download-no-exception branch from 8db314c to 4001ad5 Compare August 3, 2022 14:58

yarikoptic commented Aug 3, 2022

View reviewed changes

dandi/tests/fixtures.py Show resolved Hide resolved

yarikoptic marked this pull request as ready for review August 3, 2022 15:03

yarikoptic requested a review from jwodder August 3, 2022 15:05

jwodder requested changes Aug 3, 2022

View reviewed changes

yarikoptic commented Aug 3, 2022

View reviewed changes

dandi/download.py Outdated Show resolved Hide resolved

Suggestions from code review by @jwodder

f72c2a8

Co-authored-by: John T. Wodder II <jwodder@users.noreply.github.com>

yarikoptic force-pushed the bf-download-no-exception branch from ec6761b to f72c2a8 Compare August 3, 2022 15:41

yarikoptic requested a review from jwodder August 3, 2022 17:37

jwodder approved these changes Aug 3, 2022

View reviewed changes

yarikoptic added UX patch Increment the patch version when merged labels Aug 3, 2022

yarikoptic merged commit 1df5fad into master Aug 3, 2022

yarikoptic deleted the bf-download-no-exception branch August 3, 2022 21:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BF: guard download_generator to not propagate errors #1008

BF: guard download_generator to not propagate errors #1008

yarikoptic commented May 11, 2022 •

edited

Loading

codecov bot commented May 11, 2022 •

edited

Loading

yarikoptic commented May 11, 2022

lgtm-com bot commented Aug 3, 2022

jwodder commented Aug 3, 2022

jwodder Aug 3, 2022

jwodder Aug 3, 2022

yarikoptic Aug 3, 2022

yarikoptic Aug 3, 2022

lgtm-com bot commented Aug 3, 2022

BF: guard download_generator to not propagate errors #1008

BF: guard download_generator to not propagate errors #1008

Conversation

yarikoptic commented May 11, 2022 • edited Loading

codecov bot commented May 11, 2022 • edited Loading

Codecov Report

yarikoptic commented May 11, 2022

lgtm-com bot commented Aug 3, 2022

jwodder commented Aug 3, 2022

jwodder Aug 3, 2022

Choose a reason for hiding this comment

jwodder Aug 3, 2022

Choose a reason for hiding this comment

yarikoptic Aug 3, 2022

Choose a reason for hiding this comment

yarikoptic Aug 3, 2022

Choose a reason for hiding this comment

lgtm-com bot commented Aug 3, 2022

yarikoptic commented May 11, 2022 •

edited

Loading

codecov bot commented May 11, 2022 •

edited

Loading