From 5e3265f87b4b5fb4dcfafeb95fd82af1028db7f3 Mon Sep 17 00:00:00 2001 From: Kacper Paczos Date: Thu, 27 Aug 2026 08:58:42 +0200 Subject: [PATCH] fix(startup): resolve model size per engine instead of the legacy key (#684) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(startup): resolve model size per engine instead of the legacy key Startup read the generic "model_size" key, which always holds whichever engine was saved last, so it initialized the configured engine with another engine's model. With engine=whisper_cpp and a config left at model_size=medium by a VOSK save, startup looked for ggml-medium.bin, reported the model as missing and offered a fresh download while the working ggml-small.bin named by whisper_cpp_model_size sat on disk. Use ConfigManager.get_model_size_for_engine(), the resolver the settings dialog already uses, so the generic key stays a write-only compatibility shim. It falls back to the generic key when no per-engine key exists, so older configs keep working. * fix(config): migrate model_size into whisper_cpp_model_size Review follow-up on #684. get_model_size_for_engine() only falls back to the generic model_size key when the engine key is missing from the in-memory dict, and DEFAULT_CONFIG already ships whisper_cpp_model_size. After the merge in load_config() the key is therefore always present, so a user file holding only {engine: whisper_cpp, model_size: small} used to start on small and would now start on the default tiny — the older-config path from #681. _migrate_config already copies the model into vosk_model_size and whisper_model_size; do the same for whisper_cpp so the resolver change is behaviour-preserving for those files, and widen the migration trigger to notice a missing whisper_cpp key. Tests run against a real ConfigManager on a temp config, not a mocked resolver: the migration case fails without this change, and the two resolver cases pin the engine key winning over the generic one. --- src/vocalinux/main.py | 7 +++- src/vocalinux/ui/config_manager.py | 16 +++++++- tests/test_config_manager.py | 53 +++++++++++++++++++++++++++ tests/test_main.py | 59 ++++++++++++++++++++++++++++++ 4 files changed, 132 insertions(+), 3 deletions(-) diff --git a/src/vocalinux/main.py b/src/vocalinux/main.py index 00ef03b..b8c093f 100644 --- a/src/vocalinux/main.py +++ b/src/vocalinux/main.py @@ -397,8 +397,11 @@ def main(): model_size = args.model logger.info(f"Using model={model_size} (from command line)") else: - model_size = saved_settings.get("model_size", args.model) - logger.info(f"Using model={model_size} (from saved config)") + # Resolve per engine: the generic "model_size" key always holds the + # model of whichever engine was saved last, so reading it directly + # loads the wrong model whenever the two disagree. + model_size = config_manager.get_model_size_for_engine(engine) + logger.info(f"Using model={model_size} (saved for engine {engine})") vad_sensitivity = saved_settings.get("vad_sensitivity", 3) silence_timeout = saved_settings.get("silence_timeout", 2.0) diff --git a/src/vocalinux/ui/config_manager.py b/src/vocalinux/ui/config_manager.py index add4728..333524c 100644 --- a/src/vocalinux/ui/config_manager.py +++ b/src/vocalinux/ui/config_manager.py @@ -189,7 +189,9 @@ class ConfigManager: sr_config = user_config.get("speech_recognition", {}) # Need migration if we have model_size but not the per-engine keys return "model_size" in sr_config and ( - "vosk_model_size" not in sr_config or "whisper_model_size" not in sr_config + "vosk_model_size" not in sr_config + or "whisper_model_size" not in sr_config + or "whisper_cpp_model_size" not in sr_config ) def _migrate_config(self, user_config: dict): @@ -214,6 +216,18 @@ class ConfigManager: ) logger.info(f"Migrated whisper_model_size to: {sr_config['whisper_model_size']}") + if "whisper_cpp_model_size" not in user_sr_config: + # DEFAULT_CONFIG already carries whisper_cpp_model_size, so the + # generic fallback in get_model_size_for_engine() never fires for + # this engine. Without this copy a config that only stores + # model_size would silently start on the default instead. + sr_config["whisper_cpp_model_size"] = ( + current_model if current_engine == "whisper_cpp" else "tiny" + ) + logger.info( + f"Migrated whisper_cpp_model_size to: {sr_config['whisper_cpp_model_size']}" + ) + self.save_config() logger.info("Config migrated to new per-engine model format") diff --git a/tests/test_config_manager.py b/tests/test_config_manager.py index b9f639a..d88164c 100644 --- a/tests/test_config_manager.py +++ b/tests/test_config_manager.py @@ -281,6 +281,59 @@ class TestConfigManager(unittest.TestCase): self.assertEqual(config_manager.config["speech_recognition"]["vosk_model_size"], "medium") self.assertEqual(config_manager.config["speech_recognition"]["whisper_model_size"], "tiny") + def test_migrate_copies_model_size_to_whisper_cpp(self): + """A whisper.cpp config that predates the per-engine keys keeps its model. + + DEFAULT_CONFIG already ships whisper_cpp_model_size, so the generic + fallback inside get_model_size_for_engine() can never fire for this + engine: after the merge the key is always present. Only the migration + can carry the user's own model_size across (#681). + """ + with open(self.temp_config_file, "w") as f: + json.dump({"speech_recognition": {"engine": "whisper_cpp", "model_size": "small"}}, f) + + config_manager = ConfigManager() + + self.assertEqual( + config_manager.config["speech_recognition"]["whisper_cpp_model_size"], "small" + ) + self.assertEqual(config_manager.get_model_size_for_engine("whisper_cpp"), "small") + + def test_migrate_leaves_other_engines_on_their_defaults(self): + """The migration must not hand one engine's model to the others.""" + with open(self.temp_config_file, "w") as f: + json.dump({"speech_recognition": {"engine": "vosk", "model_size": "large"}}, f) + + config_manager = ConfigManager() + + self.assertEqual(config_manager.get_model_size_for_engine("vosk"), "large") + self.assertEqual(config_manager.get_model_size_for_engine("whisper_cpp"), "tiny") + self.assertEqual(config_manager.get_model_size_for_engine("whisper"), "tiny") + + def test_resolver_prefers_the_engine_key_over_the_generic_one(self): + """The generic key holds whichever engine saved last; it must not win. + + This is the startup bug from #681: model_size still says "medium" from + the last VOSK save while the configured engine is whisper.cpp. + """ + with open(self.temp_config_file, "w") as f: + json.dump( + { + "speech_recognition": { + "engine": "whisper_cpp", + "model_size": "medium", + "vosk_model_size": "medium", + "whisper_cpp_model_size": "small", + } + }, + f, + ) + + config_manager = ConfigManager() + + self.assertEqual(config_manager.get_model_size_for_engine("whisper_cpp"), "small") + self.assertEqual(config_manager.get_model_size_for_engine("vosk"), "medium") + def test_update_speech_recognition_settings_per_engine(self): """Test that update_speech_recognition_settings saves per-engine model sizes.""" config_manager = ConfigManager() diff --git a/tests/test_main.py b/tests/test_main.py index ee66d19..9cf9d0e 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -185,6 +185,7 @@ class TestMainModule(unittest.TestCase): "speech_recognition": {}, "general": {"first_run": False}, } + mock_config_instance.get_model_size_for_engine.return_value = "medium" mock_config_manager.return_value = mock_config_instance # Mock objects @@ -916,6 +917,7 @@ class TestMainConfigPrecedence(unittest.TestCase): }, "general": {"first_run": False}, } + mock_config_instance.get_model_size_for_engine.return_value = "medium" mock_config_manager.return_value = mock_config_instance mock_speech_instance = MagicMock() @@ -941,6 +943,63 @@ class TestMainConfigPrecedence(unittest.TestCase): self.assertEqual(call_kwargs["language"], "de") self.assertEqual(call_kwargs["audio_device_index"], 2) + @patch("vocalinux.main.check_dependencies") + @patch("vocalinux.ui.action_handler.ActionHandler") + @patch("vocalinux.speech_recognition.recognition_manager.SpeechRecognitionManager") + @patch("vocalinux.text_injection.text_injector.TextInjector") + @patch("vocalinux.ui.tray_indicator.TrayIndicator") + @patch("vocalinux.ui.config_manager.ConfigManager") + @patch("vocalinux.ui.logging_manager.initialize_logging") + def test_model_resolved_per_engine_not_from_generic_key( + self, + mock_init_logging, + mock_config_manager, + mock_tray, + mock_text, + mock_speech, + mock_action_handler, + mock_check_deps, + ): + """Startup must use the engine's own model, not the stale generic key. + + The generic "model_size" key holds whichever engine was saved last, so + a config carrying another engine's model must not decide what the + configured engine loads. + """ + mock_check_deps.return_value = True + + mock_config_instance = MagicMock() + mock_config_instance.get_settings.return_value = { + "speech_recognition": { + "engine": "whisper_cpp", + # Left behind by the last VOSK save. + "model_size": "medium", + "vosk_model_size": "medium", + "whisper_cpp_model_size": "small", + "language": "en-us", + }, + "general": {"first_run": False}, + } + mock_config_instance.get_model_size_for_engine.side_effect = lambda engine: { + "vosk": "medium", + "whisper_cpp": "small", + }[engine] + mock_config_manager.return_value = mock_config_instance + + mock_speech.return_value = MagicMock() + mock_text.return_value = MagicMock() + mock_tray.return_value = MagicMock() + mock_action_handler.return_value = MagicMock() + + with patch("sys.argv", ["vocalinux"]): + with patch("vocalinux.main.logger"): + main() + + mock_config_instance.get_model_size_for_engine.assert_called_with("whisper_cpp") + call_kwargs = mock_speech.call_args[1] + self.assertEqual(call_kwargs["engine"], "whisper_cpp") + self.assertEqual(call_kwargs["model_size"], "small") + class TestTextCallbackSpacing(unittest.TestCase): """Test spacing logic in text_callback_wrapper."""