Skills module tests (#220)

* Cleanup deprecated intent_service code
Add intent_service unit tests

* Cleanup dead code in intent_service.py
Update test coverage for NeonIntentService

* Add test coverage for SkillStore and SkillService with updated docs and minor bugfixes
Updated annotation of skills module

* Add test case for skills service

* Update tests to resolve import order errors

* Handle skill directory creation in NeonSkillManager
Rename unit tests for clarity

* Troubleshoot configured skill directories in skill_store.py

* Troubleshooting skills service

* Deprecate skill installation by ID with annotation to fix in OSM

* Troubleshoot OSM parsing errors

* Add branch spec to test URL

Co-authored-by: Daniel McKnight <daniel@neon.ai>

.github/workflows/unit_tests.yml

@@ -1,6 +1,6 @@
 # This workflow will run unit tests
 
-name: Test Utilities
+name: Test Core Modules
 on:
   push:
   workflow_dispatch:
@@ -118,4 +118,13 @@ jobs:
         uses: actions/upload-artifact@v2
         with:
           name: language-test-results
-          path: tests/language-test-results.xml
+          path: tests/language-test-results.xml
+
+      - name: Test Skills Module
+        run: |
+          pytest test/test_skills_module.py --doctest-modules --junitxml=tests/skills-module-test-results.xml
+      - name: Upload Language test results
+        uses: actions/upload-artifact@v2
+        with:
+          name: skills-module-test-results
+          path: tests/skills-module-test-results.xml

neon_core/skills/display_service.py

@@ -28,6 +28,8 @@
 from neon_core.messagebus import get_messagebus
 from os.path import abspath
 
+# TODO: Deprecate this module
+
 
 def ensure_uri(s):
     """

neon_core/skills/intent_service.py

@@ -30,7 +30,6 @@
 from neon_core.language import get_lang_config
 from neon_core.processing_modules.text import TextParsersService
 
-from copy import copy
 from mycroft_bus_client import Message
 from neon_utils.message_utils import get_message_user
 from neon_utils.metrics_utils import Stopwatch
@@ -65,7 +64,7 @@ def __init__(self, bus):
 
         set_default_lang(self.language_config["internal"])
 
-        self._setup_converse_handlers()
+        # self._setup_converse_handlers()
 
         self.parser_service = TextParsersService(self.bus)
         self.parser_service.start()
@@ -75,32 +74,13 @@ def __init__(self, bus):
         else:
             self.transcript_service = None
 
-    def _setup_converse_handlers(self):
-        self.bus.on('skill.converse.error', self.handle_converse_error)
-        self.bus.on('skill.converse.activate_skill',
-                    self.handle_activate_skill)
-        self.bus.on('skill.converse.deactivate_skill',
-                    self.handle_deactivate_skill)
-        # backwards compat
-        self.bus.on('active_skill_request',
-                    self.handle_activate_skill)
-
-    def handle_activate_skill(self, message):
-        self.add_active_skill(message.data['skill_id'])
-
-    def handle_deactivate_skill(self, message):
-        self.remove_active_skill(message.data['skill_id'])
-
-    def reset_converse(self, message):
-        """Let skills know there was a problem with speech recognition"""
-        lang = message.data.get('lang', "en-us")
-        set_default_lang(lang)
-        for skill in copy(self.active_skills):
-            self.do_converse([], skill[0], lang, message)
+    def shutdown(self):
+        self.parser_service.shutdown()
 
     def _save_utterance_transcription(self, message):
         """
-        Record a user utterance with the transcript_service. Adds the `audio_file` context to message context.
+        Record a user utterance with the transcript_service.
+        Adds the `audio_file` context to message context.
 
         Args:
             message (Message): message associated with user input
@@ -111,21 +91,25 @@ def _save_utterance_transcription(self, message):
             if audio:
                 audio = wave.open(audio, 'r')
                 audio = audio.readframes(audio.getnframes())
-            timestamp = message.context["timing"].get("transcribed", time.time())
-            audio_file = self.transcript_service.write_transcript(get_message_user(message),
-                                                                  message.data.get('utterances', [''])[0],
-                                                                  timestamp, audio)
+            timestamp = message.context["timing"].get("transcribed",
+                                                      time.time())
+            audio_file = self.transcript_service.write_transcript(
+                get_message_user(message),
+                message.data.get('utterances', [''])[0], timestamp, audio)
             message.context["audio_file"] = audio_file
 
-    def _get_parsers_service_context(self, message, lang):
+    def _get_parsers_service_context(self, message: Message):
         """
         Pipe utterance thorough text parsers to get more metadata.
         Utterances may be modified by any parser and context overwritten
+        :param message: Message to parse
         """
         utterances = message.data.get('utterances', [])
+        lang = message.data.get('lang')
         for parser in self.parser_service.modules:
             # mutate utterances and retrieve extra data
-            utterances, data = self.parser_service.parse(parser, utterances, lang)
+            utterances, data = self.parser_service.parse(parser, utterances,
+                                                         lang)
             # update message context with extra data
             message.context = merge_dict(message.context, data)
         message.data["utterances"] = utterances
@@ -145,7 +129,8 @@ def handle_utterance(self, message):
         try:
             # Get language of the utterance
             lang = get_full_lang_code(
-                message.data.get('lang', self.language_config["user"]))
+                message.data.get('lang') or self.language_config["user"])
+            message.data["lang"] = lang
 
             # Add or init timing data
             message.context = message.context or {}
@@ -157,59 +142,43 @@ def handle_utterance(self, message):
             # Ensure user profile data is present
             if "user_profiles" not in message.context:
                 message.context["user_profiles"] = [self.default_user.content]
-                message.context["username"] = self.default_user.content["username"]
+                message.context["username"] = \
+                    self.default_user.content["user"]["username"]
 
-            # Make sure there is a `transcribed` timestamp (should have been added in speech module)
+            # Make sure there is a `transcribed` timestamp
             if not message.context["timing"].get("transcribed"):
-                message.context["timing"]["transcribed"] = message.context["timing"]["handle_utterance"]
+                message.context["timing"]["transcribed"] = \
+                    message.context["timing"]["handle_utterance"]
 
             stopwatch = Stopwatch()
 
-            # TODO: Consider saving transcriptions after text parsers cleanup utterance. This should retain the raw
-            #       transcription, in addition to the one modified by the parsers DM
+            # TODO: Consider saving transcriptions after text parsers cleanup
+            #  utterance. This should retain the raw transcription, in addition
+            #  to the one modified by the parsers DM
             # Write out text and audio transcripts if service is available
             with stopwatch:
                 self._save_utterance_transcription(message)
             message.context["timing"]["save_transcript"] = stopwatch.time
 
             # Get text parser context
             with stopwatch:
-                self._get_parsers_service_context(message, lang)
+                self._get_parsers_service_context(message)
             message.context["timing"]["text_parsers"] = stopwatch.time
 
             # Catch empty utterances after parser service
-            if len([u for u in message.data["utterances"] if u.strip()]) == 0:
+            if len([u for u in message.data.get("utterances", [])
+                    if u.strip()]) == 0:
                 LOG.debug("Received empty utterance!!")
-                reply = message.reply('intent_aborted',
-                                      {'utterances': message.data.get('utterances', []),
-                                       'lang': lang})
+                reply = \
+                    message.reply('intent_aborted',
+                                  {'utterances': message.data.get('utterances',
+                                                                  []),
+                                   'lang': lang})
                 self.bus.emit(reply)
                 return
 
-            # now pass our modified message to mycroft-lib
-            message.data["lang"] = lang
-            # TODO: Consider how to implement 'and' parsing and converse here DM
+            # now pass our modified message to Mycroft
+            # TODO: Consider how to implement 'and' parsing and converse DM
             super().handle_utterance(message)
         except Exception as err:
             LOG.exception(err)
-
-    def _converse(self, utterances, lang, message):
-        """
-        Wraps the actual converse method to add timing data
-
-        Args:
-            utterances (list):  list of utterances
-            lang (string):      4 letter ISO language code
-            message (Message):  message to use to generate reply
-
-        Returns:
-            IntentMatch if handled otherwise None.
-        """
-        stopwatch = Stopwatch()
-        with stopwatch:
-            match = super()._converse(utterances, lang, message)
-        message.context["timing"]["check_converse"] = stopwatch.time
-        if match:
-            LOG.info(f"converse handling response: {match.skill_id}")
-        return match
-

neon_core/skills/neon_skill.py

@@ -49,6 +49,8 @@
     AbortQuestion, killable_event
 from mycroft.skills import MycroftSkill
 
+# TODO: Deprecate this module
+
 
 class UserReply(str, Enum):
     YES = "yes"

neon_core/skills/service.py

@@ -24,10 +24,10 @@
 # SOFTWARE,  EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 import time
+from typing import Optional
 
 from neon_utils.configuration_utils import get_neon_skills_config, \
     get_neon_lang_config, get_neon_local_config
-from neon_utils.net_utils import check_online
 from neon_utils import LOG
 from neon_utils.metrics_utils import announce_connection
 from neon_utils.signal_utils import init_signal_handlers, init_signal_bus
@@ -66,9 +66,14 @@ def on_stopping():
 
 
 class NeonSkillService:
-    def __init__(self, alive_hook=on_alive, started_hook=on_started,
-                 ready_hook=on_ready, error_hook=on_error,
-                 stopping_hook=on_stopping, watchdog=None):
+    def __init__(self,
+                 alive_hook: callable = on_alive,
+                 started_hook: callable = on_started,
+                 ready_hook: callable = on_ready,
+                 error_hook: callable = on_error,
+                 stopping_hook: callable = on_stopping,
+                 watchdog: Optional[callable] = None,
+                 config: Optional[dict] = None):
         self.bus = None
         self.skill_manager = None
         self.event_scheduler = None
@@ -79,10 +84,10 @@ def __init__(self, alive_hook=on_alive, started_hook=on_started,
                                            on_ready=ready_hook,
                                            on_error=error_hook,
                                            on_stopping=stopping_hook)
-        skill_config = get_neon_skills_config()
-        if skill_config.get("run_gui_file_server"):
+        self.config = config or get_neon_skills_config()
+        if self.config.get("run_gui_file_server"):
             self.http_server = start_qml_http_server(
-                skill_config["directory"])
+                self.config["directory"])
         else:
             self.http_server = None
 
@@ -100,13 +105,12 @@ def start(self):
         self.event_scheduler = EventScheduler(self.bus)
         self.status = ProcessStatus('skills', self.bus, self.callbacks)
         SkillApi.connect_bus(self.bus)
-        self.skill_manager = NeonSkillManager(self.bus, self.watchdog)
-        self.status.set_started()
-        # self._wait_for_internet_connection()
-        # # TODO can this be removed? its a hack around msm requiring internet...
-        # if self.skill_manager is None:
-        #     self._initialize_skill_manager()
+        self.skill_manager = NeonSkillManager(self.bus, self.watchdog,
+                                              config=self.config)
         self.skill_manager.start()
+        self.status.set_started()
+
+        # TODO: These should be event-based in Mycroft/OVOS
         while not self.skill_manager.is_alive():
             time.sleep(0.1)
         self.status.set_alive()
@@ -156,12 +160,12 @@ def _register_intent_services(self):
     #     LOG.info(f"Blacklisted={self.skill_manager.config['skills']['blacklisted_skills']}")
     #     # self.skill_manager.load_priority()
 
-    def _wait_for_internet_connection(self):
-        if get_neon_skills_config().get("wait_for_internet", True):
-            while not check_online():
-                time.sleep(1)
-        else:
-            LOG.info("Online check disabled, device may be offline")
+    # def _wait_for_internet_connection(self):
+    #     if get_neon_skills_config().get("wait_for_internet", True):
+    #         while not check_online():
+    #             time.sleep(1)
+    #     else:
+    #         LOG.info("Online check disabled, device may be offline")
 
     def shutdown(self):
         LOG.info('Shutting down Skills service')

neon_core/skills/skill_manager.py

@@ -23,6 +23,9 @@
 # NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
 # SOFTWARE,  EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
+from os import makedirs
+from os.path import isdir, expanduser
+
 from neon_utils.configuration_utils import get_neon_skills_config
 from neon_utils.log_utils import LOG
 
@@ -32,15 +35,24 @@
 from mycroft.skills.skill_manager import SkillManager
 
 SKILL_MAIN_MODULE = '__init__.py'
+# TODO: deprecate `SKILL_MAIN_MODULE`?
 
 
 class NeonSkillManager(SkillManager):
 
     def __init__(self, *args, **kwargs):
+        config = kwargs.pop("config") if "config" in kwargs else \
+            get_neon_skills_config()
+        config["directory"] = expanduser(config["directory"])
         super().__init__(*args, **kwargs)
-        self.skill_config = kwargs.get("config") or get_neon_skills_config()
-        self.skill_downloader = SkillsStore(skills_dir=self.skill_config["directory"], config=self.skill_config,
-                                            bus=self.bus)
+        self.skill_config = config
+        if not isdir(self.skill_config["directory"]):
+            LOG.warning("Creating requested skill directory")
+            makedirs(self.skill_config["directory"])
+
+        self.skill_downloader = SkillsStore(
+            skills_dir=self.skill_config["directory"],
+            config=self.skill_config, bus=self.bus)
         self.skill_downloader.skills_dir = self.skill_config["directory"]
 
     def download_or_update_defaults(self):
@@ -62,12 +74,3 @@ def run(self):
         """Load skills and update periodically from disk and internet."""
         self.download_or_update_defaults()
         super().run()
-
-    def _emit_converse_error(self, message, skill_id, error_msg):
-        if hasattr(super(), "_emit_converse_error"):
-            super()._emit_converse_error(message, skill_id, error_msg)
-        # Also emit the old error message to keep compatibility and for any
-        # listener on the bus
-        reply = message.reply('skill.converse.error',
-                              data=dict(skill_id=skill_id, error=error_msg))
-        self.bus.emit(reply)