From 5215c91ba4cc9ea8b9c96ae903c35e90c707df26 Mon Sep 17 00:00:00 2001 From: Daniel McKnight Date: Tue, 10 Oct 2023 07:32:36 -0700 Subject: [PATCH 1/4] Emit `question:handler_complete` when a CQS action is completed (functional equivalent to `mycroft.skill.handler.complete`) Adds docstrings and type annotations to CommonQuerySkill base class Adds minimal tests for CommonQuerySkill --- ovos_workshop/skills/common_query_skill.py | 114 ++++++++++++------ .../skills/test_common_query_skill.py | 53 ++++++++ 2 files changed, 127 insertions(+), 40 deletions(-) create mode 100644 test/unittests/skills/test_common_query_skill.py diff --git a/ovos_workshop/skills/common_query_skill.py b/ovos_workshop/skills/common_query_skill.py index f3757e47..7f180282 100644 --- a/ovos_workshop/skills/common_query_skill.py +++ b/ovos_workshop/skills/common_query_skill.py @@ -13,9 +13,11 @@ from abc import abstractmethod from enum import IntEnum from os.path import dirname +from typing import List, Optional +from ovos_bus_client import Message from ovos_utils.file_utils import resolve_resource_file -from ovos_utils.log import LOG +from ovos_utils.log import LOG, log_deprecation from ovos_workshop.skills.ovos import OVOSSkill from ovos_workshop.decorators.compat import backwards_compat @@ -81,13 +83,18 @@ def __init__(self, name=None, bus=None, **kwargs): translated_noise_words.split() @property - def translated_noise_words(self): - LOG.warning("self.translated_noise_words will become a private variable in next release") + def translated_noise_words(self) -> List[str]: + """ + Get a list of "noise" words in the current language + """ + log_deprecation("self.translated_noise_words will become a " + "private variable", "0.1.0") return self._translated_noise_words.get(self.lang, []) @translated_noise_words.setter - def translated_noise_words(self, val): - LOG.warning("self.translated_noise_words will become a private variable in next release") + def translated_noise_words(self, val: List[str]): + log_deprecation("self.translated_noise_words will become a " + "private variable", "0.1.0") self._translated_noise_words[self.lang] = val def bind(self, bus): @@ -98,10 +105,18 @@ def bind(self, bus): """ if bus: super().bind(bus) - self.add_event('question:query', self.__handle_question_query, speak_errors=False) - self.add_event('question:action', self.__handle_query_action, speak_errors=False) + self.add_event('question:query', self.__handle_question_query, + speak_errors=False) + self.add_event('question:action', self.__handle_query_action, + speak_errors=False) - def __handle_question_query(self, message): + def __handle_question_query(self, message: Message): + """ + Handle an incoming user query. Get a result from this skill's + `CQS_match_query_phrase` method and emit a response back to the intent + service. + @param message: Message with matched query 'phrase' + """ search_phrase = message.data["phrase"] message.context["skill_id"] = self.skill_id # First, notify the requestor that we are attempting to handle @@ -131,8 +146,14 @@ def __handle_question_query(self, message): "skill_id": self.skill_id, "searching": False})) - def __get_cq(self, search_phrase): - # Now invoke the CQS handler to let the skill perform its search + def __get_cq(self, search_phrase: str) -> List[str, CQSMatchLevel, str, + Optional[dict]]: + """ + Invoke the CQS handler to let the skill perform its search + @param search_phrase: parsed question to get an answer for + @return: (matched substring from search_phrase, + confidence level of match, speakable answer, optional callback data) + """ try: result = self.CQS_match_query_phrase(search_phrase) except: @@ -140,8 +161,13 @@ def __get_cq(self, search_phrase): result = None return result - def remove_noise(self, phrase, lang=None): - """remove noise to produce essence of question""" + def remove_noise(self, phrase: str, lang: str = None) -> str: + """ + Remove extra words from the query to produce essence of question + @param phrase: raw phrase to parse (usually from the intent service) + @param lang: language of `phrase`, else defaults to `self.lang` + @return: cleaned `phrase` with extra words removed + """ lang = lang or self.lang phrase = ' ' + phrase + ' ' for word in self._translated_noise_words.get(lang, []): @@ -151,7 +177,16 @@ def remove_noise(self, phrase, lang=None): phrase = ' '.join(phrase.split()) return phrase.strip() - def __calc_confidence(self, match, phrase, level, answer): + def __calc_confidence(self, match: str, phrase: str, level: CQSMatchLevel, + answer: str) -> float: + """ + Calculate a confidence level for the skill response. + @param match: Matched portion of the input phrase + @param phrase: User input phrase that was evaluated + @param level: Skill-determined match level of the answer + @param answer: Speakable response to the input phrase + @return: Float (0.0-1.0) confidence level of the response + """ # Assume the more of the words that get consumed, the better the match consumed_pct = len(match.split()) / len(phrase.split()) if consumed_pct > 1.0: @@ -189,7 +224,9 @@ def __calc_confidence(self, match, phrase, level, answer): return confidence def __handle_query_classic(self, message): - """does not perform self.speak, < 0.0.8 this is done by core itself""" + """ + does not perform self.speak, < 0.0.8 this is done by core itself + """ if message.data["skill_id"] != self.skill_id: # Not for this skill! return @@ -198,12 +235,14 @@ def __handle_query_classic(self, message): # Invoke derived class to provide playback data self.CQS_action(phrase, data) - @backwards_compat(classic_core=__handle_query_classic, pre_008=__handle_query_classic) - def __handle_query_action(self, message): - """Message handler for question:action. - - Extracts phrase and data from message forward this to the skills - CQS_action method. + @backwards_compat(classic_core=__handle_query_classic, + pre_008=__handle_query_classic) + def __handle_query_action(self, message: Message): + """ + If this skill's response was spoken to the user, this method is called. + Phrase and callback data from `CQS_match_query_phrase` will be passed + to the `CQS_action` method. + @param message: `question:action` message """ if message.data["skill_id"] != self.skill_id: # Not for this skill! @@ -214,35 +253,30 @@ def __handle_query_action(self, message): self.speak(data["answer"]) # Invoke derived class to provide playback data self.CQS_action(phrase, data) + self.bus.emit(message.forward("question:handler_complete")) @abstractmethod - def CQS_match_query_phrase(self, phrase): - """Analyze phrase to see if it is a answer-able phrase with this skill. - - Needs to be implemented by the skill. - - Args: - phrase (str): User phrase, "What is an aardwark" - - Returns: - (match, CQSMatchLevel[, callback_data]) or None: Tuple containing - a string with the appropriate matching phrase, the PlayMatch - type, and optionally data to return in the callback if the - match is selected. + def CQS_match_query_phrase(self, phrase: str) -> \ + Optional[(str, CQSMatchLevel, Optional[dict])]: + """ + Determine an answer to the input phrase and return match information, or + `None` if no answer can be determined. + @param phrase: User question, i.e. "What is an aardvark" + @return: (matched portion of the phrase, match confidence level, + optional callback data) if this skill can answer the question, + else None. """ - # Derived classes must implement this, e.g. return None - def CQS_action(self, phrase, data): - """Take additional action IF the skill is selected. + def CQS_action(self, phrase: str, data: dict): + """ + Take additional action IF the skill is selected. The speech is handled by the common query but if the chosen skill wants to display media, set a context or prepare for sending information info over e-mail this can be implemented here. - - Args: - phrase (str): User phrase uttered after "Play", e.g. "some music" - data (dict): Callback data specified in match_query_phrase() + @param phrase: User phrase, i.e. "What is an aardvark" + @param data: Callback data specified in CQS_match_query_phrase """ # Derived classes may implement this if they use additional media # or wish to set context after being called. diff --git a/test/unittests/skills/test_common_query_skill.py b/test/unittests/skills/test_common_query_skill.py new file mode 100644 index 00000000..cebd07b9 --- /dev/null +++ b/test/unittests/skills/test_common_query_skill.py @@ -0,0 +1,53 @@ +from unittest import TestCase +from unittest.mock import patch + +from ovos_utils.messagebus import FakeBus +from ovos_workshop.skills.base import BaseSkill +from ovos_workshop.skills.common_query_skill import CommonQuerySkill, CQSMatchLevel + + +class TestQASkill(CommonQuerySkill): + def CQS_match_query_phrase(self, phrase): + pass + + def CQS_action(self, phrase, data): + pass + + +class TestCommonQuerySkill(TestCase): + skill = TestQASkill("test_common_query", FakeBus()) + + def test_class_inheritance(self): + from ovos_workshop.skills.ovos import OVOSSkill + from ovos_workshop.skills.mycroft_skill import MycroftSkill + self.assertIsInstance(self.skill, BaseSkill) + self.assertIsInstance(self.skill, OVOSSkill) + self.assertIsInstance(self.skill, MycroftSkill) + self.assertIsInstance(self.skill, CommonQuerySkill) + + def test_00_skill_init(self): + for conf in self.skill.level_confidence: + self.assertIsInstance(conf, CQSMatchLevel) + self.assertIsInstance(self.skill.level_confidence[conf], float) + self.assertIsNotNone(self.skill.bus.ee.listeners("question:query")) + self.assertIsNotNone(self.skill.bus.ee.listeners("question:action")) + + def test_handle_question_query(self): + # TODO + pass + + def test_get_cq(self): + # TODO + pass + + def test_remove_noise(self): + # TODO + pass + + def test_calc_confidence(self): + # TODO + pass + + def test_handle_query_action(self): + # TODO + pass From 5869a895c8983819d7d073e1c7e9cfac73df00c9 Mon Sep 17 00:00:00 2001 From: Daniel McKnight Date: Tue, 10 Oct 2023 07:36:05 -0700 Subject: [PATCH 2/4] Fix return type annotation --- ovos_workshop/skills/common_query_skill.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ovos_workshop/skills/common_query_skill.py b/ovos_workshop/skills/common_query_skill.py index 7f180282..ae4c759b 100644 --- a/ovos_workshop/skills/common_query_skill.py +++ b/ovos_workshop/skills/common_query_skill.py @@ -146,8 +146,8 @@ def __handle_question_query(self, message: Message): "skill_id": self.skill_id, "searching": False})) - def __get_cq(self, search_phrase: str) -> List[str, CQSMatchLevel, str, - Optional[dict]]: + def __get_cq(self, search_phrase: str) -> (str, CQSMatchLevel, str, + Optional[dict]): """ Invoke the CQS handler to let the skill perform its search @param search_phrase: parsed question to get an answer for From b3e277b38aa7dcf69aa8b5d7bbc3a2fc14c0e4c0 Mon Sep 17 00:00:00 2001 From: Daniel McKnight Date: Tue, 10 Oct 2023 07:38:19 -0700 Subject: [PATCH 3/4] Fix return type annotation --- ovos_workshop/skills/common_query_skill.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ovos_workshop/skills/common_query_skill.py b/ovos_workshop/skills/common_query_skill.py index ae4c759b..86e99038 100644 --- a/ovos_workshop/skills/common_query_skill.py +++ b/ovos_workshop/skills/common_query_skill.py @@ -13,7 +13,7 @@ from abc import abstractmethod from enum import IntEnum from os.path import dirname -from typing import List, Optional +from typing import List, Optional, Tuple from ovos_bus_client import Message from ovos_utils.file_utils import resolve_resource_file @@ -257,7 +257,7 @@ def __handle_query_action(self, message: Message): @abstractmethod def CQS_match_query_phrase(self, phrase: str) -> \ - Optional[(str, CQSMatchLevel, Optional[dict])]: + Optional[Tuple[str, CQSMatchLevel, Optional[dict]]]: """ Determine an answer to the input phrase and return match information, or `None` if no answer can be determined. From cd5dcce3142f070932ac03ec5ca7e81f85a73d8a Mon Sep 17 00:00:00 2001 From: Daniel McKnight Date: Tue, 10 Oct 2023 08:03:33 -0700 Subject: [PATCH 4/4] Update emitted message to use current `mycroft.skill.handler.complete` --- ovos_workshop/skills/common_query_skill.py | 4 ++-- test/unittests/skills/test_common_query_skill.py | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/ovos_workshop/skills/common_query_skill.py b/ovos_workshop/skills/common_query_skill.py index 86e99038..13e0ee48 100644 --- a/ovos_workshop/skills/common_query_skill.py +++ b/ovos_workshop/skills/common_query_skill.py @@ -253,8 +253,8 @@ def __handle_query_action(self, message: Message): self.speak(data["answer"]) # Invoke derived class to provide playback data self.CQS_action(phrase, data) - self.bus.emit(message.forward("question:handler_complete")) - + self.bus.emit(message.forward("mycroft.skill.handler.complete", + {"handler": "common_query"})) @abstractmethod def CQS_match_query_phrase(self, phrase: str) -> \ Optional[Tuple[str, CQSMatchLevel, Optional[dict]]]: diff --git a/test/unittests/skills/test_common_query_skill.py b/test/unittests/skills/test_common_query_skill.py index cebd07b9..ef182e86 100644 --- a/test/unittests/skills/test_common_query_skill.py +++ b/test/unittests/skills/test_common_query_skill.py @@ -1,5 +1,4 @@ from unittest import TestCase -from unittest.mock import patch from ovos_utils.messagebus import FakeBus from ovos_workshop.skills.base import BaseSkill