MycroftAI
/
mycroft-core
mirror of https://github.com/MycroftAI/mycroft-core.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

Handle multiple intents with the same name (#2921) 

* Add check for duplicate adapt intents

There are two cases, duplicated named intent and duplicated anonymous intent.

A named intent will cause a ValueError exception notifying the skill
author that there is a collision.

An anonymous intent will silently derive a new name and use that
instead of the default generated one.

* Add tests for intent collisions

* Make enable/disable intent handle the new exception

The enable/disable intent did not mark an intent as detached, instead it
remained in the list of intents after disabling in the IntentServiceInterface
to be retrieved when the intent should be re-enabled.

This moves detached intents into a list of detached intents to so they
won't cause the double enable exception.

* Add move logic to find if intent is detached

MycroftSkill.enable_intent() will now check if the intent is detached
before trying to re-enable it.

* Lock updates of intents

This should avoid some race conditions that may occur if multiple
threads tries to enable / disable intents
pull/3133/head^2
Åke 2022-11-23 07:22:56 +01:00 committed by GitHub
parent 5f964f7d40
commit ab242a2c82
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 147 additions and 40 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

37
mycroft/skills/intent_service_interface.py
View File

@ -35,6 +35,7 @@ class IntentServiceInterface:
def __init__(self, bus=None):
self.bus = bus
self.registered_intents = []
self.detached_intents = []
def set_bus(self, bus):
self.bus = bus
@ -83,14 +84,40 @@ class IntentServiceInterface:
"""
self.bus.emit(Message("register_intent", intent_parser.__dict__))
self.registered_intents.append((name, intent_parser))
self.detached_intents = [detached for detached in self.detached_intents
if detached[0] != name]
def detach_intent(self, intent_name):
"""Remove an intent from the intent service.
The intent is saved in the list of detached intents for use when
re-enabling an intent.
Args:
intent_name(str): Intent reference
"""
self.bus.emit(Message("detach_intent", {"intent_name": intent_name}))
name = intent_name.split(':')[1]
if name in self:
self.bus.emit(Message("detach_intent",
{"intent_name": intent_name}))
self.detached_intents.append((name, self.get_intent(name)))
self.registered_intents = [pair for pair in self.registered_intents
if pair[0] != name]
def intent_is_detached(self, intent_name):
"""Determine if an intent is detached.
Args:
intent_name(str): Intent reference
Returns:
(bool) True if intent is found, else False.
"""
for (name, _) in self.detached_intents:
if name == intent_name:
return True
return False
def set_adapt_context(self, context, word, origin):
"""Set an Adapt context.
@ -161,6 +188,8 @@ class IntentServiceInterface:
def get_intent(self, intent_name):
"""Get intent from intent_name.
This will find both enabled and disabled intents.
Args:
intent_name (str): name to find.
@ -170,8 +199,10 @@ class IntentServiceInterface:
for name, intent in self:
if name == intent_name:
return intent
else:
return None
for name, intent in self.detached_intents:
if name == intent_name:
return intent
return None
class IntentQueryApi:

106
mycroft/skills/mycroft_skill/mycroft_skill.py
View File

@ -22,7 +22,7 @@ from itertools import chain
from os import walk
from os.path import join, abspath, dirname, basename, exists
from pathlib import Path
from threading import Event, Timer
from threading import Event, Timer, Lock
from xdg import BaseDirectory
@ -161,6 +161,7 @@ class MycroftSkill:
# Delegator classes
self.event_scheduler = EventSchedulerInterface(self.name)
self.intent_service = IntentServiceInterface()
self.intent_service_lock = Lock()
# Skill Public API
self.public_api = {}
@ -364,9 +365,10 @@ class MycroftSkill:
self.settings_change_callback()
def detach(self):
for (name, _) in self.intent_service:
name = '{}:{}'.format(self.skill_id, name)
self.intent_service.detach_intent(name)
with self.intent_service_lock:
for (name, _) in self.intent_service:
name = '{}:{}'.format(self.skill_id, name)
self.intent_service.detach_intent(name)
def initialize(self):
"""Perform any final setup needed for the skill.
@ -958,14 +960,28 @@ class MycroftSkill:
def _register_adapt_intent(self, intent_parser, handler):
"""Register an adapt intent.
Will handle registration of anonymous
Args:
intent_parser: Intent object to parse utterance for the handler.
handler (func): function to register with intent
"""
# Default to the handler's function name if none given
is_anonymous = not intent_parser.name
name = intent_parser.name or handler.__name__
if is_anonymous:
# Find a good name
original_name = name
nbr = 0
while name in self.intent_service:
nbr += 1
name = f'{original_name}{nbr}'
else:
if name in self.intent_service:
raise ValueError(f'The intent name {name} is already taken')
munge_intent_parser(intent_parser, name, self.skill_id)
self.intent_service.register_adapt_intent(name, intent_parser)
if handler:
self.add_event(intent_parser.name, handler,
'mycroft.skill.handler')
@ -973,6 +989,17 @@ class MycroftSkill:
def register_intent(self, intent_parser, handler):
"""Register an Intent with the intent service.
Args:
intent_parser: Intent, IntentBuilder object or padatious intent
file to parse utterance for the handler.
handler (func): function to register with intent
"""
with self.intent_service_lock:
self._register_intent(intent_parser, handler)
def _register_intent(self, intent_parser, handler):
"""Register an Intent with the intent service.
Args:
intent_parser: Intent, IntentBuilder object or padatious intent
file to parse utterance for the handler.
@ -1019,6 +1046,7 @@ class MycroftSkill:
filename = self.find_resource(intent_file, 'vocab')
if not filename:
raise FileNotFoundError('Unable to find "{}"'.format(intent_file))
self.intent_service.register_padatious_intent(name, filename)
if handler:
self.add_event(name, handler, 'mycroft.skill.handler')
@ -1047,23 +1075,20 @@ class MycroftSkill:
raise FileNotFoundError('Unable to find "{}"'.format(entity_file))
name = '{}:{}'.format(self.skill_id, entity_file)
self.intent_service.register_padatious_entity(name, filename)
with self.intent_service_lock:
self.intent_service.register_padatious_entity(name, filename)
def handle_enable_intent(self, message):
"""Listener to enable a registered intent if it belongs to this skill.
"""
intent_name = message.data['intent_name']
for (name, _) in self.intent_service:
if name == intent_name:
return self.enable_intent(intent_name)
return self.enable_intent(intent_name)
def handle_disable_intent(self, message):
"""Listener to disable a registered intent if it belongs to this skill.
"""
intent_name = message.data['intent_name']
for (name, _) in self.intent_service:
if name == intent_name:
return self.disable_intent(intent_name)
self.disable_intent(intent_name)
def disable_intent(self, intent_name):
"""Disable a registered intent if it belongs to this skill.
@ -1074,15 +1099,16 @@ class MycroftSkill:
Returns:
bool: True if disabled, False if it wasn't registered
"""
if intent_name in self.intent_service:
LOG.debug('Disabling intent ' + intent_name)
name = '{}:{}'.format(self.skill_id, intent_name)
self.intent_service.detach_intent(name)
return True
else:
LOG.error('Could not disable '
'{}, it hasn\'t been registered.'.format(intent_name))
return False
with self.intent_service_lock:
if intent_name in self.intent_service:
LOG.info('Disabling intent ' + intent_name)
name = '{}:{}'.format(self.skill_id, intent_name)
self.intent_service.detach_intent(name)
return True
else:
LOG.error('Could not disable '
f'{intent_name}, it hasn\'t been registered.')
return False
def enable_intent(self, intent_name):
"""(Re)Enable a registered intent if it belongs to this skill.
@ -1094,17 +1120,21 @@ class MycroftSkill:
bool: True if enabled, False if it wasn't registered
"""
intent = self.intent_service.get_intent(intent_name)
if intent:
if ".intent" in intent_name:
self.register_intent_file(intent_name, None)
with self.intent_service_lock:
if intent and self.intent_service.intent_is_detached(intent_name):
if ".intent" in intent_name:
self.register_intent_file(intent_name, None)
else:
intent.name = intent_name
self._register_intent(intent, None)
LOG.debug('Enabling intent {}'.format(intent_name))
return True
elif intent:
LOG.error(f'Could not enable {intent_name}, '
'it\'s not detached')
else:
intent.name = intent_name
self.register_intent(intent, None)
LOG.debug('Enabling intent {}'.format(intent_name))
return True
else:
LOG.error('Could not enable '
'{}, it hasn\'t been registered.'.format(intent_name))
LOG.error('Could not enable '
f'{intent_name}, it hasn\'t been registered.')
return False
def set_context(self, context, word='', origin=''):
@ -1169,7 +1199,8 @@ class MycroftSkill:
entity_type: Intent handler entity to tie the word to
"""
keyword_type = to_alnum(self.skill_id) + entity_type
self.intent_service.register_adapt_keyword(keyword_type, entity)
with self.intent_service_lock:
self.intent_service.register_adapt_keyword(keyword_type, entity)
def register_regex(self, regex_str):
"""Register a new regex.
@ -1179,7 +1210,8 @@ class MycroftSkill:
self.log.debug('registering regex string: ' + regex_str)
regex = munge_regex(regex_str, self.skill_id)
re.compile(regex) # validate regex
self.intent_service.register_adapt_regex(regex)
with self.intent_service_lock:
self.intent_service.register_adapt_regex(regex)
def speak(self, utterance, expect_response=False, wait=False, meta=None):
"""Speak a sentence.
@ -1295,9 +1327,10 @@ class MycroftSkill:
for line in keywords[vocab_type]:
entity = line[0]
aliases = line[1:]
self.intent_service.register_adapt_keyword(vocab_type,
entity,
aliases)
with self.intent_service_lock:
self.intent_service.register_adapt_keyword(vocab_type,
entity,
aliases)
def load_regex_files(self, root_directory):
""" Load regex files found under the skill directory.
@ -1314,7 +1347,8 @@ class MycroftSkill:
regexes = load_regex(locale_dir, self.skill_id)
for regex in regexes:
self.intent_service.register_adapt_regex(regex)
with self.intent_service_lock:
self.intent_service.register_adapt_regex(regex)
def __handle_stop(self, _):
"""Handler for the "mycroft.stop" signal. Runs the user defined

44
test/unittests/skills/test_mycroft_skill.py
View File

@ -27,7 +27,8 @@ from mycroft.configuration import Configuration
from mycroft.messagebus.message import Message
from mycroft.skills.skill_data import (load_regex_from_file, load_regex,
load_vocabulary, read_vocab_file)
from mycroft.skills.core import MycroftSkill, resting_screen_handler
from mycroft.skills import (MycroftSkill, resting_screen_handler,
intent_handler)
from mycroft.skills.intent_service import open_intent_envelope
from test.util import base_config
@ -628,6 +629,23 @@ class TestMycroftSkill(unittest.TestCase):
s.speak_dialog(key='key')
class TestIntentCollisions(unittest.TestCase):
def test_two_intents_with_same_name(self):
emitter = MockEmitter()
skill = SameIntentNameSkill()
skill.bind(emitter)
with self.assertRaises(ValueError):
skill.initialize()
def test_two_anonymous_intent_decorators(self):
"""Two anonymous intent handlers should be ok."""
emitter = MockEmitter()
skill = SameAnonymousIntentDecoratorsSkill()
skill.bind(emitter)
skill._register_decorated()
self.assertEqual(len(skill.intent_service.registered_intents), 2)
class _TestSkill(MycroftSkill):
def __init__(self):
super().__init__()
@ -705,3 +723,27 @@ class SimpleSkill6(_TestSkill):
def handler(self, message):
pass
class SameIntentNameSkill(_TestSkill):
"""Test skill for duplicate intent namesr."""
skill_id = 'A'
def initialize(self):
intent = IntentBuilder('TheName').require('Keyword')
intent2 = IntentBuilder('TheName').require('Keyword')
self.register_intent(intent, self.handler)
self.register_intent(intent2, self.handler)
def handler(self, message):
pass
class SameAnonymousIntentDecoratorsSkill(_TestSkill):
"""Test skill for duplicate anonymous intent handlers."""
skill_id = 'A'
@intent_handler(IntentBuilder('').require('Keyword'))
@intent_handler(IntentBuilder('').require('OtherKeyword'))
def handler(self, message):
pass