fix(ex_app): retry model downloads when the host throttles - #460
Conversation
huggingface.co answers 429 to unauthenticated downloads under load, and a single such answer aborted fetch_models_task, so the whole ExApp init failed. It is also the most frequent reason our own CI goes red. Retry the statuses that mean "try again" (408, 425, 429 and 5xx), following Retry-After when the server sends a usable one and backing off exponentially otherwise. Waits are capped at a minute so a hoster asking for an hour cannot hang an init, the number of extra attempts defaults to 5 and can be set per model with the new max_retries option. Statuses that will not change, 404 for example, still fail on the first answer. Extract the "file already on disk" check into a helper to keep the download function within the local-variable budget. Signed-off-by: Oleksandr Piskun <oleksandr2088@icloud.com>
|
Warning Review limit reached
Next review available in: 4 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughDirect model downloads now retry selected transient HTTP responses. The flow supports bounded ChangesDirect model download retries
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant fetch_models_task
participant niquests.get
participant ModelHost
fetch_models_task->>niquests.get: Request direct model URL
niquests.get->>ModelHost: Fetch model
ModelHost-->>niquests.get: Return transient or successful response
niquests.get-->>fetch_models_task: Return response
fetch_models_task->>fetch_models_task: Apply bounded delay
fetch_models_task->>fetch_models_task: Validate existing file
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests_unit/test_fetch_model_file.py`:
- Around line 222-251: Update FakeResponse and TestFetchModelRetries to track
response closure by adding a close() method that records invocation, then retain
references to the throttled responses and assert each is closed before the
subsequent request. Ensure the test exposes missing close() calls rather than
allowing contextlib.suppress to hide them.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 030031db-6c4b-4961-b9e0-001737ea6b12
📒 Files selected for processing (3)
CHANGELOG.mdnc_py_api/ex_app/integration_fastapi.pytests_unit/test_fetch_model_file.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #460 +/- ##
==========================================
+ Coverage 94.79% 94.89% +0.09%
==========================================
Files 50 50
Lines 5767 5875 +108
==========================================
+ Hits 5467 5575 +108
Misses 300 300
🚀 New features to boost your workflow:
|
The retry loop closes a throttled answer before asking again, because its body is never read and the connection would otherwise stay checked out. Nothing verified that: the fake response had no close(), and suppressing every exception around the call meant even a misspelled method name kept the tests green while leaking a connection per retry in production. Track close() on the fake and assert it, and narrow the guard to OSError so transport errors are still ignored while a wrong call surfaces. Signed-off-by: Oleksandr Piskun <oleksandr2088@icloud.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nc_py_api/ex_app/integration_fastapi.py (2)
287-291: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRedact the model URL before logging.
If
model_pathis a signed URL, Lines [289]-[290] log its query credentials on every retry. This exposes credentials even when the download later succeeds. Log only a redacted host and path. Apply the same redaction to any final failure message that contains the direct URL.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nc_py_api/ex_app/integration_fastapi.py` around lines 287 - 291, Update the retry warning in the download flow around model_path to log only its redacted host and path, never query credentials from a signed URL. Apply the same redaction to the final failure message that reports the direct URL, reusing the existing URL-redaction utility if available.
315-322: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAvoid mutating
models_to_fetchoptions.
set_handlersreuses the same mapping for each/initrequest.__fetch_model_as_fileremovessave_pathandmax_retries, thenfetch_models_taskaddspath. A second request loses the configured save path and retry count. Copy the options before modifying them. Add a repeated-init test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nc_py_api/ex_app/integration_fastapi.py` around lines 315 - 322, In __fetch_model_as_file, copy the model options mapping before popping save_path and max_retries so the shared models_to_fetch configuration remains unchanged across /init requests. Preserve fetch_models_task’s path handling, and add a test that performs repeated initialization with the same options and verifies the configured save path and retry count are retained.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests_unit/test_fetch_model_file.py`:
- Around line 242-256: The test_retries_until_the_download_succeeds test does
not enforce the documented five-retry default. Extend throttled to five
responses before FakeResponse, then assert mocked.call_count is 6 and
sleep.call_count is 5 while preserving the existing connection-closure
assertions.
---
Outside diff comments:
In `@nc_py_api/ex_app/integration_fastapi.py`:
- Around line 287-291: Update the retry warning in the download flow around
model_path to log only its redacted host and path, never query credentials from
a signed URL. Apply the same redaction to the final failure message that reports
the direct URL, reusing the existing URL-redaction utility if available.
- Around line 315-322: In __fetch_model_as_file, copy the model options mapping
before popping save_path and max_retries so the shared models_to_fetch
configuration remains unchanged across /init requests. Preserve
fetch_models_task’s path handling, and add a test that performs repeated
initialization with the same options and verifies the configured save path and
retry count are retained.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 06d1dfa7-35f1-4cb8-bdd0-9948b2710770
📒 Files selected for processing (2)
nc_py_api/ex_app/integration_fastapi.pytests_unit/test_fetch_model_file.py
Nothing enforced the default that the docstring and the changelog promise: lowering it to three or four kept every test green. Exhaust it instead of satisfying it, since serving a success after five throttled answers also passes with a larger default, the loop returning as soon as it gets one. Signed-off-by: Oleksandr Piskun <oleksandr2088@icloud.com>
1 participant
huggingface.coanswers429to unauthenticated downloads under load, and a single such answer abortedfetch_models_task, failing the whole ExAppinitwithModelFetchError: ... returned (429) <!DOCTYPE html>. It is also the most frequent reason our own CI goes red: 4 of the 5 recentGenerate coverage report (2)failures I sampled were exactly this.408,425,429,500,502,503,504)Retry-Afterwhen the server sends a usable one, exponential backoff otherwise, both capped at 60s so a hoster asking for an hour cannot hang an initmax_retriesdownload option,0restores the old behaviour404) still fail on the first answerVerified against a local HTTP server that throttles like the real one: before, 1 request then
ModelFetchError; after, the download succeeds on the 3rd request, and a permanently throttling host still fails, after 6 requests. Plus 7 unit tests covering attempt counts, theRetry-Afterforms and the backoff.Summary by CodeRabbit
New Features
Retry-Aftervalues and use capped exponential backoff.Bug Fixes
Documentation