From 693100e8c61566ffb140b53ebb76a1303bada238 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=85ke=20Forslund?= Date: Thu, 29 Jul 2021 13:10:54 +0200 Subject: [PATCH 1/3] Match keyword entity terms in Mycroft with Adapt This changes the internally used names for entities and entity values when sent on the messagebus and used interanally in the intent service from start / end to entity_value and entity_type. This makes the terminology easier to understand and follow across into Adapt. The old terms are still included and usable for compatibility but should be removed in an upcoming major release (22.02). --- mycroft/skills/intent_service.py | 40 +++++++++++++++++-- mycroft/skills/intent_service_interface.py | 24 ++++++++--- .../skills/intent_services/adapt_service.py | 25 +++++++++++- test/unittests/skills/test_intent_service.py | 29 +++++++++++--- .../skills/test_intent_service_interface.py | 36 +++++++++++++---- 5 files changed, 130 insertions(+), 24 deletions(-) diff --git a/mycroft/skills/intent_service.py b/mycroft/skills/intent_service.py index 4a17a001272c..f3ab801fc3c7 100644 --- a/mycroft/skills/intent_service.py +++ b/mycroft/skills/intent_service.py @@ -358,12 +358,18 @@ def handle_register_vocab(self, message): Args: message (Message): message containing vocab info """ - start_concept = message.data.get('start') - end_concept = message.data.get('end') + # TODO: 22.02 Remove backwards compatibility + if _is_old_style_keyword_message(message): + LOG.warning('Deprecated: Registering keywords with old message. ' + 'This will be removed in v22.02.') + _update_keyword_message(message) + + entity_value = message.data.get('entity_value') + entity_type = message.data.get('entity_type') regex_str = message.data.get('regex') alias_of = message.data.get('alias_of') - self.adapt_service.register_vocab(start_concept, end_concept, - alias_of, regex_str) + self.adapt_service.register_vocabulary(entity_value, entity_type, + alias_of, regex_str) self.registered_vocab.append(message.data) def handle_register_intent(self, message): @@ -554,3 +560,29 @@ def handle_entity_manifest(self, message): self.bus.emit(message.reply( "intent.service.padatious.entities.manifest", {"entities": self.padatious_service.registered_entities})) + + +def _is_old_style_keyword_message(message): + """Simple check that the message is not using the updated format. + + TODO: Remove in v22.02 + + Args: + message (Message): Message object to check + + Returns: + (bool) True if this is an old messagem, else False + """ + return ('entity_value' not in message.data and 'start' in message.data) + + +def _update_keyword_message(message): + """Make old style keyword registration message compatible. + + Copies old keys in message data to new names. + + Args: + message (Message): Message to update + """ + message.data['entity_value'] = message.data['start'] + message.data['entity_type'] = message.data['end'] diff --git a/mycroft/skills/intent_service_interface.py b/mycroft/skills/intent_service_interface.py index 6fe69aadcf60..89a2f292be93 100644 --- a/mycroft/skills/intent_service_interface.py +++ b/mycroft/skills/intent_service_interface.py @@ -44,15 +44,27 @@ def register_adapt_keyword(self, vocab_type, entity, aliases=None): vocab_type(str): Keyword reference entity (str): Primary keyword - aliases (list): List of alternative kewords + aliases (list): List of alternative keywords """ + # TODO 22.02: Remove compatibility data aliases = aliases or [] - self.bus.emit(Message("register_vocab", - {'start': entity, 'end': vocab_type})) + entity_data = {'entity_value': entity, 'entity_type': vocab_type} + compatibility_data = {'start': entity, 'end': vocab_type} + + self.bus.emit( + Message("register_vocab", + {**entity_data, **compatibility_data}) + ) for alias in aliases: - self.bus.emit(Message("register_vocab", { - 'start': alias, 'end': vocab_type, 'alias_of': entity - })) + alias_data = { + 'entity_value': alias, + 'entity_type': vocab_type, + 'alias_of': entity} + compatibility_data = {'start': alias, 'end': vocab_type} + self.bus.emit( + Message("register_vocab", + {**alias_data, **compatibility_data}) + ) def register_adapt_regex(self, regex): """Register a regex with the intent service. diff --git a/mycroft/skills/intent_services/adapt_service.py b/mycroft/skills/intent_services/adapt_service.py index 9025d2b775a6..a1e46115833d 100644 --- a/mycroft/skills/intent_services/adapt_service.py +++ b/mycroft/skills/intent_services/adapt_service.py @@ -249,14 +249,35 @@ def take_best(intent, utt): ret = None return ret + # TODO 22.02: Remove this deprecated method def register_vocab(self, start_concept, end_concept, alias_of, regex_str): - """Register vocabulary.""" + """Register Vocabulary. DEPRECATED + + This method should not be used, it has been replaced by + register_vocabulary(). + """ + self.register_vocabulary(start_concept, end_concept, + alias_of, regex_str) + + def register_vocabulary(self, entity_value, entity_type, + alias_of, regex_str): + """Register skill vocabulary as adapt entity. + + This will handle both regex registration and registration of normal + keywords. if the "regex_str" argument is set all other arguments will + be ignored. + + Argument: + entity_value: the natural langauge word + entity_type: the type/tag of an entity instance + alias_of: entity this is an alternative for + """ with self.lock: if regex_str: self.engine.register_regex_entity(regex_str) else: self.engine.register_entity( - start_concept, end_concept, alias_of=alias_of) + entity_value, entity_type, alias_of=alias_of) def register_intent(self, intent): """Register new intent with adapt engine. diff --git a/test/unittests/skills/test_intent_service.py b/test/unittests/skills/test_intent_service.py index 2363561f48ee..776c442926f4 100644 --- a/test/unittests/skills/test_intent_service.py +++ b/test/unittests/skills/test_intent_service.py @@ -214,12 +214,18 @@ def test_lang_exists(self): self.assertEqual(_get_message_lang(msg), 'sv-se') -def create_vocab_msg(keyword, value): +def create_old_style_vocab_msg(keyword, value): """Create a message for registering an adapt keyword.""" return Message('register_vocab', {'start': value, 'end': keyword}) +def create_vocab_msg(keyword, value): + """Create a message for registering an adapt keyword.""" + return Message('register_vocab', + {'entity_value': value, 'entity_type': keyword}) + + def get_last_message(bus): """Get last sent message on mock bus.""" last = bus.emit.call_args @@ -230,14 +236,27 @@ class TestIntentServiceApi(TestCase): def setUp(self): self.intent_service = IntentService(mock.Mock()) - def setup_simple_adapt_intent(self): - msg = create_vocab_msg('testKeyword', 'test') + def setup_simple_adapt_intent(self, + msg=create_vocab_msg('testKeyword', 'test')): self.intent_service.handle_register_vocab(msg) intent = IntentBuilder('skill:testIntent').require('testKeyword') msg = Message('register_intent', intent.__dict__) self.intent_service.handle_register_intent(msg) + def test_keyword_backwards_compatibility(self): + self.setup_simple_adapt_intent( + create_old_style_vocab_msg('testKeyword', 'test') + ) + + # Check that the intent is returned + msg = Message('intent.service.adapt.get', data={'utterance': 'test'}) + self.intent_service.handle_get_adapt(msg) + + reply = get_last_message(self.intent_service.bus) + self.assertEqual(reply.data['intent']['intent_type'], + 'skill:testIntent') + def test_get_adapt_intent(self): self.setup_simple_adapt_intent() # Check that the intent is returned @@ -300,8 +319,8 @@ def test_get_adapt_vocab_manifest(self): msg = Message('intent.service.adapt.vocab.manifest.get') self.intent_service.handle_vocab_manifest(msg) reply = get_last_message(self.intent_service.bus) - value = reply.data['vocab'][0]['start'] - keyword = reply.data['vocab'][0]['end'] + value = reply.data['vocab'][0]['entity_value'] + keyword = reply.data['vocab'][0]['entity_type'] self.assertEqual(keyword, 'testKeyword') self.assertEqual(value, 'test') diff --git a/test/unittests/skills/test_intent_service_interface.py b/test/unittests/skills/test_intent_service_interface.py index f5d60c9024b2..0ce739941e7a 100644 --- a/test/unittests/skills/test_intent_service_interface.py +++ b/test/unittests/skills/test_intent_service_interface.py @@ -40,18 +40,40 @@ def setUp(self): def test_register_keyword(self): intent_service = IntentServiceInterface(self.emitter) intent_service.register_adapt_keyword('test_intent', 'test') - self.check_emitter([{'start': 'test', 'end': 'test_intent'}]) + entity_data = {'entity_value': 'test', 'entity_type': 'test_intent'} + compatibility_data = {'start': 'test', 'end': 'test_intent'} + expected_data = {**entity_data, **compatibility_data} + self.check_emitter([expected_data]) def test_register_keyword_with_aliases(self): + # TODO 22.02: Remove compatibility data intent_service = IntentServiceInterface(self.emitter) intent_service.register_adapt_keyword('test_intent', 'test', ['test2', 'test3']) - self.check_emitter([{'start': 'test', 'end': 'test_intent'}, - {'start': 'test2', 'end': 'test_intent', - 'alias_of': 'test'}, - {'start': 'test3', 'end': 'test_intent', - 'alias_of': 'test'}, - ]) + + entity_data = {'entity_value': 'test', 'entity_type': 'test_intent'} + compatibility_data = {'start': 'test', 'end': 'test_intent'} + expected_initial_vocab = {**entity_data, **compatibility_data} + + alias_data = { + 'entity_value': 'test2', + 'entity_type': 'test_intent', + 'alias_of': 'test' + } + alias_compatibility = {'start': 'test2', 'end': 'test_intent'} + expected_alias1 = {**alias_data, **alias_compatibility} + + alias_data2 = { + 'entity_value': 'test3', + 'entity_type': 'test_intent', + 'alias_of': 'test' + } + alias_compatibility2 = {'start': 'test3', 'end': 'test_intent'} + expected_alias2 = {**alias_data2, **alias_compatibility2} + + self.check_emitter([expected_initial_vocab, + expected_alias1, + expected_alias2]) def test_register_regex(self): intent_service = IntentServiceInterface(self.emitter) From 189267b6f66c0569f25a5538051b05369e101d05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=85ke=20Forslund?= Date: Thu, 29 Jul 2021 13:17:56 +0200 Subject: [PATCH 2/3] Minor cleanup of test case for keyword registration --- .../skills/test_intent_service_interface.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/test/unittests/skills/test_intent_service_interface.py b/test/unittests/skills/test_intent_service_interface.py index 0ce739941e7a..0645b86857f2 100644 --- a/test/unittests/skills/test_intent_service_interface.py +++ b/test/unittests/skills/test_intent_service_interface.py @@ -25,13 +25,15 @@ def reset(self): self.results = [] -class FunctionTest(unittest.TestCase): - def check_emitter(self, result_list): - for type in self.emitter.get_types(): - self.assertEqual(type, 'register_vocab') - self.assertEqual(sorted(self.emitter.get_results(), - key=lambda d: sorted(d.items())), - sorted(result_list, key=lambda d: sorted(d.items()))) +class KeywordRegistrationTest(unittest.TestCase): + def check_emitter(self, expected_message_data): + """Verify that the registration messages matches the expected.""" + for msg_type in self.emitter.get_types(): + self.assertEqual(msg_type, 'register_vocab') + self.assertEqual( + sorted(self.emitter.get_results(), + key=lambda d: sorted(d.items())), + sorted(expected_message_data, key=lambda d: sorted(d.items()))) self.emitter.reset() def setUp(self): From f709bb9a1e4c7315be9ffa433aeabfb24c185891 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=85ke=20Forslund?= Date: Mon, 2 Aug 2021 10:52:10 +0200 Subject: [PATCH 3/3] Use IntentServiceInterface in MycroftSkill.register_vocabulary() This moves the message logic for adapt keyword registration into a single location. --- mycroft/skills/mycroft_skill/mycroft_skill.py | 5 ++--- test/unittests/skills/test_mycroft_skill.py | 9 ++++++++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/mycroft/skills/mycroft_skill/mycroft_skill.py b/mycroft/skills/mycroft_skill/mycroft_skill.py index 67bfac529f8a..4f331e27b6a5 100644 --- a/mycroft/skills/mycroft_skill/mycroft_skill.py +++ b/mycroft/skills/mycroft_skill/mycroft_skill.py @@ -1168,9 +1168,8 @@ def register_vocabulary(self, entity, entity_type): entity: word to register entity_type: Intent handler entity to tie the word to """ - self.bus.emit(Message('register_vocab', { - 'start': entity, 'end': to_alnum(self.skill_id) + entity_type - })) + keyword_type = to_alnum(self.skill_id) + entity_type + self.intent_service.register_adapt_keyword(keyword_type, entity) def register_regex(self, regex_str): """Register a new regex. diff --git a/test/unittests/skills/test_mycroft_skill.py b/test/unittests/skills/test_mycroft_skill.py index 0af99abf2899..bc60ca5c90a7 100644 --- a/test/unittests/skills/test_mycroft_skill.py +++ b/test/unittests/skills/test_mycroft_skill.py @@ -303,7 +303,14 @@ def test_register_vocab(self): # Normal vocaubulary self.emitter.reset() - expected = [{'start': 'hello', 'end': 'AHelloKeyword'}] + expected = [ + { + 'start': 'hello', + 'end': 'AHelloKeyword', + 'entity_value': 'hello', + 'entity_type': 'AHelloKeyword' + } + ] s.register_vocabulary('hello', 'HelloKeyword') self.check_register_vocabulary(expected) # Regex