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

download: more consistent and exhaustive logging, new DANDI_DEVEL_AGGRESSIVE_RETRY mode, respect (?) Retry-After #1509

Merged
merged 17 commits into from
Nov 15, 2024

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Sep 27, 2024

It was originally developed as part of the PR but derailed into fixing/robustifying download functionality. So sits on top of

TODOs

@yarikoptic yarikoptic added minor Increment the minor version when merged cmd-download labels Sep 27, 2024
Copy link

codecov bot commented Sep 27, 2024

Codecov Report

Attention: Patch coverage is 72.72727% with 30 lines in your changes missing coverage. Please review.

Project coverage is 88.51%. Comparing base (6aa414c) to head (7f97394).
Report is 25 commits behind head on master.

Files with missing lines Patch % Lines
dandi/download.py 35.71% 27 Missing ⚠️
dandi/cli/command.py 0.00% 1 Missing ⚠️
dandi/dandiapi.py 80.00% 1 Missing ⚠️
dandi/tests/test_download.py 98.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1509      +/-   ##
==========================================
- Coverage   88.58%   88.51%   -0.08%     
==========================================
  Files          78       78              
  Lines       10589    10689     +100     
==========================================
+ Hits         9380     9461      +81     
- Misses       1209     1228      +19     
Flag Coverage Δ
unittests 88.51% <72.72%> (-0.08%) ⬇️

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.

Also shortened the log line to not include traceback
not sure why was not failing for me locally but fails on CI
That is my guess for what is happening in

    ________________________ test_DownloadDirectory_basic _________________________
    dandi\tests\test_download.py:1048: in test_DownloadDirectory_basic
        with DownloadDirectory(tmp_path, digests={}) as dl:
    dandi\download.py:889: in __exit__
        self.writefile.replace(self.filepath)
    C:\hostedtoolcache\windows\Python\3.10.11\x64\lib\pathlib.py:1247: in replace
        self._accessor.replace(self, target)
    E   PermissionError: [WinError 5] Access is denied: 'C:\\Users\\runneradmin\\AppData\\Local\\Temp\\pytest-of-runneradmin\\pytest-0\\test_DownloadDirectory_basic0.dandidownload\\file' -> 'C:\\Users\\runneradmin\\AppData\\Local\\Temp\\pytest-of-runneradmin\\pytest-0\\test_DownloadDirectory_basic0'
Very unlikely but it could be that directory already existed but without
checksum file for some reason.
no v prefix since is not providing any information on top; sorting for deterministic order
…IVE_RETRY mode

This is all to address that odd case with 000026 where connection keeps interrupting.
Unclear why so adding more specific cases handling and allowing for such an aggressive
retrying where we would proceed as long as we are getting something (but sleep would also increase)
@yarikoptic
Copy link
Member Author

FTR: rebased/force-pushed to get a fresh run of tests after recent changes and drop of 3.8 support.

pip fails to deal with metadata of 0.5.0 due to missing __version__
…basic on windows

Somehow that causes "indigestion" to pytest process later in its life cycle
@yarikoptic
Copy link
Member Author

yarikoptic commented Nov 14, 2024

ok

overall -- we are "ready" here as testing concerned.

@yarikoptic yarikoptic added the release Create a release when this pr is merged label Nov 15, 2024
@yarikoptic yarikoptic merged commit 3671917 into master Nov 15, 2024
22 of 25 checks passed
@yarikoptic yarikoptic deleted the enh-log2 branch November 15, 2024 21:01
Copy link

🚀 PR was released in 0.64.0 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd-download minor Increment the minor version when merged release Create a release when this pr is merged released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant