From 08c94ddc0abfaedfe5ca7ca30563ac340e8079ab Mon Sep 17 00:00:00 2001 From: JarbasAi Date: Sun, 16 Aug 2026 08:11:50 +0100 Subject: [PATCH] fix: don't let a missing ahocorasick_ner break OCP keyword registration register_ocp_keyword raised ImportError when the optional ahocorasick_ner extra was absent, so the ovos.common_play.register_keyword bus emit never happened and OCP never learned the skill's keywords. The local NER index is an optional optimization; the bus emit is the contract. Missing NER now logs a one-time warning and the message still goes out with the same payload shape, falling back from a CSV export to raw samples when the local matcher can't be built. --- ovos_workshop/skills/common_play.py | 43 ++++--- .../skills/test_common_play_extended.py | 117 ++++++++++++++++++ 2 files changed, 144 insertions(+), 16 deletions(-) diff --git a/ovos_workshop/skills/common_play.py b/ovos_workshop/skills/common_play.py index 800ee436..7466eb25 100644 --- a/ovos_workshop/skills/common_play.py +++ b/ovos_workshop/skills/common_play.py @@ -26,6 +26,8 @@ except ImportError: AhocorasickNER = None # optional dependency +_ner_missing_warned = False + # backwards compat imports, do not delete, skills import from here from ovos_workshop.decorators.ocp import ocp_play, ocp_next, ocp_pause, ocp_resume, ocp_search, \ ocp_previous, ocp_featured_media @@ -239,12 +241,22 @@ def _register_ocp_ner(self, label:str, samples: List[str], lang: str = None): samples (List[str]): A list of phrases or keywords to register for the label. lang (str, optional): The language code for registration. If not specified, registers for all native languages. """ - if AhocorasickNER is None: - raise ImportError("can not register ocp keywords, AhocorasickNER is not installed, 'pip install ahocorasick_ner'") - if label not in self._ocp_ents: self._ocp_ents[label] = [] self._ocp_ents[label] += samples + + global _ner_missing_warned + if AhocorasickNER is None: + # local matching is an optional optimization, the bus registration + # (the actual contract with OCP) still needs to happen + if not _ner_missing_warned: + LOG.warning("ahocorasick_ner is not installed, OCP keyword " + "matching will not run locally, 'pip install " + "ahocorasick_ner' to enable it. keywords are " + "still registered with OCP over the bus") + _ner_missing_warned = True + return + langs = [lang] if lang else self.native_langs for lang in langs: if lang not in self.ocp_matchers: @@ -325,22 +337,21 @@ def register_ocp_keyword(self, media_type: MediaType, label: str, # prefer to search netflix instead of spotify # NB: we send a file path, bus messages with thousands of entities dont work well + payload = {"skill_id": self.skill_id, + "label": label, # if in OCP_ENTITIES it influences classifier + "media_type": media_type} if len(samples) >= 20: csv = f"{self.ocp_cache_dir}/{self.skill_id}_{label}.csv" - self.export_ocp_keywords_csv(csv, label=label) - self.bus.emit( - Message('ovos.common_play.register_keyword', - {"skill_id": self.skill_id, - "label": label, # if in OCP_ENTITIES it influences classifier - "csv": csv, - "media_type": media_type})) + try: + self.export_ocp_keywords_csv(csv, label=label) + payload["csv"] = csv + except Exception as e: + LOG.debug(f"could not export OCP keywords csv, sending " + f"samples directly instead: {e}") + payload["samples"] = samples else: - self.bus.emit( - Message('ovos.common_play.register_keyword', - {"skill_id": self.skill_id, - "label": label, # if in OCP_ENTITIES it influences classifier - "samples": samples, - "media_type": media_type})) + payload["samples"] = samples + self.bus.emit(Message('ovos.common_play.register_keyword', payload)) def deregister_ocp_keyword(self, media_type: MediaType, label: str, langs: List[str] = None): diff --git a/test/unittests/skills/test_common_play_extended.py b/test/unittests/skills/test_common_play_extended.py index 37c2c10f..cf081385 100644 --- a/test/unittests/skills/test_common_play_extended.py +++ b/test/unittests/skills/test_common_play_extended.py @@ -13,8 +13,10 @@ # limitations under the License. """Extended tests for ovos_workshop/skills/common_play.py — OVOSCommonPlaybackSkill.""" import os +import sys import tempfile import unittest +from unittest.mock import patch from ovos_utils.fakebus import FakeBus @@ -112,5 +114,120 @@ def test_ocp_voc_match_no_matchers(self) -> None: self.assertEqual(result, {}) +class TestOCPKeywordSoftFail(unittest.TestCase): + """register_ocp_keyword must still emit ovos.common_play.register_keyword + over the bus even when the optional ahocorasick_ner dependency is missing. + """ + + def setUp(self) -> None: + self.tmp = tempfile.mkdtemp() + self._orig_config_home = os.environ.get("XDG_CONFIG_HOME") + self._orig_cache_home = os.environ.get("XDG_CACHE_HOME") + os.environ["XDG_CONFIG_HOME"] = self.tmp + os.environ["XDG_CACHE_HOME"] = self.tmp + self.bus = FakeBus() + self.skill = _SimplePlaybackSkill.get(self.bus) + + def tearDown(self) -> None: + if self._orig_config_home is not None: + os.environ["XDG_CONFIG_HOME"] = self._orig_config_home + else: + os.environ.pop("XDG_CONFIG_HOME", None) + if self._orig_cache_home is not None: + os.environ["XDG_CACHE_HOME"] = self._orig_cache_home + else: + os.environ.pop("XDG_CACHE_HOME", None) + + def _collect(self, event): + messages = [] + self.bus.on(event, lambda m: messages.append(m)) + return messages + + def test_register_ocp_keyword_emits_with_ner_available(self) -> None: + """Sanity check: with AhocorasickNER present, the message still emits + with the same payload shape (samples list).""" + from ovos_utils.ocp import MediaType + + messages = self._collect("ovos.common_play.register_keyword") + self.skill.register_ocp_keyword(MediaType.MUSIC, "artist_name", + ["queen", "abba"], langs=["en-us"]) + self.assertEqual(len(messages), 1) + self.assertEqual(messages[0].data["skill_id"], "test.common_play") + self.assertEqual(messages[0].data["label"], "artist_name") + self.assertEqual(sorted(messages[0].data["samples"]), ["abba", "queen"]) + + def test_register_ocp_keyword_soft_fails_without_ahocorasick_ner(self) -> None: + """With ahocorasick_ner unavailable (AhocorasickNER is None), the + bus emit must still happen with the same payload shape, and no + exception should propagate.""" + from ovos_utils.ocp import MediaType + import ovos_workshop.skills.common_play as common_play_mod + + messages = self._collect("ovos.common_play.register_keyword") + + with patch.object(common_play_mod, "AhocorasickNER", None): + # must not raise, unlike the unfixed code which raised ImportError + self.skill.register_ocp_keyword(MediaType.MUSIC, "artist_name", + ["queen", "abba"], langs=["en-us"]) + + self.assertEqual(len(messages), 1) + self.assertEqual(messages[0].data["skill_id"], "test.common_play") + self.assertEqual(messages[0].data["label"], "artist_name") + self.assertEqual(messages[0].data["media_type"], MediaType.MUSIC) + self.assertEqual(sorted(messages[0].data["samples"]), ["abba", "queen"]) + # no local matcher was built for the missing optional dependency + self.assertNotIn("en-us", self.skill.ocp_matchers) + + def test_register_ocp_keyword_soft_fails_with_empty_samples(self) -> None: + """Even with an empty sample list and no ahocorasick_ner, the bus + emit is still the contract and must still fire.""" + from ovos_utils.ocp import MediaType + import ovos_workshop.skills.common_play as common_play_mod + + messages = self._collect("ovos.common_play.register_keyword") + + with patch.object(common_play_mod, "AhocorasickNER", None): + self.skill.register_ocp_keyword(MediaType.MUSIC, "artist_name", + [], langs=["en-us"]) + + self.assertEqual(len(messages), 1) + self.assertEqual(messages[0].data["samples"], []) + + def test_register_ocp_keyword_soft_fails_large_sample_set(self) -> None: + """The >=20 samples branch exports a CSV via export_ocp_keywords_csv, + which depends on ocp_matchers being populated. Without + ahocorasick_ner that export is not possible, so the emit must fall + back to sending the raw samples instead of silently dropping them.""" + from ovos_utils.ocp import MediaType + import ovos_workshop.skills.common_play as common_play_mod + + messages = self._collect("ovos.common_play.register_keyword") + samples = [f"track{i}" for i in range(25)] + + with patch.object(common_play_mod, "AhocorasickNER", None): + self.skill.register_ocp_keyword(MediaType.MUSIC, "album_name", + samples, langs=["en-us"]) + + self.assertEqual(len(messages), 1) + self.assertIn("samples", messages[0].data) + self.assertEqual(sorted(messages[0].data["samples"]), sorted(samples)) + self.assertNotIn("csv", messages[0].data) + + def test_register_ocp_keyword_warns_once(self) -> None: + """The missing-dependency warning should be logged, not silently + swallowed, so operators can discover why local matching is off.""" + from ovos_utils.ocp import MediaType + import ovos_workshop.skills.common_play as common_play_mod + + common_play_mod._ner_missing_warned = False + with patch.object(common_play_mod, "AhocorasickNER", None), \ + patch.object(common_play_mod, "LOG") as mock_log: + self.skill.register_ocp_keyword(MediaType.MUSIC, "artist_name", + ["queen"], langs=["en-us"]) + self.skill.register_ocp_keyword(MediaType.MUSIC, "artist_name", + ["abba"], langs=["en-us"]) + self.assertEqual(mock_log.warning.call_count, 1) + + if __name__ == "__main__": unittest.main()