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

Reduce messaging to pyout on progress of downloads #1554

Merged
merged 5 commits into from
Dec 17, 2024

Conversation

yarikoptic
Copy link
Member

In an attempt to help with

Also includes small tune up for summary over messages

@yarikoptic yarikoptic requested a review from jwodder December 16, 2024 17:02
@yarikoptic yarikoptic added the performance Improve performance of an existing feature label Dec 16, 2024
dandi/download.py Show resolved Hide resolved
dandi/download.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 16, 2024

Codecov Report

Attention: Patch coverage is 86.84211% with 5 lines in your changes missing coverage. Please review.

Project coverage is 88.52%. Comparing base (dc938f6) to head (2f74075).
Report is 7 commits behind head on master.

Files with missing lines Patch % Lines
dandi/download.py 87.87% 4 Missing ⚠️
dandi/support/pyout.py 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1554      +/-   ##
==========================================
+ Coverage   88.49%   88.52%   +0.03%     
==========================================
  Files          78       78              
  Lines       10742    10736       -6     
==========================================
- Hits         9506     9504       -2     
+ Misses       1236     1232       -4     
Flag Coverage Δ
unittests 88.52% <86.84%> (+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.

…r file or when done

This is primarily to resolve the problem of pyout "dragging us behind" which
slows down entire download process.

#1549
…sted here/before logic

Now, that to filter out some progress records we rely on having "done%" reported,
reflect that we are reportint it for final records
OPT: this avoids 2nd loop (after Counter) over the .files in .messge property
which was used only to report stats anyways.  This could be notable while
reporting on a zarr with 100_000 or more files
For that exception we did get logged:

    ❯ grep Assertion /home/yoh/.local/state/dandi-cli/log/2024.12.16*
    /home/yoh/.local/state/dandi-cli/log/2024.12.16-15.24.12Z-119733.log:AssertionError: attempts_allowed_or_not is None of type <class 'NoneType'>

which makes little sense (besides some thread unsafe handling or None becoming
bool(True))). To overcome and avoid need for assert, just fold that
condition directly into "if" statement.
@yarikoptic yarikoptic requested a review from jwodder December 17, 2024 00:15
@yarikoptic yarikoptic added the release Create a release when this pr is merged label Dec 17, 2024
@yarikoptic yarikoptic merged commit 03af701 into master Dec 17, 2024
25 of 26 checks passed
@yarikoptic yarikoptic deleted the rf-pyoutefficiency branch December 17, 2024 14:36
Copy link

🚀 PR was released in 0.66.2 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Improve performance of an existing feature release Create a release when this pr is merged released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants