From c2b54916cea2fe0023f595d0d7e104e3e8298767 Mon Sep 17 00:00:00 2001 From: JarbasAI <33701864+JarbasAl@users.noreply.github.com> Date: Thu, 21 Sep 2023 00:39:10 +0100 Subject: [PATCH] feat/active skills from Session (#350) --- ovos_core/intent_services/__init__.py | 61 +++----- ovos_core/intent_services/converse_service.py | 142 +++++++++++++----- test/unittests/skills/test_intent_service.py | 6 +- test/unittests/skills/test_mycroft_skill.py | 8 +- 4 files changed, 138 insertions(+), 79 deletions(-) diff --git a/ovos_core/intent_services/__init__.py b/ovos_core/intent_services/__init__.py index 94f23249003a..5c26cace4486 100644 --- a/ovos_core/intent_services/__init__.py +++ b/ovos_core/intent_services/__init__.py @@ -26,7 +26,7 @@ from ovos_core.intent_services.padacioso_service import PadaciosoService from ovos_core.transformers import MetadataTransformersService, UtteranceTransformersService from ovos_utils.intents.intent_service_interface import open_intent_envelope -from ovos_utils.log import LOG +from ovos_utils.log import LOG, deprecated, log_deprecation from ovos_utils.messagebus import get_message_lang from ovos_utils.metrics import Stopwatch @@ -85,22 +85,12 @@ def __init__(self, bus): self.bus.on('clear_context', self.handle_clear_context) # Converse method - self.bus.on('mycroft.speech.recognition.unknown', self.reset_converse) self.bus.on('mycroft.skills.loaded', self.update_skill_name_dict) - self.bus.on('intent.service.skills.activate', - self.handle_activate_skill_request) - self.bus.on('intent.service.skills.deactivate', - self.handle_deactivate_skill_request) - # TODO backwards compat, deprecate - self.bus.on('active_skill_request', self.handle_activate_skill_request) - # Intents API self.registered_vocab = [] self.bus.on('intent.service.intent.get', self.handle_get_intent) self.bus.on('intent.service.skills.get', self.handle_get_skills) - self.bus.on('intent.service.active_skills.get', - self.handle_get_active_skills) self.bus.on('intent.service.adapt.get', self.handle_get_adapt) self.bus.on('intent.service.adapt.manifest.get', self.handle_adapt_manifest) @@ -153,34 +143,32 @@ def get_skill_name(self, skill_id): # converse handling @property def active_skills(self): - return self.converse.active_skills # [skill_id , timestamp] - + log_deprecation("self.active_skills is deprecated! use Session instead", "0.0.9") + session = SessionManager.get() + return session.active_skills + + @active_skills.setter + def active_skills(self, val): + log_deprecation("self.active_skills is deprecated! use Session instead", "0.0.9") + session = SessionManager.get() + session.active_skills = [] + for skill_id, ts in val: + session.activate_skill(skill_id) + + @deprecated("handle_activate_skill_request moved to ConverseService, overriding this method has no effect, " + "it has been disconnected from the bus event", "0.0.8") def handle_activate_skill_request(self, message): - # TODO imperfect solution - only a skill can activate itself - # someone can forge this message and emit it raw, but in OpenVoiceOS all - # skill messages should have skill_id in context, so let's make sure - # this doesnt happen accidentally at very least - skill_id = message.data['skill_id'] - source_skill = message.context.get("skill_id") - self.converse.activate_skill(skill_id, source_skill) + self.converse.handle_activate_skill_request(message) + @deprecated("handle_deactivate_skill_request moved to ConverseService, overriding this method has no effect, " + "it has been disconnected from the bus event", "0.0.8") def handle_deactivate_skill_request(self, message): - # TODO imperfect solution - only a skill can deactivate itself - # someone can forge this message and emit it raw, but in ovos-core all - # skill message should have skill_id in context, so let's make sure - # this doesnt happen accidentally - skill_id = message.data['skill_id'] - source_skill = message.context.get("skill_id") or skill_id - self.converse.deactivate_skill(skill_id, source_skill) + self.converse.handle_deactivate_skill_request(message) + @deprecated("reset_converse moved to ConverseService, overriding this method has no effect, " + "it has been disconnected from the bus event", "0.0.8") def reset_converse(self, message): - """Let skills know there was a problem with speech recognition""" - lang = get_message_lang(message) - try: - setup_locale(lang) # restore default lang - except Exception as e: - LOG.exception(f"Failed to set lingua_franca default lang to {lang}") - self.converse.converse_with_skills([], lang, message) + self.converse.reset_converse(message) def _handle_transformers(self, message): """ @@ -472,14 +460,15 @@ def handle_get_skills(self, message): self.bus.emit(message.reply("intent.service.skills.reply", {"skills": self.skill_names})) + @deprecated("handle_get_active_skills moved to ConverseService, overriding this method has no effect, " + "it has been disconnected from the bus event", "0.0.8") def handle_get_active_skills(self, message): """Send active skills to caller. Argument: message: query message to reply to. """ - self.bus.emit(message.reply("intent.service.active_skills.reply", - {"skills": self.converse.active_skills})) + self.converse.handle_get_active_skills(message) def handle_get_adapt(self, message): """handler getting the adapt response for an utterance. diff --git a/ovos_core/intent_services/converse_service.py b/ovos_core/intent_services/converse_service.py index 0eb26934ad3b..915ddaa58a57 100644 --- a/ovos_core/intent_services/converse_service.py +++ b/ovos_core/intent_services/converse_service.py @@ -1,10 +1,14 @@ import time -from ovos_bus_client.message import Message + from ovos_config.config import Configuration +from ovos_config.locale import setup_locale import ovos_core.intent_services +from ovos_bus_client.message import Message +from ovos_bus_client.session import SessionManager from ovos_utils import flatten_list from ovos_utils.log import LOG +from ovos_utils.messagebus import get_message_lang from ovos_workshop.permissions import ConverseMode, ConverseActivationMode @@ -14,7 +18,11 @@ class ConverseService: def __init__(self, bus): self.bus = bus self._consecutive_activations = {} - self.active_skills = [] # [skill_id , timestamp] + self.bus.on('mycroft.speech.recognition.unknown', self.reset_converse) + self.bus.on('intent.service.skills.deactivate', self.handle_deactivate_skill_request) + self.bus.on('intent.service.skills.activate', self.handle_activate_skill_request) + self.bus.on('active_skill_request', self.handle_activate_skill_request) # TODO backwards compat, deprecate + self.bus.on('intent.service.active_skills.get', self.handle_get_active_skills) @property def config(self): @@ -24,16 +32,29 @@ def config(self): """ return Configuration().get("skills", {}).get("converse") or {} - def get_active_skills(self): + @property + def active_skills(self): + session = SessionManager.get() + return session.active_skills + + @active_skills.setter + def active_skills(self, val): + session = SessionManager.get() + session.active_skills = [] + for skill_id, ts in val: + session.activate_skill(skill_id) + + def get_active_skills(self, message=None): """Active skill ids ordered by converse priority this represents the order in which converse will be called Returns: active_skills (list): ordered list of skill_ids """ - return [skill[0] for skill in self.active_skills] + session = SessionManager.get(message) + return [skill[0] for skill in session.active_skills] - def deactivate_skill(self, skill_id, source_skill=None): + def deactivate_skill(self, skill_id, source_skill=None, message=None): """Remove a skill from being targetable by converse. Args: @@ -42,18 +63,23 @@ def deactivate_skill(self, skill_id, source_skill=None): """ source_skill = source_skill or skill_id if self._deactivate_allowed(skill_id, source_skill): - active_skills = self.get_active_skills() - if skill_id in active_skills: - idx = active_skills.index(skill_id) - self.active_skills.pop(idx) + session = SessionManager.get(message) + if session.is_active(skill_id): + # update converse session + if message: + session.update_history(message) + session.deactivate_skill(skill_id) + + # keep message.context + message = message or Message("") + # send bus event self.bus.emit( - Message("intent.service.skills.deactivated", - data={"skill_id": skill_id}, - context={"skill_id": skill_id})) + message.forward("intent.service.skills.deactivated", + data={"skill_id": skill_id})) if skill_id in self._consecutive_activations: self._consecutive_activations[skill_id] = 0 - def activate_skill(self, skill_id, source_skill=None): + def activate_skill(self, skill_id, source_skill=None, message=None): """Add a skill or update the position of an active skill. The skill is added to the front of the list, if it's already in the @@ -65,20 +91,19 @@ def activate_skill(self, skill_id, source_skill=None): """ source_skill = source_skill or skill_id if self._activate_allowed(skill_id, source_skill): - # NOTE: do not call self.remove_active_skill - # do not want to send the deactivation bus event! - active_skills = self.get_active_skills() - if skill_id in active_skills: - idx = active_skills.index(skill_id) - self.active_skills.pop(idx) - - # add skill with timestamp to start of skill_list - self.active_skills.insert(0, [skill_id, time.time()]) - self.bus.emit( - Message("intent.service.skills.activated", - data={"skill_id": skill_id}, - context={"skill_id": skill_id})) - + # update converse session + session = SessionManager.get(message) + if message: + session.update_history(message) + session.activate_skill(skill_id) + + # keep message.context + message = message or Message("") + message = message.forward("intent.service.skills.activated", + {"skill_id": skill_id}) + # send bus event + self.bus.emit(message) + # update activation counter self._consecutive_activations[skill_id] += 1 def _activate_allowed(self, skill_id, source_skill=None): @@ -115,7 +140,7 @@ def _activate_allowed(self, skill_id, source_skill=None): # define their default priority, this is a user/developer setting priority = prio.get(skill_id, 50) if any(p > priority for p in - [prio.get(s[0], 50) for s in self.active_skills]): + [prio.get(s, 50) for s in self.get_active_skills()]): return False elif acmode == ConverseActivationMode.BLACKLIST: if skill_id in self.config.get("converse_blacklist", []): @@ -181,16 +206,16 @@ def _converse_allowed(self, skill_id): return False return True - def _collect_converse_skills(self): + def _collect_converse_skills(self, message): """use the messagebus api to determine which skills want to converse This includes all skills and external applications""" skill_ids = [] want_converse = [] active_skills = self.get_active_skills() - def handle_ack(message): - skill_id = message.data["skill_id"] - if message.data.get("can_handle", True): + def handle_ack(msg): + skill_id = msg.data["skill_id"] + if msg.data.get("can_handle", True): if skill_id in active_skills: want_converse.append(skill_id) skill_ids.append(skill_id) @@ -198,7 +223,7 @@ def handle_ack(message): self.bus.on("skill.converse.pong", handle_ack) # wait for all skills to acknowledge they want to converse - self.bus.emit(Message("skill.converse.ping")) + self.bus.emit(message.forward("skill.converse.ping")) start = time.time() while not all(s in skill_ids for s in active_skills) \ and time.time() - start <= 0.5: @@ -207,12 +232,13 @@ def handle_ack(message): self.bus.remove("skill.converse.pong", handle_ack) return want_converse - def _check_converse_timeout(self): + def _check_converse_timeout(self, message): """ filter active skill list based on timestamps """ timeouts = self.config.get("skill_timeouts") or {} def_timeout = self.config.get("timeout", 300) - self.active_skills = [ - skill for skill in self.active_skills + session = SessionManager.get(message) + session.active_skills = [ + skill for skill in session.active_skills if time.time() - skill[1] <= timeouts.get(skill[0], def_timeout)] def converse(self, utterances, skill_id, lang, message): @@ -228,7 +254,10 @@ def converse(self, utterances, skill_id, lang, message): Returns: handled (bool): True if handled otherwise False. """ + session = SessionManager.get(message) + session.lang = lang if self._converse_allowed(skill_id): + session.update_history(message) converse_msg = message.reply("skill.converse.request", {"skill_id": skill_id, "utterances": utterances, @@ -257,9 +286,46 @@ def converse_with_skills(self, utterances, lang, message): # we call flatten in case someone is sending the old style list of tuples utterances = flatten_list(utterances) # filter allowed skills - self._check_converse_timeout() + self._check_converse_timeout(message) # check if any skill wants to handle utterance - for skill_id in self._collect_converse_skills(): + for skill_id in self._collect_converse_skills(message): if self.converse(utterances, skill_id, lang, message): return ovos_core.intent_services.IntentMatch('Converse', None, None, skill_id, utterances[0]) return None + + def handle_activate_skill_request(self, message): + # TODO imperfect solution - only a skill can activate itself + # someone can forge this message and emit it raw, but in OpenVoiceOS all + # skill messages should have skill_id in context, so let's make sure + # this doesnt happen accidentally at very least + skill_id = message.data['skill_id'] + source_skill = message.context.get("skill_id") + self.activate_skill(skill_id, source_skill, message) + + def handle_deactivate_skill_request(self, message): + # TODO imperfect solution - only a skill can deactivate itself + # someone can forge this message and emit it raw, but in ovos-core all + # skill message should have skill_id in context, so let's make sure + # this doesnt happen accidentally + skill_id = message.data['skill_id'] + source_skill = message.context.get("skill_id") or skill_id + self.deactivate_skill(skill_id, source_skill, message) + + def reset_converse(self, message): + """Let skills know there was a problem with speech recognition""" + lang = get_message_lang(message) + try: + setup_locale(lang) # restore default lang + except Exception as e: + LOG.exception(f"Failed to set lingua_franca default lang to {lang}") + + self.converse_with_skills([], lang, message) + + def handle_get_active_skills(self, message): + """Send active skills to caller. + + Argument: + message: query message to reply to. + """ + self.bus.emit(message.reply("intent.service.active_skills.reply", + {"skills": self.get_active_skills(message)})) diff --git a/test/unittests/skills/test_intent_service.py b/test/unittests/skills/test_intent_service.py index e6dab4460026..cba2b55ac56c 100644 --- a/test/unittests/skills/test_intent_service.py +++ b/test/unittests/skills/test_intent_service.py @@ -98,8 +98,8 @@ def setUp(self): self.intent_service.converse.activate_skill('atari_skill') self.intent_service.converse.activate_skill('c64_skill') - def _collect(): - return [i[0] for i in self.intent_service.active_skills] + def _collect(message=None): + return [i[0] for i in self.intent_service.converse.active_skills] self.intent_service.converse._collect_converse_skills = _collect @@ -183,7 +183,7 @@ def response(message, return_msg_type): 'result': False}) msgs = {'c64_skill': c64, 'atari_skill': atari} - return msgs[message.data['skill_id']] + return msgs.get(message.data['skill_id'], None) reset_msg = Message('mycroft.speech.recognition.unknown', data={'lang': 'en-US'}) diff --git a/test/unittests/skills/test_mycroft_skill.py b/test/unittests/skills/test_mycroft_skill.py index 6581c60f824f..95165fd5e242 100644 --- a/test/unittests/skills/test_mycroft_skill.py +++ b/test/unittests/skills/test_mycroft_skill.py @@ -23,12 +23,13 @@ from adapt.intent import IntentBuilder from ovos_config import Configuration -from ovos_utils.intents.intent_service_interface import open_intent_envelope from mycroft.skills.skill_data import (load_regex_from_file, load_regex, load_vocabulary, read_vocab_file) from ovos_bus_client.message import Message +from ovos_utils.intents.intent_service_interface import open_intent_envelope from ovos_workshop.decorators import resting_screen_handler, intent_handler +from ovos_workshop.skills.mycroft_skill import MycroftSkill from ovos_workshop.skills.ovos import OVOSSkill from test.util import base_config @@ -631,8 +632,9 @@ class _TestSkill(OVOSSkill): pass -class SimpleSkill1(_TestSkill): +class SimpleSkill1(MycroftSkill): """ Test skill for normal intent builder syntax """ + def initialize(self): self.handler_run = False i = IntentBuilder('a').require('Keyword').build() @@ -647,6 +649,7 @@ def stop(self): class SimpleSkill2(_TestSkill): """ Test skill for intent builder without .build() """ + def initialize(self): i = IntentBuilder('a').require('Keyword') self.register_intent(i, self.handler) @@ -660,6 +663,7 @@ def stop(self): class SimpleSkill3(_TestSkill): """ Test skill for invalid Intent for register_intent """ + def initialize(self): self.register_intent('string', self.handler)