feat(models): make the recommendation applicable and stop ignoring the disk (#797)
* feat(models): make the recommendation applicable and stop ignoring the disk The panel worked out which model fits the hardware and the language, printed it as a plain label, and left the pickers on the wrong one. Applying it meant hitting two dropdowns by hand after decoding that "Small EN" means size Small plus specialization English-only. Three changes: - the recommendation gets a button that moves both pickers at once - before offering a download, look at what is already on disk. A candidate has to be no smaller than the recommendation, and an English-only recommendation is not answered with multilingual weights, which weigh the same and recognise English worse (#776). So nothing is ever quietly downgraded. - the size list shows weight and the downloaded marker, the way the specialization list already did. A size is a download decision too. Closes #778 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TKVa2qTLQYMCHffjp97yqT * fix(settings): keep recommendation fresh across language and apply Changing EN to PL left Use it pointed at the old English-only target, and clicking it briefly auto-applied the default size (full large) before the intended turbo specialization landed. Refresh the recommendation card after language changes, and move both pickers under _populating_models before the single apply. * fix(settings): type the recommendation language and apply callbacks * style(settings): wrap whispercpp model info import for isort Lint Python was failing the isort check on the single-line WHISPERCPP_MODEL_INFO import. Parenthesize it so black/isort agree. --------- Co-authored-by: Kacper Paczos <23406555+kacperpaczos@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Jatin K Malik <jatinkrmalik@gmail.com>
This commit is contained in:
co-authored by
Kacper Paczos
Claude Opus 5
Jatin K Malik
parent
40bc31bebf
commit
9a0a677271
@@ -2786,10 +2786,22 @@ class SettingsDialog(Gtk.Dialog):
|
||||
self.model_info_subtitle.get_style_context().add_class("model-info-subtitle")
|
||||
self.model_info_card.pack_start(self.model_info_subtitle, False, False, 0)
|
||||
|
||||
# The recommendation used to be a plain label, which left the panel stating
|
||||
# the right answer while the pickers kept the wrong one (#778).
|
||||
self.model_recommendation_box = Gtk.Box(orientation=Gtk.Orientation.HORIZONTAL, spacing=8)
|
||||
self.model_recommendation_box.set_no_show_all(True)
|
||||
|
||||
self.model_recommendation = Gtk.Label(xalign=0, wrap=True)
|
||||
self.model_recommendation.get_style_context().add_class("model-info-subtitle")
|
||||
self.model_recommendation.set_no_show_all(True)
|
||||
self.model_info_card.pack_start(self.model_recommendation, False, False, 0)
|
||||
self.model_recommendation_box.pack_start(self.model_recommendation, True, True, 0)
|
||||
|
||||
self.model_recommendation_button = Gtk.Button(label="Use it")
|
||||
self.model_recommendation_button.set_valign(Gtk.Align.CENTER)
|
||||
self.model_recommendation_button.set_no_show_all(True)
|
||||
self.model_recommendation_button.connect("clicked", self._on_apply_recommendation)
|
||||
self.model_recommendation_box.pack_end(self.model_recommendation_button, False, False, 0)
|
||||
|
||||
self.model_info_card.pack_start(self.model_recommendation_box, False, False, 0)
|
||||
|
||||
# Language warning (e.g. auto-detect, English-only models) lives in
|
||||
# the card so there is a single explanation surface below the group.
|
||||
@@ -4702,11 +4714,74 @@ class SettingsDialog(Gtk.Dialog):
|
||||
language_id,
|
||||
)
|
||||
|
||||
def _downloaded_alternative_for(self, recommended: str) -> Optional[str]:
|
||||
"""Return a model already on disk that can stand in for ``recommended``.
|
||||
|
||||
Every entry in these pickers is a download decision priced in gigabytes, so
|
||||
suggesting a fresh download while something equivalent is already on disk
|
||||
wastes the user's bandwidth (#778). Only candidates that are not smaller than
|
||||
the recommendation qualify, so this never quietly downgrades accuracy, and
|
||||
English-only weights are only offered when English is actually selected.
|
||||
"""
|
||||
if recommended in WHISPERCPP_MODEL_INFO and is_whispercpp_model_downloaded(recommended):
|
||||
return None
|
||||
|
||||
language_id = self.language_combo.get_active_id() or self.language
|
||||
wants_english = _language_is_english(language_id)
|
||||
recommended_mb = WHISPERCPP_MODEL_INFO.get(recommended, {}).get("size_mb", 0)
|
||||
|
||||
# An English-only recommendation must not be answered with multilingual
|
||||
# weights: they weigh the same and recognise English worse, which is the
|
||||
# accuracy loss #776 is about.
|
||||
needs_english_only = is_english_only_whispercpp_model(recommended)
|
||||
|
||||
best = None
|
||||
best_mb = None
|
||||
for model_name in list_downloaded_whispercpp_models():
|
||||
info = WHISPERCPP_MODEL_INFO.get(model_name)
|
||||
if not info:
|
||||
continue
|
||||
if is_english_only_whispercpp_model(model_name) and not wants_english:
|
||||
continue
|
||||
if needs_english_only and not is_english_only_whispercpp_model(model_name):
|
||||
continue
|
||||
if info["size_mb"] < recommended_mb:
|
||||
continue
|
||||
if best_mb is None or info["size_mb"] < best_mb:
|
||||
best, best_mb = model_name, info["size_mb"]
|
||||
|
||||
return best
|
||||
|
||||
def _get_default_whispercpp_variant_for_size(self, model_size: str) -> Optional[str]:
|
||||
"""Return the default specialization for a user-selected size."""
|
||||
language_id = self.language_combo.get_active_id() or self.language
|
||||
return _default_whispercpp_variant_for_size(model_size, language_id)
|
||||
|
||||
def _on_apply_recommendation(self, _button: Any) -> None:
|
||||
"""Set both pickers to the model the card is offering.
|
||||
|
||||
Size and specialization must land under ``_populating_models`` so
|
||||
``_on_model_changed`` does not auto-apply the default size variant
|
||||
(e.g. full ``large``) before the intended target (e.g. turbo q5_0).
|
||||
"""
|
||||
target = getattr(self, "_recommended_target_model", None)
|
||||
if not target or target not in WHISPERCPP_MODEL_INFO:
|
||||
return
|
||||
|
||||
model_size = get_whispercpp_model_size(target)
|
||||
self._populating_models = True
|
||||
try:
|
||||
self.model_combo.set_active_id(model_size)
|
||||
self._populate_whispercpp_variant_options(model_size, target)
|
||||
self.model_variant_combo.set_active_id(target)
|
||||
self._sync_language_options_for_selected_model()
|
||||
finally:
|
||||
self._populating_models = False
|
||||
|
||||
self._update_model_info()
|
||||
self._refresh_unused_downloads()
|
||||
self._auto_apply_settings()
|
||||
|
||||
def _is_selected_whispercpp_model_english_only(self) -> bool:
|
||||
"""Return whether the selected model is a whisper.cpp English-only variant."""
|
||||
if self._get_selected_engine() != "whisper_cpp":
|
||||
@@ -4895,7 +4970,14 @@ class SettingsDialog(Gtk.Dialog):
|
||||
saved_size = get_whispercpp_model_size(saved_model)
|
||||
|
||||
for model_size in ENGINE_MODELS["whisper_cpp"]:
|
||||
# Same shape as the specialization list: a size is a download decision
|
||||
# too, so its price and whether it is already paid for belong here (#778).
|
||||
size_variant = self._get_default_whispercpp_variant_for_size(model_size)
|
||||
display_text = _model_display_name(model_size)
|
||||
if size_variant in WHISPERCPP_MODEL_INFO:
|
||||
info = WHISPERCPP_MODEL_INFO[size_variant]
|
||||
status = "✓" if is_whispercpp_model_downloaded(size_variant) else "↓"
|
||||
display_text += f" ({_format_size(info['size_mb'])}) {status}"
|
||||
if model_size == recommended_size:
|
||||
display_text += " ★"
|
||||
self.model_combo.append(model_size, display_text)
|
||||
@@ -5291,8 +5373,13 @@ class SettingsDialog(Gtk.Dialog):
|
||||
self.language_warning.set_markup("")
|
||||
self.language_warning.hide()
|
||||
|
||||
def _on_language_changed(self, widget):
|
||||
"""Handle language selection change."""
|
||||
def _on_language_changed(self, widget: Any) -> None:
|
||||
"""Handle language selection change.
|
||||
|
||||
Repaint the recommendation card after the size list rebuilds so
|
||||
``_recommended_target_model`` / ★ / "Use it" track EN→non-EN flips
|
||||
instead of applying a stale English-only target.
|
||||
"""
|
||||
if self._processing_language_change:
|
||||
return
|
||||
if self._initializing or self._applying_settings:
|
||||
@@ -5311,6 +5398,7 @@ class SettingsDialog(Gtk.Dialog):
|
||||
self.language = lang_code
|
||||
self._populate_model_options()
|
||||
self._update_language_warning()
|
||||
self._update_model_info()
|
||||
self._auto_apply_settings()
|
||||
finally:
|
||||
self._processing_language_change = False
|
||||
@@ -5458,13 +5546,30 @@ class SettingsDialog(Gtk.Dialog):
|
||||
status = f"<span foreground='#e5a50a'>Download ~{_format_size(info['size_mb'])}</span>"
|
||||
self.model_info_subtitle.set_markup(f"{extra_info} · {status}")
|
||||
|
||||
if model_name == recommended:
|
||||
self.model_recommendation.hide()
|
||||
target = recommended
|
||||
message = None
|
||||
if engine == "whisper_cpp":
|
||||
already_have = self._downloaded_alternative_for(recommended)
|
||||
if already_have and already_have != model_name:
|
||||
target = already_have
|
||||
message = (
|
||||
f"You already have {_model_display_name(already_have)} on disk — "
|
||||
"using it needs no download"
|
||||
)
|
||||
|
||||
if target == model_name:
|
||||
self._recommended_target_model = None
|
||||
self.model_recommendation_box.hide()
|
||||
self.model_recommendation_button.hide()
|
||||
else:
|
||||
self.model_recommendation.set_text(
|
||||
f"Recommended: {recommended_display_name} ({reason})"
|
||||
)
|
||||
self._recommended_target_model = target
|
||||
if message is None:
|
||||
message = f"Recommended: {_model_display_name(target)} ({reason})"
|
||||
self.model_recommendation.set_text(message)
|
||||
self.model_recommendation.show()
|
||||
can_apply = engine == "whisper_cpp" and target in WHISPERCPP_MODEL_INFO
|
||||
self.model_recommendation_button.set_visible(can_apply)
|
||||
self.model_recommendation_box.show()
|
||||
|
||||
self._update_model_picker_tooltips()
|
||||
self.model_info_card.show()
|
||||
|
||||
@@ -0,0 +1,198 @@
|
||||
"""The recommendation must be applicable, and must not ignore the disk (#778)."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import importlib
|
||||
import sys
|
||||
from collections.abc import Iterator
|
||||
from typing import Any
|
||||
from unittest.mock import MagicMock, Mock, call, patch
|
||||
|
||||
import pytest
|
||||
|
||||
import vocalinux.ui
|
||||
|
||||
|
||||
@pytest.fixture(scope="module")
|
||||
def settings_dialog() -> Iterator[Any]:
|
||||
"""Import settings_dialog with real base classes for its GTK subclasses.
|
||||
|
||||
Same reasoning as tests/test_settings_model_state.py; both sys.modules and the
|
||||
package attribute are restored so later tests patching the module by name do
|
||||
not reach a second module object (#686).
|
||||
"""
|
||||
repository = sys.modules["gi.repository"]
|
||||
bases = {name: type(name, (), {}) for name in ("Box", "ListBoxRow", "Dialog")}
|
||||
saved_module = sys.modules.pop("vocalinux.ui.settings_dialog", None)
|
||||
saved_attribute = getattr(vocalinux.ui, "settings_dialog", None)
|
||||
try:
|
||||
with patch.object(repository, "Gtk", MagicMock(**bases)):
|
||||
module = importlib.import_module("vocalinux.ui.settings_dialog")
|
||||
yield module
|
||||
finally:
|
||||
sys.modules.pop("vocalinux.ui.settings_dialog", None)
|
||||
if saved_module is not None:
|
||||
sys.modules["vocalinux.ui.settings_dialog"] = saved_module
|
||||
if saved_attribute is not None:
|
||||
vocalinux.ui.settings_dialog = saved_attribute
|
||||
elif hasattr(vocalinux.ui, "settings_dialog"):
|
||||
del vocalinux.ui.settings_dialog
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def dialog_class(settings_dialog: Any) -> Any:
|
||||
return settings_dialog.SettingsDialog
|
||||
|
||||
|
||||
def _dialog_stub(language: str = "en-us") -> Mock:
|
||||
dialog = Mock()
|
||||
dialog.language = language
|
||||
dialog.language_combo.get_active_id.return_value = language
|
||||
dialog._populating_models = False
|
||||
dialog._processing_language_change = False
|
||||
return dialog
|
||||
|
||||
|
||||
def _with_disk(settings_dialog: Any, downloaded: list[str]) -> tuple[Any, Any]:
|
||||
"""Patch the disk so only ``downloaded`` counts as present."""
|
||||
return (
|
||||
patch.object(settings_dialog, "list_downloaded_whispercpp_models", return_value=downloaded),
|
||||
patch.object(
|
||||
settings_dialog,
|
||||
"is_whispercpp_model_downloaded",
|
||||
side_effect=lambda name: name in downloaded,
|
||||
),
|
||||
)
|
||||
|
||||
|
||||
def test_a_bigger_model_on_disk_is_offered_instead_of_a_download(
|
||||
settings_dialog: Any, dialog_class: Any
|
||||
) -> None:
|
||||
"""The reported case: recommended small.en, medium.en already paid for."""
|
||||
dialog = _dialog_stub()
|
||||
listed, downloaded = _with_disk(settings_dialog, ["medium.en", "small", "tiny"])
|
||||
|
||||
with listed, downloaded:
|
||||
alternative = dialog_class._downloaded_alternative_for(dialog, "small.en")
|
||||
|
||||
assert alternative == "medium.en"
|
||||
|
||||
|
||||
def test_nothing_is_offered_when_the_recommendation_is_already_on_disk(
|
||||
settings_dialog: Any, dialog_class: Any
|
||||
) -> None:
|
||||
dialog = _dialog_stub()
|
||||
listed, downloaded = _with_disk(settings_dialog, ["small.en", "medium.en"])
|
||||
|
||||
with listed, downloaded:
|
||||
assert dialog_class._downloaded_alternative_for(dialog, "small.en") is None
|
||||
|
||||
|
||||
def test_a_smaller_model_is_never_offered(settings_dialog: Any, dialog_class: Any) -> None:
|
||||
"""Reusing a download must not quietly cost accuracy."""
|
||||
dialog = _dialog_stub()
|
||||
listed, downloaded = _with_disk(settings_dialog, ["tiny", "tiny.en"])
|
||||
|
||||
with listed, downloaded:
|
||||
assert dialog_class._downloaded_alternative_for(dialog, "medium.en") is None
|
||||
|
||||
|
||||
def test_english_only_weights_are_not_offered_for_another_language(
|
||||
settings_dialog: Any, dialog_class: Any
|
||||
) -> None:
|
||||
dialog = _dialog_stub(language="pl")
|
||||
listed, downloaded = _with_disk(settings_dialog, ["medium.en"])
|
||||
|
||||
with listed, downloaded:
|
||||
assert dialog_class._downloaded_alternative_for(dialog, "small") is None
|
||||
|
||||
|
||||
def test_the_smallest_qualifying_model_wins(settings_dialog: Any, dialog_class: Any) -> None:
|
||||
"""Between two usable downloads, take the cheaper one to run."""
|
||||
dialog = _dialog_stub()
|
||||
listed, downloaded = _with_disk(settings_dialog, ["large", "medium.en"])
|
||||
|
||||
with listed, downloaded:
|
||||
assert dialog_class._downloaded_alternative_for(dialog, "small.en") == "medium.en"
|
||||
|
||||
|
||||
def test_applying_the_recommendation_sets_both_pickers(
|
||||
settings_dialog: Any, dialog_class: Any
|
||||
) -> None:
|
||||
"""Clicking must move size and specialization together, not just one."""
|
||||
dialog = _dialog_stub()
|
||||
dialog._recommended_target_model = "small.en"
|
||||
|
||||
dialog_class._on_apply_recommendation(dialog, None)
|
||||
|
||||
dialog.model_combo.set_active_id.assert_called_once_with("small")
|
||||
dialog._populate_whispercpp_variant_options.assert_called_once_with("small", "small.en")
|
||||
dialog.model_variant_combo.set_active_id.assert_called_once_with("small.en")
|
||||
dialog._sync_language_options_for_selected_model.assert_called_once_with()
|
||||
dialog._update_model_info.assert_called_once_with()
|
||||
dialog._refresh_unused_downloads.assert_called_once_with()
|
||||
dialog._auto_apply_settings.assert_called_once_with()
|
||||
assert dialog._populating_models is False
|
||||
|
||||
|
||||
def test_applying_does_nothing_without_a_target(settings_dialog: Any, dialog_class: Any) -> None:
|
||||
dialog = _dialog_stub()
|
||||
dialog._recommended_target_model = None
|
||||
|
||||
dialog_class._on_apply_recommendation(dialog, None)
|
||||
|
||||
dialog.model_combo.set_active_id.assert_not_called()
|
||||
dialog._auto_apply_settings.assert_not_called()
|
||||
|
||||
|
||||
def test_applying_recommendation_suppresses_model_changed_auto_apply(
|
||||
settings_dialog: Any, dialog_class: Any
|
||||
) -> None:
|
||||
"""Size change must not briefly auto-apply the default size before the target.
|
||||
|
||||
Without ``_populating_models``, ``set_active_id(large)`` would fire
|
||||
``_on_model_changed`` and download full large before turbo-q5_0 lands.
|
||||
"""
|
||||
dialog = _dialog_stub()
|
||||
dialog._recommended_target_model = "large-v3-turbo-q5_0"
|
||||
seen_while_setting_size: list[bool] = []
|
||||
|
||||
def capture_flag(_model_size: str) -> bool:
|
||||
seen_while_setting_size.append(dialog._populating_models)
|
||||
return True
|
||||
|
||||
dialog.model_combo.set_active_id.side_effect = capture_flag
|
||||
|
||||
dialog_class._on_apply_recommendation(dialog, None)
|
||||
|
||||
assert seen_while_setting_size == [True]
|
||||
dialog._populate_whispercpp_variant_options.assert_called_once_with(
|
||||
"large", "large-v3-turbo-q5_0"
|
||||
)
|
||||
dialog.model_variant_combo.set_active_id.assert_called_once_with("large-v3-turbo-q5_0")
|
||||
# One apply after both pickers settle — not during the size change.
|
||||
dialog._auto_apply_settings.assert_called_once_with()
|
||||
assert dialog._populating_models is False
|
||||
|
||||
|
||||
def test_language_change_refreshes_recommendation_ui(
|
||||
settings_dialog: Any, dialog_class: Any
|
||||
) -> None:
|
||||
"""EN→PL must refresh recommendation target / label so Use it is not stale."""
|
||||
dialog = _dialog_stub(language="en-us")
|
||||
dialog.language_combo.get_active_id.return_value = "pl"
|
||||
dialog.engine_combo.get_active_text.return_value = "whisper.cpp"
|
||||
dialog._recommended_target_model = "small.en"
|
||||
|
||||
dialog_class._on_language_changed(dialog, None)
|
||||
|
||||
assert dialog.language == "pl"
|
||||
dialog._populate_model_options.assert_called_once_with()
|
||||
dialog._update_language_warning.assert_called_once_with()
|
||||
dialog._update_model_info.assert_called_once_with()
|
||||
dialog._auto_apply_settings.assert_called_once_with()
|
||||
# Refresh happens before auto-apply so the card is current when settings land.
|
||||
assert dialog.mock_calls.index(call._update_model_info()) < dialog.mock_calls.index(
|
||||
call._auto_apply_settings()
|
||||
)
|
||||
assert dialog._processing_language_change is False
|
||||
Reference in New Issue
Block a user