fix(settings): keep the download dialog visibly working after the last byte (#864)
The transfer ending is not the dialog's work ending. The checksum still has to run and the engine still has to load the file, seconds of it on a large model, and until now that window showed a bar parked at 100% with the downloader's "Complete!" under it and Cancel as the only button. It reads as a finished download waiting to be confirmed, or as a hang, and the one button on offer is the opposite of what the user wants to press. - ModelDownloadDialog: a fraction of 1.0 no longer paints a finished-looking bar. It bounces the bar, names the step, says the download is done but the work is not, and greys out the Cancel button, which cannot interrupt verification or a model load anyway. Only set_complete() says "Complete!" and offers OK. - whisper.cpp: report "Verifying model..." before hashing a file that can be hundreds of megabytes, the same step the Vosk path has always reported, and "Loading model..." before backend detection and the load. The tray's download notification reads the same statuses. - tests: add return annotations on the new finishing-feedback helpers and tests (Greptile P2). Co-authored-by: Jatin K Malik <jatinkrmalik@gmail.com>
This commit is contained in:
Guilherme Feitoza
co-authored by
Jatin K Malik
Jatin K Malik
parent
7b69ac2f46
commit
be77ea40f4
@@ -1893,6 +1893,11 @@ class SpeechRecognitionManager:
|
||||
logger.info(f"Downloading whisper.cpp '{self.model_size}' model...")
|
||||
self._download_whispercpp_model()
|
||||
|
||||
# Backend detection plus the load itself run after the download
|
||||
# reports "Complete!", so a UI watching the download needs to hear
|
||||
# that this is still going. No callback is set outside that flow.
|
||||
if self._download_progress_callback:
|
||||
self._download_progress_callback(1.0, 0, "Loading model...")
|
||||
self._load_whispercpp_model(model_path)
|
||||
|
||||
except ImportError as e:
|
||||
@@ -2957,6 +2962,11 @@ class SpeechRecognitionManager:
|
||||
|
||||
try:
|
||||
self._stream_model_download(url, temp_file)
|
||||
# Say so before hashing: on a multi-hundred-MB model this is
|
||||
# seconds with nothing else to show, and the UI would otherwise
|
||||
# sit on the last chunk's update. Same step the Vosk path reports.
|
||||
if self._download_progress_callback:
|
||||
self._download_progress_callback(1.0, 0, "Verifying model...")
|
||||
# Verify before the rename: whisper.cpp loads ggml files through
|
||||
# ctypes, so an unverified file must never reach its final path
|
||||
# where is_model_downloaded() would treat it as good.
|
||||
|
||||
@@ -2238,6 +2238,11 @@ class SettingsPage:
|
||||
self.extras.append(child)
|
||||
|
||||
|
||||
# What the downloaders report when the transfer itself ends. It is not the
|
||||
# end of the dialog's work, so it must not be shown as one.
|
||||
_DOWNLOAD_DONE_STATUSES = frozenset({"complete", "completed", "done"})
|
||||
|
||||
|
||||
class ModelDownloadDialog(Gtk.Dialog):
|
||||
"""Dialog showing model download progress with cancel support."""
|
||||
|
||||
@@ -2302,15 +2307,26 @@ class ModelDownloadDialog(Gtk.Dialog):
|
||||
|
||||
self.show_all()
|
||||
|
||||
self._pulse_timeout = None
|
||||
# For Whisper, we can't track progress, so pulse
|
||||
if engine == "whisper":
|
||||
self._start_pulsing()
|
||||
|
||||
def _start_pulsing(self) -> None:
|
||||
"""Bounce the bar, for work that reports no measurable progress."""
|
||||
if self._pulse_timeout is None:
|
||||
self._pulse_timeout = GLib.timeout_add(100, self._pulse_progress)
|
||||
else:
|
||||
|
||||
def _stop_pulsing(self) -> None:
|
||||
"""Stop the bounce if it is running."""
|
||||
if self._pulse_timeout is not None:
|
||||
GLib.source_remove(self._pulse_timeout)
|
||||
self._pulse_timeout = None
|
||||
|
||||
def _pulse_progress(self):
|
||||
"""Pulse the progress bar while downloading (for Whisper)."""
|
||||
if self.cancelled:
|
||||
self._pulse_timeout = None
|
||||
return False
|
||||
self.progress_bar.pulse()
|
||||
return True # Continue pulsing
|
||||
@@ -2327,20 +2343,47 @@ class ModelDownloadDialog(Gtk.Dialog):
|
||||
if self.cancelled:
|
||||
return
|
||||
|
||||
# Stop pulsing if we were pulsing
|
||||
if self._pulse_timeout:
|
||||
GLib.source_remove(self._pulse_timeout)
|
||||
self._pulse_timeout = None
|
||||
if fraction >= 1.0:
|
||||
self._show_finishing(status_text)
|
||||
return
|
||||
|
||||
self._stop_pulsing()
|
||||
|
||||
self.progress_bar.set_fraction(fraction)
|
||||
self.progress_bar.set_text(f"{fraction * 100:.0f}%")
|
||||
self.status_label.set_markup(f"<i>{status_text}</i>")
|
||||
|
||||
def _show_finishing(self, status_text: str) -> None:
|
||||
"""Show the work that follows the last byte as work, not as an end.
|
||||
|
||||
The bytes being in is not the dialog being done: the checksum still
|
||||
has to run and the engine still has to load the file, seconds of it
|
||||
on a large model, and neither reports progress or can be interrupted.
|
||||
A bar parked at 100% — or worse, at the downloader's "Complete!" —
|
||||
under a lone Cancel button reads as "finished, waiting for me to
|
||||
confirm", or as a hang. So bounce the bar, name the step, and stop
|
||||
offering a cancel that no longer cancels anything.
|
||||
"""
|
||||
self._start_pulsing()
|
||||
|
||||
step = status_text.strip()
|
||||
if not step or step.strip("!.").casefold() in _DOWNLOAD_DONE_STATUSES:
|
||||
step = "Finishing..."
|
||||
self.progress_bar.set_text(step)
|
||||
self.status_label.set_markup(
|
||||
"<i>Download finished, still working - this can take a moment...</i>"
|
||||
)
|
||||
|
||||
if self.cancel_button.get_sensitive():
|
||||
self.cancel_button.set_sensitive(False)
|
||||
self.cancel_button.set_tooltip_text(
|
||||
"The download is finished. Verifying and loading the model "
|
||||
"cannot be interrupted."
|
||||
)
|
||||
|
||||
def set_complete(self, success: bool, message: str = ""):
|
||||
"""Mark download as complete."""
|
||||
if self._pulse_timeout:
|
||||
GLib.source_remove(self._pulse_timeout)
|
||||
self._pulse_timeout = None
|
||||
self._stop_pulsing()
|
||||
|
||||
# Hide cancel button
|
||||
self.cancel_button.hide()
|
||||
|
||||
@@ -0,0 +1,218 @@
|
||||
"""The window must keep saying it is working after the last byte arrives.
|
||||
|
||||
The transfer ending is not the dialog's work ending: the checksum runs, then
|
||||
the engine loads the file, and on a large model that is seconds. Those seconds
|
||||
used to show a bar parked at 100% with the downloader's "Complete!" under it
|
||||
and Cancel as the only button, which reads as a finished download waiting to be
|
||||
confirmed - or as a hang.
|
||||
"""
|
||||
|
||||
import importlib
|
||||
import sys
|
||||
from typing import Any
|
||||
from unittest.mock import MagicMock, Mock, patch
|
||||
|
||||
from vocalinux.speech_recognition.recognition_manager import SpeechRecognitionManager
|
||||
|
||||
|
||||
def _load_settings_dialog() -> Any:
|
||||
"""Import settings_dialog with real bases so unbound methods stay callable.
|
||||
|
||||
conftest swaps gi for a MagicMock, which leaves every ``class X(Gtk.Y)`` as
|
||||
a mock. Handing the three bases a real class keeps the methods intact.
|
||||
|
||||
Restore both sys.modules and the vocalinux.ui.settings_dialog attribute, so
|
||||
mock and later imports see the same module object.
|
||||
"""
|
||||
import vocalinux.ui as ui_pkg
|
||||
|
||||
repository = sys.modules["gi.repository"]
|
||||
bases = {name: type(name, (), {}) for name in ("Box", "ListBoxRow", "Dialog")}
|
||||
saved = sys.modules.pop("vocalinux.ui.settings_dialog", None)
|
||||
try:
|
||||
with patch.object(repository, "Gtk", MagicMock(**bases)):
|
||||
module = importlib.import_module("vocalinux.ui.settings_dialog")
|
||||
finally:
|
||||
sys.modules.pop("vocalinux.ui.settings_dialog", None)
|
||||
if saved is not None:
|
||||
sys.modules["vocalinux.ui.settings_dialog"] = saved
|
||||
ui_pkg.settings_dialog = saved
|
||||
else:
|
||||
ui_pkg.__dict__.pop("settings_dialog", None)
|
||||
return module
|
||||
|
||||
|
||||
settings_dialog = _load_settings_dialog()
|
||||
ModelDownloadDialog = settings_dialog.ModelDownloadDialog
|
||||
|
||||
_REAL_METHODS = (
|
||||
"update_progress",
|
||||
"_show_finishing",
|
||||
"_start_pulsing",
|
||||
"_stop_pulsing",
|
||||
"set_complete",
|
||||
)
|
||||
|
||||
|
||||
def _dialog_stub() -> Mock:
|
||||
"""A ModelDownloadDialog with real progress methods over mocked widgets."""
|
||||
dialog = Mock()
|
||||
dialog.cancelled = False
|
||||
dialog._pulse_timeout = None
|
||||
dialog.cancel_button.get_sensitive.return_value = True
|
||||
for name in _REAL_METHODS:
|
||||
setattr(dialog, name, getattr(ModelDownloadDialog, name).__get__(dialog))
|
||||
return dialog
|
||||
|
||||
|
||||
def _bar_text(dialog: Mock) -> str:
|
||||
return dialog.progress_bar.set_text.call_args[0][0]
|
||||
|
||||
|
||||
def _status_markup(dialog: Mock) -> str:
|
||||
return dialog.status_label.set_markup.call_args[0][0]
|
||||
|
||||
|
||||
def test_download_in_flight_still_shows_the_percentage() -> None:
|
||||
dialog = _dialog_stub()
|
||||
|
||||
with patch.object(settings_dialog, "GLib") as glib:
|
||||
dialog.update_progress(0.42, 3.5, "3.5 MB/s - 2 min left")
|
||||
|
||||
dialog.progress_bar.set_fraction.assert_called_once_with(0.42)
|
||||
assert _bar_text(dialog) == "42%"
|
||||
assert "3.5 MB/s" in _status_markup(dialog)
|
||||
glib.timeout_add.assert_not_called()
|
||||
|
||||
|
||||
def test_finished_transfer_does_not_read_as_a_finished_dialog() -> None:
|
||||
"""The one the bug report is about: "Complete!" while it is not complete."""
|
||||
dialog = _dialog_stub()
|
||||
|
||||
with patch.object(settings_dialog, "GLib"):
|
||||
dialog.update_progress(1.0, 0, "Complete!")
|
||||
|
||||
assert _bar_text(dialog) == "Finishing..."
|
||||
assert "still working" in _status_markup(dialog)
|
||||
|
||||
|
||||
def test_the_bar_moves_while_the_model_is_verified_and_loaded() -> None:
|
||||
"""Motion is what tells the user the app is alive; there is no fraction."""
|
||||
dialog = _dialog_stub()
|
||||
|
||||
with patch.object(settings_dialog, "GLib") as glib:
|
||||
glib.timeout_add.return_value = 77
|
||||
dialog.update_progress(1.0, 0, "Verifying model...")
|
||||
dialog.update_progress(1.0, 0, "Loading model...")
|
||||
|
||||
# Named steps are shown as they come, and the bounce is started once.
|
||||
assert _bar_text(dialog) == "Loading model..."
|
||||
assert glib.timeout_add.call_count == 1
|
||||
assert dialog._pulse_timeout == 77
|
||||
|
||||
|
||||
def test_cancel_stops_being_offered_once_it_cannot_cancel() -> None:
|
||||
dialog = _dialog_stub()
|
||||
|
||||
with patch.object(settings_dialog, "GLib"):
|
||||
dialog.update_progress(1.0, 0, "Verifying model...")
|
||||
|
||||
dialog.cancel_button.set_sensitive.assert_called_once_with(False)
|
||||
assert dialog.cancel_button.set_tooltip_text.called
|
||||
|
||||
|
||||
def test_set_complete_ends_the_bounce_and_offers_the_way_out() -> None:
|
||||
dialog = _dialog_stub()
|
||||
|
||||
with patch.object(settings_dialog, "GLib") as glib:
|
||||
glib.timeout_add.return_value = 77
|
||||
dialog.update_progress(1.0, 0, "Loading model...")
|
||||
dialog.set_complete(True)
|
||||
|
||||
glib.source_remove.assert_called_once_with(77)
|
||||
assert dialog._pulse_timeout is None
|
||||
assert _bar_text(dialog) == "Complete!"
|
||||
dialog.add_button.assert_called_once()
|
||||
|
||||
|
||||
def _make_manager() -> SpeechRecognitionManager:
|
||||
"""A whisper.cpp manager whose engine init is stubbed out."""
|
||||
with (
|
||||
patch.object(SpeechRecognitionManager, "_init_vosk"),
|
||||
patch.object(SpeechRecognitionManager, "_init_whisper"),
|
||||
patch.object(SpeechRecognitionManager, "_init_whispercpp"),
|
||||
patch.object(SpeechRecognitionManager, "_init_parakeet"),
|
||||
patch.object(SpeechRecognitionManager, "_init_faster_whisper"),
|
||||
):
|
||||
return SpeechRecognitionManager(
|
||||
engine="whisper_cpp",
|
||||
model_size="small",
|
||||
language="en-us",
|
||||
defer_download=True,
|
||||
)
|
||||
|
||||
|
||||
def test_whispercpp_download_reports_the_verification_before_running_it() -> None:
|
||||
"""Hashing a 465 MB model is seconds of silence if nobody announces it.
|
||||
|
||||
Asserted as ordering, not presence: a status reported after the hash tells
|
||||
the user nothing while they are waiting on it. The Vosk path has reported
|
||||
this step all along.
|
||||
"""
|
||||
manager = _make_manager()
|
||||
statuses = []
|
||||
manager._download_progress_callback = lambda fraction, speed, text: statuses.append(text)
|
||||
seen_when_hashing = []
|
||||
|
||||
requests = MagicMock()
|
||||
response = MagicMock()
|
||||
response.headers = {"content-length": "1000"}
|
||||
response.iter_content.return_value = [b"x" * 1000]
|
||||
requests.get.return_value = response
|
||||
requests.exceptions.RequestException = Exception
|
||||
|
||||
with (
|
||||
patch.dict("sys.modules", {"requests": requests}),
|
||||
patch(
|
||||
"vocalinux.speech_recognition.recognition_manager.get_model_path",
|
||||
return_value="/tmp/ggml-small.bin",
|
||||
),
|
||||
patch.object(SpeechRecognitionManager, "_stream_model_download"),
|
||||
patch("os.rename"),
|
||||
patch(
|
||||
"vocalinux.speech_recognition.recognition_manager.verify_model_file",
|
||||
side_effect=lambda *a, **kw: seen_when_hashing.extend(statuses),
|
||||
),
|
||||
):
|
||||
manager._download_whispercpp_model()
|
||||
|
||||
assert "Verifying model..." in seen_when_hashing
|
||||
|
||||
|
||||
def test_whispercpp_init_reports_the_load_before_running_it() -> None:
|
||||
"""Backend detection and the load itself come after "Complete!"."""
|
||||
manager = _make_manager()
|
||||
statuses = []
|
||||
manager._download_progress_callback = lambda fraction, speed, text: statuses.append(text)
|
||||
seen_when_loading = []
|
||||
|
||||
with (
|
||||
patch("os.path.exists", return_value=True),
|
||||
patch(
|
||||
"vocalinux.speech_recognition.recognition_manager."
|
||||
"_preload_pywhispercpp_shared_libraries"
|
||||
),
|
||||
patch.object(SpeechRecognitionManager, "_whispercpp_model_is_verified", return_value=True),
|
||||
patch.dict(
|
||||
"sys.modules",
|
||||
{"pywhispercpp": MagicMock(), "pywhispercpp.model": MagicMock()},
|
||||
),
|
||||
patch.object(
|
||||
SpeechRecognitionManager,
|
||||
"_load_whispercpp_model",
|
||||
side_effect=lambda *a, **kw: seen_when_loading.extend(statuses),
|
||||
),
|
||||
):
|
||||
manager._init_whispercpp()
|
||||
|
||||
assert "Loading model..." in seen_when_loading
|
||||
Reference in New Issue
Block a user