Repository navigation
🐛 fix(models): track model availability - #168
Conversation
- 🐛 Defects:
- `model_available` was never derived from the weight being on disk. It
was set only by a `ModelAvailabilityChanged` event, which the Model
Manager dispatches, or as a side effect of a preview advancing from
`PREPARING_MODEL` to `PREVIEWING`. A preview that failed, timed out or
was cancelled after the download finished therefore left the flag
`False` although the weight was cached — and all three surfaces that
read it agreed on the wrong answer: the action button kept offering
`Download & Preview`, the status row kept reading `Not cached`, and the
download notice kept demanding 927.6 MiB. Only opening the Model
Manager corrected it.
- `PreviewController._notification` now dispatches
`ModelAvailabilityChanged(True)` when the download stage reports
completion, so the fact is recorded independently of what the job does
next.
- `ModelDownloader._download_locked` emits its terminal `(size, size)`
progress **after** the checksum is verified and the file is atomically
promoted, never before. A weight that fails verification is deleted, so
reporting it as available on bytes received would have been worse than
the stale label. This also makes completion observable when the server
sends no Content-Length.
- 🧪 Tests:
- `test_completed_model_stays_available_when_preview_setup_fails` and
`test_completed_model_stays_available_when_preview_is_cancelled` pin
the flag together with the button, the status row and the download
notice, so the three cannot drift apart again.
- `test_download_reports_completion_after_verified_weight_is_promoted`
pins that the callback does not fire until the target file exists, and
`test_unknown_content_length_reports_post_promotion_completion` pins
the case the old progress path could not report at all.
- 📝 Copy:
- `Herunterladen & Vorschau` was a word-for-word rendering of
`Download & Preview` — a bare infinitive beside a noun, which is not
how a German action label reads. It is now `Modell laden & Vorschau`,
naming what the button fetches first. `%s — %s Download erforderlich`
becomes the sentence `Für %s ist ein Download von %s erforderlich`.
Trigger: repro — select a model that is not cached, start a preview, let
the download finish and the preview then fail with
`segmentation process timed out`; the inspector still reports the model as
not cached and the button still offers to download it.
Closes #167
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
- 🐛 Defects:
- Two early returns leave a verified weight on disk without reporting
the terminal progress: the pre-flight check before any request is
made, and the post-download check after a concurrent writer won the
race. A caller that reads that progress as "the weight is usable now"
never hears it on either path.
- That is the same defect this branch exists to fix, on the cached path:
`ModelChanged` sets `model_available` to `False`, and selecting a model
that is already downloaded then takes the pre-flight return, so nothing
dispatches `ModelAvailabilityChanged(True)`. If the preview that
follows fails, times out or is cancelled, the inspector again reports a
cached model as missing and offers to download it.
- Every path that leaves a usable weight now reports completion.
- 🧪 `test_an_already_cached_weight_still_reports_completion` downloads
twice and asserts the second, fully cached call reports completion as the
first did. It fails on the pre-flight path without this change; the
post-download path was found by that test rather than by reading, because
fixing only the reported line left the test red.
Trigger: reported by the review bot on PR #168 — the cache-hit early return
at `download.py:266` skips the terminal completion progress.
Refs #167
|
Gitar's finding is correct and fixed in
There were two such returns, not one. Gitar named the post-download check at
Verification after the fix: ruff, mypy on all three platforms, guardrails, |
|
Confirmed against |
Code Review ✅ Approved 1 resolved / 1 findingsFixes model availability tracking so the inspector, button, and status row no longer show stale ✅ 1 resolved✅ Edge Case: Cache-hit early return skips terminal completion progress
OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Important Your trial ends in 7 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more. Was this helpful? React with 👍 / 👎 | Gitar |
1 participant
After a model finished downloading as part of a preview, the inspector kept
describing it as missing and the primary action kept offering to download it
again — a 927.6 MiB download the user had just waited for.
Root cause
model_availablewas never derived from the weight being present on disk. Itwas set in exactly two places: from a
ModelAvailabilityChangedevent, whichonly startup and the Model Manager dispatch, and as a side effect of a preview
job advancing from
JobState.PREPARING_MODELtoPREVIEWING.So the download succeeding was not itself observed. A preview that failed,
timed out or was cancelled after the download completed left the flag
Falsewhile the weight was cached, and the three surfaces that read it all agreed on
the wrong answer:
ui/presenter.py)Download & Previewui/presentation_model.py)Not cachedui/inspector.py)… 927.6 MiB download requiredOnly opening the Model Manager corrected it. The maintainer hit this with a
segmentation process timed out, which is why the timeout and the stale labelarrived together.
Fixing only the timeout path would have left the same defect on the failure
and cancellation paths, so the fix records the fact instead: the completed
download dispatches availability itself, independently of what the job does
next.
Reported only after verification
The terminal
(size, size)progress is emitted after the checksum isverified and the file is atomically promoted, never on bytes received. A weight
that fails verification is deleted, so announcing availability earlier would
have been worse than the stale label — the interface would claim a model is
ready that is about to be removed. This also makes completion observable when
the server sends no
Content-Length, which the previous progress path couldnot report at all.
German copy, also from the issue
Herunterladen & Vorschauwas a word-for-word rendering ofDownload & Preview: a bare infinitive beside a noun, which is not how aGerman action label reads. Now
Modell laden & Vorschau, naming what thebutton fetches first.
%s — %s Download erforderlichbecomes the sentenceFür %s ist ein Download von %s erforderlich.Catalogues: 529 strings, 529 finished, 0 unfinished in both languages.
Tests
test_completed_model_stays_available_when_preview_setup_failsandtest_completed_model_stays_available_when_preview_is_cancelledpin the flagtogether with the button, the status row and the download notice, so the three
cannot drift apart again.
test_download_reports_completion_after_verified_weight_is_promotedpins thatthe callback does not fire until the target file exists, and
test_unknown_content_length_reports_post_promotion_completionpins the casethe old path could not report.
Verification
Closes #167