From 4d17ba586a59195a02182eaf08bf6459c6d4bfd6 Mon Sep 17 00:00:00 2001 From: JarbasAi Date: Tue, 8 Sep 2026 12:15:15 +0100 Subject: [PATCH] fix: key resource caches by resource directory --- ovos_workshop/skills/auto_translatable.py | 11 ++-- ovos_workshop/skills/ovos.py | 41 ++++++------- .../skills/test_auto_translatable.py | 20 +++++++ test/unittests/skills/test_base.py | 57 +++++++++++++------ test/unittests/test_abstract_app.py | 16 +++++- .../test_auto_register_entity_files.py | 53 ++++++++++++++++- 6 files changed, 155 insertions(+), 43 deletions(-) diff --git a/ovos_workshop/skills/auto_translatable.py b/ovos_workshop/skills/auto_translatable.py index 78a8237e..45523a5e 100644 --- a/ovos_workshop/skills/auto_translatable.py +++ b/ovos_workshop/skills/auto_translatable.py @@ -68,12 +68,13 @@ def _load_lang(self, root_directory=None, lang=None): """ lang = lang or self.internal_language # self.lang in base class root_directory = root_directory or self.res_dir - if lang not in self._lang_resources: - self._lang_resources[lang] = SkillResources(root_directory, lang, - skill_id=self.skill_id) + key = (root_directory, lang) + if key not in self._lang_resources: + self._lang_resources[key] = SkillResources(root_directory, lang, + skill_id=self.skill_id) # see OVOSSkill.load_lang - same auto entity-file discovery - self._auto_register_entity_files(lang) - return self._lang_resources[lang] + self._auto_register_entity_files(lang, self._lang_resources[key]) + return self._lang_resources[key] def detect_language(self, utterance: str): """ diff --git a/ovos_workshop/skills/ovos.py b/ovos_workshop/skills/ovos.py index 1aff289f..bca771a7 100644 --- a/ovos_workshop/skills/ovos.py +++ b/ovos_workshop/skills/ovos.py @@ -187,11 +187,11 @@ def __init__(self, name: Optional[str] = None, # loaded lang file resources self._lang_resources = {} - # OVOS-INTENT-3 §auto-entity: tracks which languages already had - # their locale .entity files auto-discovered and registered, so a - # repeated load_lang() for the same lang (reload/retrain) never - # double-emits the same entity registrations. - self._auto_registered_entity_langs = set() + # OVOS-INTENT-3 §auto-entity: tracks which (resource directory, + # lang) pairs already had their locale .entity files auto-discovered + # and registered, so a repeated load_lang() for the same pair + # (reload/retrain) never double-emits the same entity registrations. + self._auto_registered_entity_dirs = set() # OVOS-INTENT-3 §auto-entity: tracks (lang -> {entity_file, ...}) # already sent to the intent service, keyed by the bare entity file # name (no extension). Shared by auto-discovery AND the explicit @@ -611,9 +611,10 @@ def load_lang(self, root_directory: Optional[str] = None, """ lang = standardize_lang(lang or self.lang) root_directory = root_directory or self.res_dir - if lang not in self._lang_resources: - self._lang_resources[lang] = SkillResources(root_directory, lang, - skill_id=self.skill_id) + key = (root_directory, lang) + if key not in self._lang_resources: + self._lang_resources[key] = SkillResources(root_directory, lang, + skill_id=self.skill_id) # OVOS-INTENT-3 §auto-entity: register every shipped .entity file # for this lang the first time its resources are loaded, so it # reaches the matcher in the same batch as (and strictly before) @@ -621,8 +622,8 @@ def load_lang(self, root_directory: Optional[str] = None, # calls load_lang() before building/emitting its own template, # so hooking the cache-miss path here guarantees ordering without # requiring skill authors to call register_entity_file() at all. - self._auto_register_entity_files(lang) - return self._lang_resources[lang] + self._auto_register_entity_files(lang, self._lang_resources[key]) + return self._lang_resources[key] def load_dialog_files(self, root_directory: Optional[str] = None): """ @@ -1633,7 +1634,8 @@ def _register_entity_file_for_lang(self, entity_file: str, lang: str, self.intent_service.register_entity(name, samples, lang, blacklisted_words=blacklist) - def _auto_register_entity_files(self, lang: str): + def _auto_register_entity_files(self, lang: str, + resources: Optional[SkillResources] = None): """ Auto-discover and register every ".entity" file shipped in this skill's locale resources for `lang`. @@ -1656,23 +1658,24 @@ def _auto_register_entity_files(self, lang: str): Idempotency: guarded twice - `load_lang` only calls this on a cache-miss (one call per lang per skill instance in normal use), - and `_auto_registered_entity_langs` guards direct/repeated calls - (e.g. tests, or a future reload path) from double-emitting. + and `_auto_registered_entity_dirs` guards direct/repeated calls + (e.g. tests, or a future reload path) from double-emitting. The + guard is per (resource directory, lang): a skill pointed at a new + `res_dir` registers that directory's entity files. Can be disabled entirely via the "skills" section of mycroft.conf: {"skills": {"auto_register_entity_files": false}} """ - if lang in self._auto_registered_entity_langs: + resources = resources or self.load_lang(lang=lang) + key = (resources.skill_directory, lang) + if key in self._auto_registered_entity_dirs: return - self._auto_registered_entity_langs.add(lang) + self._auto_registered_entity_dirs.add(key) if not self.config_core.get("skills", {}).get( "auto_register_entity_files", True): return - resources = self._lang_resources.get(lang) - if resources is None: - return entity_dir = resources.types.entity.base_directory if not entity_dir or not Path(entity_dir).is_dir(): return @@ -2526,7 +2529,7 @@ def voc_list(self, voc_filename: str, @return: list of string vocab options """ lang = standardize_lang(lang or self.lang) - cache_key = lang + voc_filename + cache_key = (self.res_dir, lang, voc_filename) if cache_key not in self._voc_cache: vocab = self.resources.load_vocabulary_file(voc_filename) diff --git a/test/unittests/skills/test_auto_translatable.py b/test/unittests/skills/test_auto_translatable.py index 02d622ec..4934368e 100644 --- a/test/unittests/skills/test_auto_translatable.py +++ b/test/unittests/skills/test_auto_translatable.py @@ -1,4 +1,8 @@ +import os +import shutil +import tempfile import unittest +from os.path import join from ovos_workshop.skills.fallback import FallbackSkill from ovos_workshop.skills.ovos import OVOSSkill @@ -12,6 +16,22 @@ def test_00_init(self): self.assertIsInstance(self.test_skill, self.UniversalSkill) self.assertIsInstance(self.test_skill, OVOSSkill) + def test_load_lang_honours_root_directory(self): + skill = self.UniversalSkill() + lang = skill.internal_language + skill._load_lang(lang=lang) + + other = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, other) + voc_dir = join(other, "locale", lang) + os.makedirs(voc_dir) + with open(join(voc_dir, "condition.voc"), "w") as f: + f.write("sunny\n") + + resources = skill._load_lang(root_directory=other, lang=lang) + self.assertEqual(resources.load_vocabulary_file("condition"), + [["sunny"]]) + # TODO: Test other class methods diff --git a/test/unittests/skills/test_base.py b/test/unittests/skills/test_base.py index 787aa16d..31c18ff3 100644 --- a/test/unittests/skills/test_base.py +++ b/test/unittests/skills/test_base.py @@ -14,6 +14,7 @@ import json import os import shutil +import tempfile import unittest from logging import Logger @@ -280,7 +281,6 @@ def test_voc_list(self): def test_voc_match(self): skill = OVOSSkill(bus=self.bus, skill_id=self.skill_id) - skill._lang_resources = dict() skill.res_dir = join(dirname(__file__), "test_locale") lang = "en-US" @@ -289,9 +289,48 @@ def test_voc_match(self): self.assertFalse(skill.voc_match("it is nice outside", "condition", lang=lang)) + def test_voc_match_after_res_dir_reassigned(self): + skill = OVOSSkill(bus=self.bus, skill_id=self.skill_id) + skill.res_dir = join(dirname(__file__), "test_locale") + lang = "en-US" + self.assertTrue(skill.voc_match("it is hot outside", "condition", + lang=lang)) + + other_dir = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, other_dir) + voc_dir = join(other_dir, "locale", lang) + os.makedirs(voc_dir) + with open(join(voc_dir, "condition.voc"), "w") as f: + f.write("sunny\n") + skill.res_dir = other_dir + + self.assertTrue(skill.voc_match("it is sunny outside", "condition", + lang=lang)) + self.assertFalse(skill.voc_match("it is hot outside", "condition", + lang=lang)) + + def test_load_lang_honours_root_directory(self): + skill = OVOSSkill(bus=self.bus, skill_id=self.skill_id) + skill.res_dir = join(dirname(__file__), "test_locale") + lang = "en-US" + self.assertEqual( + skill.load_lang(lang=lang).load_vocabulary_file("condition"), + [["hot"], ["cold"], ["freezing"]]) + + other_dir = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, other_dir) + voc_dir = join(other_dir, "locale", lang) + os.makedirs(voc_dir) + with open(join(voc_dir, "condition.voc"), "w") as f: + f.write("sunny\n") + + self.assertEqual( + skill.load_lang(root_directory=other_dir, + lang=lang).load_vocabulary_file("condition"), + [["sunny"]]) + def test_voc_match_span(self): skill = OVOSSkill(bus=self.bus, skill_id=self.skill_id) - skill._lang_resources = dict() skill.res_dir = join(dirname(__file__), "test_locale") lang = "en-US" @@ -382,7 +421,6 @@ def handler(self, message): skill_cls = type("_IntentFileContextGateSkill", (OVOSSkill,), {"handle_time_intent": handler}) skill = skill_cls(bus=self.bus, skill_id=self.skill_id) - skill._lang_resources = dict() skill.intent_service = Mock() skill.res_dir = join(dirname(__file__), "test_locale") skill.config_core["lang"] = "en-US" @@ -516,7 +554,6 @@ def test_register_intent_adapt_context_gating_reaches_intent_service(self): from ovos_spec_tools import IntentBuilder skill = OVOSSkill(bus=self.bus, skill_id=self.skill_id) - skill._lang_resources = dict() skill.intent_service = Mock() skill.intent_service.intent_names = [] skill.intent_service.intent_is_detached.return_value = False @@ -532,7 +569,6 @@ def test_register_intent_adapt_context_gating_reaches_intent_service(self): def test_register_intent_file(self): skill = OVOSSkill(bus=self.bus, skill_id=self.skill_id) - skill._lang_resources = dict() skill.intent_service = Mock() skill.res_dir = join(dirname(__file__), "test_locale") en_intent_file = join(skill.res_dir, "locale", "en-US", "time.intent") @@ -566,7 +602,6 @@ def test_register_intent_file(self): def test_register_intent_file_with_context_gating(self): skill = OVOSSkill(bus=self.bus, skill_id=self.skill_id) - skill._lang_resources = dict() skill.intent_service = Mock() skill.res_dir = join(dirname(__file__), "test_locale") en_samples = ["what time is it"] @@ -589,7 +624,6 @@ def test_register_intent_file_binds_the_canonical_event_only(self): # workshop must neither register nor listen on the suffixed twin — # that compat belongs to ovos-spec-tools at the bus layer. skill = OVOSSkill(bus=FakeBus(), skill_id=self.skill_id) - skill._lang_resources = dict() skill.intent_service = Mock() skill.res_dir = join(dirname(__file__), "test_locale") skill.config_core["lang"] = "en-US" @@ -608,7 +642,6 @@ def test_register_intent_file_registers_the_canonical_name(self): # the name that goes to the intent service (and so onto the wire in # the INTENT-4 registration payload) carries no authoring extension skill = OVOSSkill(bus=FakeBus(), skill_id=self.skill_id) - skill._lang_resources = dict() skill.intent_service = Mock() skill.res_dir = join(dirname(__file__), "test_locale") skill.config_core["lang"] = "en-US" @@ -628,7 +661,6 @@ def test_no_suffixed_topic_originates_from_workshop(self): bus.on("message", lambda m: emitted.append( json.loads(m)["type"] if isinstance(m, str) else m.msg_type)) skill = OVOSSkill(bus=bus, skill_id=self.skill_id) - skill._lang_resources = dict() skill.res_dir = join(dirname(__file__), "test_locale") skill.config_core["lang"] = "en-US" skill.config_core["secondary_langs"] = [] @@ -646,7 +678,6 @@ def test_register_intent_file_canonical_topic_fires_handler(self): # the registered handler, matching how a pipeline dispatches bus = FakeBus() skill = OVOSSkill(bus=bus, skill_id=self.skill_id) - skill._lang_resources = dict() skill.intent_service = Mock() skill.res_dir = join(dirname(__file__), "test_locale") skill.config_core["lang"] = "en-US" @@ -710,7 +741,6 @@ def test_dual_registration_does_not_double_fire(self): def test_disable_intent_removes_the_canonical_event(self): # the author still names the intent by its authoring file skill = OVOSSkill(bus=FakeBus(), skill_id=self.skill_id) - skill._lang_resources = dict() skill.intent_service = Mock() skill.intent_service.__contains__ = Mock(return_value=True) skill.res_dir = join(dirname(__file__), "test_locale") @@ -729,7 +759,6 @@ def test_disable_intent_removes_the_canonical_event(self): def test_register_entity_file(self): skill = OVOSSkill(bus=self.bus, skill_id=self.skill_id) - skill._lang_resources = dict() skill.intent_service = Mock() skill.res_dir = join(dirname(__file__), "test_locale") en_file = join(skill.res_dir, "locale", "en-US", "dow.entity") @@ -775,7 +804,6 @@ def test_disable_intent(self): bus = FakeBus() skill = OVOSSkill(bus=bus, skill_id=self.skill_id) - skill._lang_resources = dict() skill.res_dir = join(dirname(__file__), "test_locale") skill.config_core["lang"] = "en-US" skill.config_core["secondary_langs"] = [] @@ -823,7 +851,6 @@ def test_disable_enable_intent_canonical_spelling_round_trip(self): bus = FakeBus() skill = OVOSSkill(bus=bus, skill_id=self.skill_id) - skill._lang_resources = dict() skill.res_dir = join(dirname(__file__), "test_locale") skill.config_core["lang"] = "en-US" skill.config_core["secondary_langs"] = [] @@ -906,7 +933,6 @@ def test_handle_disable_intent(self): bus = FakeBus() skill = OVOSSkill(bus=bus, skill_id=self.skill_id) - skill._lang_resources = dict() skill.res_dir = join(dirname(__file__), "test_locale") skill.config_core["lang"] = "en-US" skill.config_core["secondary_langs"] = [] @@ -928,7 +954,6 @@ def test_handle_enable_intent(self): bus = FakeBus() skill = OVOSSkill(bus=bus, skill_id=self.skill_id) - skill._lang_resources = dict() skill.res_dir = join(dirname(__file__), "test_locale") skill.config_core["lang"] = "en-US" skill.config_core["secondary_langs"] = [] diff --git a/test/unittests/test_abstract_app.py b/test/unittests/test_abstract_app.py index 885b4a93..352cb494 100644 --- a/test/unittests/test_abstract_app.py +++ b/test/unittests/test_abstract_app.py @@ -61,7 +61,13 @@ def test_settings_path(self): remove(test_app.settings_path) remove(test_skill.settings_path) - @patch("ovos_workshop.app.OVOSSkill.default_shutdown") + # autospec records the instance, so this asserts WHICH skill was shut + # down. The patch replaces the method on OVOSSkill itself, and skills + # built by earlier tests stay alive on the shared FakeBus until the + # generational collector takes them, which can happen at any point + # inside this test; counting calls therefore counts other skills' + # finalisers too. + @patch("ovos_workshop.app.OVOSSkill.default_shutdown", autospec=True) def test_default_shutdown(self, skill_shutdown): real_clear_intents = self.app.clear_intents real_bus_close = self.app.bus.close @@ -70,7 +76,13 @@ def test_default_shutdown(self, skill_shutdown): self.app.default_shutdown() self.app.clear_intents.assert_called_once() self.app.bus.close.assert_not_called() # No dedicated bus here - skill_shutdown.assert_called_once() + # count only OUR app's shutdowns: identity keeps other skills' + # finalisers out, the count keeps a double shutdown in + ours = [c for c in skill_shutdown.call_args_list + if c.args[0] is self.app] + self.assertEqual(len(ours), 1, + f"expected exactly one OVOSSkill.default_shutdown " + f"for this app, got {len(ours)}") self.app.bus.close = real_bus_close self.app.clear_intents = real_clear_intents diff --git a/test/unittests/test_auto_register_entity_files.py b/test/unittests/test_auto_register_entity_files.py index 264bea0b..743e185d 100644 --- a/test/unittests/test_auto_register_entity_files.py +++ b/test/unittests/test_auto_register_entity_files.py @@ -29,8 +29,11 @@ digit wildcard) rather than silently doing nothing """ import json +import os +import shutil +import tempfile import unittest -from os.path import dirname +from os.path import dirname, join from ovos_workshop.skills.ovos import OVOSSkill from ovos_utils.fakebus import FakeBus @@ -100,6 +103,54 @@ def test_nested_entity_file_is_registered(self): self.assertEqual(set(data["samples"]), {"cat", "dog"}) +class TestAutoEntityResourceDirectory(unittest.TestCase): + """Discovery follows the resource directory the resources were built + from, not just the language: a skill pointed at a second directory + registers that directory's entity files.""" + + def setUp(self): + self.bus = _make_bus() + self.skill = _make_skill(self.bus) + + def _entity_regs(self): + return [m["data"] for m in self.bus.emitted_msgs + if m["type"] == "ovos.entity.register"] + + def _other_res_dir(self, samples=("go", "shogi")): + other = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, other) + locale = join(other, "locale", "en-US") + os.makedirs(locale) + with open(join(locale, "boardgame.entity"), "w") as f: + f.write("\n".join(samples) + "\n") + return other + + def test_entity_files_registered_after_res_dir_reassigned(self): + self.skill.load_lang(RES_DIR, "en-US") + self.skill.res_dir = self._other_res_dir() + self.skill.load_lang(lang="en-US") + + data = next((d for d in self._entity_regs() + if d["entity_name"] == "boardgame"), None) + self.assertIsNotNone( + data, + f"entity files of the new res_dir were not registered, saw: " + f"{[d['entity_name'] for d in self._entity_regs()]}") + self.assertEqual(set(data["samples"]), {"go", "shogi"}) + + def test_discovery_scans_the_resources_it_is_given(self): + """The `resources` argument selects what is scanned; it is not a + hint that a lookup by language may override.""" + from ovos_workshop.resource_files import SkillResources + other = self._other_res_dir() + resources = SkillResources(other, "en-US", + skill_id=self.skill.skill_id) + self.skill._auto_register_entity_files("en-US", resources) + + names = {d["entity_name"] for d in self._entity_regs()} + self.assertIn("boardgame", names) + + class TestAutoEntityOrdering(unittest.TestCase): """Entities must reach the bus before/with the intent template that names their slot - never after (the ggwave/pokepedia bug class was