Harden artifact loading for downloads and cache validation#10
Harden artifact loading for downloads and cache validation#10mbsantiago wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens audioclass.utils.load_artifact by improving URL detection, adding resilient HTTP download behavior (timeout + retry/backoff + streaming), and introducing optional integrity checks for downloaded and cached artifacts.
Changes:
- Restrict URL detection to valid HTTP(S) URLs and add a regression test for Windows-like paths.
- Add streaming downloads with configurable timeout, retries, and exponential backoff.
- Add optional cached/downloaded artifact validation via expected SHA256 and/or expected file size, and clean up invalid/partial cache files.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/audioclass/utils.py |
Adds HTTP(S)-only URL detection, download retry/backoff + streaming, and artifact integrity validation with cache cleanup. |
tests/test_utils.py |
Adds coverage for URL detection, timeout/streaming usage, retry behavior, validation failures, and partial-file cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if attempt == retries: | ||
| raise | ||
|
|
There was a problem hiding this comment.
_download_with_retry writes into the same fileobj across retry attempts, but on a requests.RequestException it does not rewind/truncate the file before retrying. If a network error happens mid-stream (e.g., ChunkedEncodingError from iter_content), the next attempt will append to the partial content, producing a corrupted artifact even if the later attempt succeeds. Reset/truncate the file between attempts (or download into a temp file per attempt and only move into place after a successful full download).
| fileobj.seek(0) | |
| fileobj.truncate() |
| basename = Path(urlparse(path).path).name | ||
| new_path = directory / basename | ||
|
|
There was a problem hiding this comment.
basename = Path(urlparse(path).path).name can be empty for valid HTTP(S) URLs that end with / (or otherwise have no filename). In that case new_path becomes the cache directory itself and new_path.open('wb') will fail with IsADirectoryError. Consider explicitly handling the empty-basename case (e.g., derive a filename from the URL hash or raise a clear ValueError).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10 +/- ##
==========================================
- Coverage 90.81% 89.60% -1.22%
==========================================
Files 17 17
Lines 599 654 +55
Branches 61 74 +13
==========================================
+ Hits 544 586 +42
- Misses 29 33 +4
- Partials 26 35 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
load_artifactdownloads to make network fetches more resilientexpected_sha256,expected_size) for both newly downloaded files and cached filesTesting
ruff check src/audioclass/utils.py tests/test_utils.py --select E,F,B,Ipytest tests/test_utils.py(fails in this environment: missing optional dependencysoundfileimported bytests/conftest.py)