fix: don't let a missing ahocorasick_ner break OCP keyword registration - #552
fix: don't let a missing ahocorasick_ner break OCP keyword registration#552JarbasAl wants to merge 1 commit into
Conversation
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.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
I've scrutinized every line of your PR. Here's the report. 🧐I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthScanning for any signs of code rot or decay. 🍄 ✅ All required files present. Latest Version: ✅ 🔒 Security (pip-audit)Checking for any potential privacy concerns. 🕶️ ✅ No known vulnerabilities found (74 packages scanned). 🔍 LintThe automated pipeline is running smoothly. 🚂 ❌ ruff: issues found — see job log 🔨 Build TestsCompiling thoughts and code into something real. 🧠 ✅ All versions pass
Automatically generated, personally appreciated 💖 |
1 participant
register_ocp_keywordinovos_workshop/skills/common_play.pyhard-depended on the optionalahocorasick_nerpackage. When it wasn't installed, calling the method raised anImportErrorbefore theovos.common_play.register_keywordbus message was ever emitted. That message is what tells the OCP classifier a skill owns a given keyword (an artist name, a movie title, and so on), so without it OCP never learns the skill's vocabulary and can't use it to disambiguate between media types. At least one consumer, the OCP media daemon, ended up reimplementing the same bus emit locally as a workaround.The fix treats the local Aho-Corasick index as what it actually is: an optional optimization for in-process keyword matching, not a prerequisite for talking to OCP.
_register_ocp_nernow always records the samples and, ifahocorasick_neris missing, logs a one-time warning and returns instead of raising.register_ocp_keywordalways reaches the bus emit with the same payload shape it always had. The large-sample-set path (20+ samples), which exports a CSV built from the local matcher, now falls back to sending the raw sample list directly if that export isn't possible, rather than losing the registration.deregister_ocp_keywordwas checked for the same failure mode and doesn't have one — the only NER-dependent call inside it is already commented out pending upstream support, so it was already safe.Five new tests in
test/unittests/skills/test_common_play_extended.pycover the fix: a sanity check that the emit still carries samples with the dependency present, the missing-dependency case (patchingAhocorasickNERtoNone), an empty-samples edge case, the large-sample-set CSV-export fallback, and a check that the warning logs exactly once across repeated calls. All five were verified to fail against the pre-fix code (via a patch-file revert of the source only) and pass after the fix. The full suite runs at 624 passed both before and after, with no pre-existing failures on either side.