Repository navigation
[utils] Download to a temporary file, so an interrupted download leaves no truncated file - #2204
Conversation
…es no truncated file
felixdittrich92
added
type: enhancement
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2204 +/- ##
=======================================
Coverage 97.77% 97.78%
=======================================
Files 170 170
Lines 10980 10993 +13
=======================================
+ Hits 10736 10749 +13
Misses 244 244
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Thanks, downloading to a temporary file is a good fix: an interrupted download no longer leaves a truncated file that the next call would reuse. One issue with the fixed .part name, though:
Concurrent downloads of the same file now fail
Every call uses the same "{filename}.part". When two processes download the same file into the same cache at the same time (e.g. several web-server workers calling ocr_predictor(pretrained=True) on an empty cache), they write to the same .part file. The first one to finish renames it, and the second one's os.replace then fails:
FileNotFoundError: [Errno 2] No such file or directory: '.../model.pt.part' -> '.../model.pt'
I ran two processes downloading the same file into the same cache: on main both succeed, with this PR the second one crashes. A unique temporary name per call fixes it:
tmp_path = Path(f"{filename}.{uuid.uuid4().hex}.part")
(plus import uuid at the top). With this change, both concurrent downloads succeed, the final file is intact, no temporary file is left behind, and your test still passes.
Please avoid tempfile.mkstemp for this: it creates the file with mode 0600, and os.replace would carry that onto the cached model, which can break a cache shared between users. The uuid name keeps the usual permissions (644).
Could you also add a test that a download doesn't touch another download in progress? This one fails with the current fixed name and passes with the uuid one
|
Thanks, you're right: both calls shared the same .part file. It now uses a name unique to each call |
_urlretrievewrote the download straight to its final path. When the download was interrupted (dropped connection, Ctrl+C), the truncated file stayed there, and the nextdownload_from_urlcall reused it as is when there was no hash to check (a URL without a hash in its file name and nohash_prefix), so loading the file failed every time until it was deleted by hand.The file is now downloaded to
<name>.partand moved to its final path only once complete; the partial file is removed if the download fails