fix(startup): resolve model size per engine instead of the legacy key (#684)
* 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.
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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")
|
||||
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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."""
|
||||
|
||||
Reference in New Issue
Block a user