diff --git a/mycroft/configuration/mycroft.conf b/mycroft/configuration/mycroft.conf index 3a79613351fc..ee4491aedc99 100644 --- a/mycroft/configuration/mycroft.conf +++ b/mycroft/configuration/mycroft.conf @@ -109,7 +109,9 @@ # audio -> audio playback reported ready # gui -> gui websocket reported ready - NOT IMPLEMENTED # enclosure -> enclosure/HAL reported ready - NOT IMPLEMENTED - "ready_settings": ["skills"], + # network_skills -> skills with network requirements + # internet_skills -> skills with internet requirements + "ready_settings": ["skills", "network_skills", "internet_skills"], // General skill values "skills": { diff --git a/mycroft/skills/skill_loader.py b/mycroft/skills/skill_loader.py index b1ac246c4939..8e5306925873 100644 --- a/mycroft/skills/skill_loader.py +++ b/mycroft/skills/skill_loader.py @@ -24,7 +24,7 @@ from ovos_config.locations import get_xdg_data_dirs, get_xdg_data_save_path from ovos_config.meta import get_xdg_base from ovos_plugin_manager.skills import find_skill_plugins - +from ovos_workshop.skills.base import SkillNetworkRequirements, BaseSkill from ovos_config.config import Configuration from mycroft.messagebus import Message from mycroft.skills.mycroft_skill.mycroft_skill import MycroftSkill @@ -298,20 +298,75 @@ def get_create_skill_function(skill_module): class SkillLoader: - def __init__(self, bus, skill_directory): + def __init__(self, bus, skill_directory=None): self.bus = bus - self.skill_directory = skill_directory - self.skill_id = os.path.basename(skill_directory) + self._skill_directory = skill_directory + self._skill_id = None + self._skill_class = None + self._loaded = None self.load_attempted = False - self.loaded = False self.last_modified = 0 self.last_loaded = 0 - self.instance = None + self.instance: BaseSkill = None self.active = True self._watchdog = None self.config = Configuration() self.modtime_error_log_written = False + self.skill_module = None + + @property + def loaded(self): + return self._loaded # or self.instance is None + + @loaded.setter + def loaded(self, val): + self._loaded = val + + @property + def skill_directory(self): + skill_dir = self._skill_directory + if self.instance and not skill_dir: + skill_dir = self.instance.root_dir + return skill_dir + + @skill_directory.setter + def skill_directory(self, val): + self._skill_directory = val + + @property + def skill_id(self): + skill_id = self._skill_id + if self.instance and not skill_id: + LOG.debug(f"skill_id from instance") + skill_id = self.instance.skill_id + if self.skill_directory and not skill_id: + LOG.debug(f"skill_id from directory") + skill_id = os.path.basename(self.skill_directory) + return skill_id + + @skill_id.setter + def skill_id(self, val): + self._skill_id = val + + @property + def skill_class(self): + skill_class = self._skill_class + if self.instance and not skill_class: + skill_class = self.instance.__class__ + if self.skill_module and not skill_class: + skill_class = get_skill_class(self.skill_module) + return skill_class + + @skill_class.setter + def skill_class(self, val): + self._skill_class = val + + @property + def network_requirements(self): + if not self.skill_class: + return SkillNetworkRequirements() + return self.skill_class.network_requirements @property def is_blacklisted(self): @@ -334,7 +389,7 @@ def reload_needed(self): return self.instance is None def reload(self): - LOG.info('ATTEMPTING TO RELOAD SKILL: ' + self.skill_id) + LOG.info(f'ATTEMPTING TO RELOAD SKILL: {self.skill_id}') if self.instance: if not self.instance.reload_skill: LOG.info("skill does not allow reloading!") @@ -343,7 +398,7 @@ def reload(self): return self._load() def load(self): - LOG.info('ATTEMPTING TO LOAD SKILL: ' + self.skill_id) + LOG.info(f'ATTEMPTING TO LOAD SKILL: {self.skill_id}') return self._load() def _unload(self): @@ -355,13 +410,11 @@ def _unload(self): self._execute_instance_shutdown() if self.config.get("debug", False): self._garbage_collect() - self.loaded = False self._emit_skill_shutdown_event() def unload(self): if self.instance: self._execute_instance_shutdown() - self.loaded = False def activate(self): self.active = True @@ -379,6 +432,8 @@ def _execute_instance_shutdown(self): LOG.exception(f'An error occurred while shutting down {self.skill_id}') else: LOG.info(f'Skill {self.skill_id} shut down successfully') + del self.instance + self.instance = None def _garbage_collect(self): """Invoke Python garbage collector to remove false references""" @@ -401,9 +456,8 @@ def _load(self): if self.is_blacklisted: self._skip_load() else: - skill_module = self._load_skill_source() - if skill_module and self._create_skill_instance(skill_module): - self.loaded = True + self.skill_module = self._load_skill_source() + self.loaded = self._create_skill_instance() self.last_loaded = time() self._communicate_load_status() @@ -441,7 +495,6 @@ def _handle_filechange(self): def _prepare_for_load(self): self.load_attempted = True - self.loaded = False self.instance = None def _skip_load(self): @@ -460,7 +513,7 @@ def _load_skill_source(self): LOG.exception(f'Failed to load skill: {self.skill_id} ({e})') return skill_module - def _create_skill_instance(self, skill_module): + def _create_skill_instance(self, skill_module=None): """create the skill object. Arguments: @@ -469,9 +522,10 @@ def _create_skill_instance(self, skill_module): Returns: (bool): True if skill was loaded successfully. """ + skill_module = skill_module or self.skill_module try: skill_creator = get_create_skill_function(skill_module) or \ - get_skill_class(skill_module) + self.skill_class # create the skill self.instance = skill_creator() @@ -512,26 +566,20 @@ def _communicate_load_status(self): class PluginSkillLoader(SkillLoader): def __init__(self, bus, skill_id): - super().__init__(bus, skill_id) - self.skill_directory = skill_id - self.skill_id = skill_id + super().__init__(bus) + self._skill_id = skill_id def reload_needed(self): return False - def _create_skill_instance(self, skill_module): - if super()._create_skill_instance(skill_module): - self.skill_directory = self.instance.root_dir - return True - return False - - def load(self, skill_module): + def load(self, skill_class): LOG.info('ATTEMPTING TO LOAD PLUGIN SKILL: ' + self.skill_id) + self._skill_class = skill_class self._prepare_for_load() if self.is_blacklisted: self._skip_load() else: - self.loaded = self._create_skill_instance(skill_module) + self.loaded = self._create_skill_instance() self.last_loaded = time() self._communicate_load_status() diff --git a/mycroft/skills/skill_manager.py b/mycroft/skills/skill_manager.py index b037500e7dc5..c81da606c6ce 100644 --- a/mycroft/skills/skill_manager.py +++ b/mycroft/skills/skill_manager.py @@ -103,6 +103,10 @@ def __init__(self, bus, watchdog=None, alive_hook=on_alive, started_hook=on_star self._setup_event = Event() self._stop_event = Event() self._connected_event = Event() + self._network_event = Event() + self._network_loaded = Event() + self._internet_loaded = Event() + self._network_skill_timeout = 300 self.config = Configuration() self.manifest_uploader = SeleneSkillManifestUploader() self.upload_queue = UploadQueue() # DEPRECATED @@ -119,14 +123,16 @@ def __init__(self, bus, watchdog=None, alive_hook=on_alive, started_hook=on_star self.status.bind(self.bus) + # If PHAL loaded first, make sure we get network state + resp = self.bus.wait_for_response(Message("ovos.PHAL.internet_check")) + if resp: + if resp.data.get('internet_connected'): + self.handle_internet_connected(resp) + elif resp.data.get('network_connected'): + self.handle_network_connected(resp) + def _define_message_bus_events(self): """Define message bus events with handlers defined in this class.""" - # Update on initial connection - self.bus.once( - 'mycroft.internet.connected', - lambda x: self._connected_event.set() - ) - # Update upon request self.bus.on('skillmanager.list', self.send_skill_list) self.bus.on('skillmanager.deactivate', self.deactivate_skill) @@ -136,6 +142,10 @@ def _define_message_bus_events(self): self.handle_check_device_readiness) self.bus.once('mycroft.skills.trained', self.handle_initial_training) + # load skills waiting for connectivity + self.bus.on("mycroft.network.connected", self.handle_network_connected) + self.bus.on("mycroft.internet.connected", self.handle_internet_connected) + def is_device_ready(self): is_ready = False # different setups will have different needs @@ -197,9 +207,11 @@ def setup_finish_interrupt(message): # in offline mode (default) is_paired always returns True # but setup skill may enable backend # wait for backend selection event - response = self.bus.wait_for_response(Message('ovos.setup.state.get', - context={"source": "skills", - "destination": "ovos-setup"}), 'ovos.setup.state') + response = self.bus.wait_for_response( + Message('ovos.setup.state.get', + context={"source": "skills", + "destination": "ovos-setup"}), + 'ovos.setup.state') if response: state = response.data['state'] LOG.debug(f"Setup state: {state}") @@ -212,6 +224,10 @@ def setup_finish_interrupt(message): # not implemented services[ser] = True continue + elif ser in ["network_skills", "internet_skills"]: + # Implemented in skill manager + services[ser] = True + continue response = self.bus.wait_for_response( Message(f'mycroft.{ser}.is_ready', context={"source": "skills", "destination": ser})) @@ -262,15 +278,60 @@ def handle_paired(self, _): upload of settings is done at individual skill level in ovos-core """ pass - def load_plugin_skills(self): + def handle_internet_connected(self, message): + LOG.debug("Internet Connected") + self._network_event.set() + self._connected_event.set() + self._load_on_internet() + + # Sync backend and skills. + # why does selene need to know about skills without settings? + if is_paired(): + self.manifest_uploader.post_manifest() + + def handle_network_connected(self, message): + LOG.debug("Network Connected") + self._network_event.set() + self._load_on_network() + + def load_plugin_skills(self, network=None, internet=None): + if network is None: + network = self._network_event.is_set() + if internet is None: + internet = self._connected_event.is_set() plugins = find_skill_plugins() loaded_skill_ids = [basename(p) for p in self.skill_loaders] for skill_id, plug in plugins.items(): if skill_id not in self.plugin_skills and skill_id not in loaded_skill_ids: + skill_loader = self._get_plugin_skill_loader(skill_id, init_bus=False) + requirements = skill_loader.network_requirements + if not network and requirements.network_before_load: + continue + if not internet and requirements.internet_before_load: + continue self._load_plugin_skill(skill_id, plug) + def _get_internal_skill_bus(self): + if not self.config["websocket"].get("shared_connection", True): + # see BusBricker skill to understand why this matters + # any skill can manipulate the bus from other skills + # this patch ensures each skill gets it's own + # connection that can't be manipulated by others + # https://github.com/EvilJarbas/BusBrickerSkill + bus = MessageBusClient(cache=True) + bus.run_in_thread() + else: + bus = self.bus + return bus + + def _get_plugin_skill_loader(self, skill_id, init_bus=True): + bus = None + if init_bus: + bus = self._get_internal_skill_bus() + return PluginSkillLoader(bus, skill_id) + def _load_plugin_skill(self, skill_id, skill_plugin): - skill_loader = PluginSkillLoader(self.bus, skill_id) + skill_loader = self._get_plugin_skill_loader(skill_id) try: load_status = skill_loader.load(skill_plugin) except Exception: @@ -303,23 +364,45 @@ def run(self): self.status.set_alive() - if self.skills_config.get("wait_for_internet", True): - while not connected() and not self._connected_event.is_set(): - sleep(1) - self._connected_event.set() - self._load_on_startup() - # Sync backend and skills. - # why does selene need to know about skills without settings? - if is_paired(): - self.manifest_uploader.post_manifest() + if self.skills_config.get("wait_for_internet", False): + LOG.warning("`wait_for_internet` is a deprecated option, update to " + "specify `network_skills` and `internet_skills` in " + "`ready_settings`") + # NOTE - self._connected_event will never be set + # if PHAL plugin is not running to emit the connected events + while not self._connected_event.is_set(): + sleep(1) + LOG.debug("Internet Connected") + if "network_skills" in self.config.get("ready_settings"): + self._connected_event.wait() # Wait for user to connect to network + if self._network_loaded.wait(self._network_skill_timeout): + LOG.debug("Network skills loaded") + else: + LOG.error("Gave up waiting for network skills to load") + if "internet_skills" in self.config.get("ready_settings"): + self._connected_event.wait() # Wait for user to connect to network + if self._internet_loaded.wait(self._network_skill_timeout): + LOG.debug("Internet skills loaded") + else: + LOG.error("Gave up waiting for internet skills to load") + if not all((self._network_loaded.is_set(), + self._internet_loaded.is_set())): + self.bus.emit(Message( + 'mycroft.skills.error', + {'internet_loaded': self._internet_loaded.is_set(), + 'network_loaded': self._network_loaded.is_set()})) + self.bus.emit(Message('mycroft.skills.initialized')) # wait for initial intents training + LOG.debug("Waiting for initial training") while not self.initial_load_complete: sleep(0.5) self.status.set_ready() + LOG.info("Skills all loaded!") + # Scan the file folder that contains Skills. If a Skill is updated, # unload the existing version from memory and reload from the disk. while not self._stop_event.is_set(): @@ -342,20 +425,40 @@ def _remove_git_locks(self): LOG.warning('Found and removed git lock file: ' + i) os.remove(i) + def _load_on_network(self): + LOG.info('Loading network skills...') + self._load_new_skills(network=True, internet=False) + self._network_loaded.set() + + def _load_on_internet(self): + LOG.info('Loading internet skills...') + self._load_new_skills(network=True, internet=True) + self._internet_loaded.set() + def _load_on_startup(self): """Handle initial skill load.""" - self.load_plugin_skills() - LOG.info('Loading installed skills...') - self._load_new_skills() - LOG.info("Skills all loaded!") - self.bus.emit(Message('mycroft.skills.initialized')) + LOG.info('Loading offline skills...') + self._load_new_skills(network=False, internet=False) - def _load_new_skills(self): + def _load_new_skills(self, network=None, internet=None): """Handle load of skills installed since startup.""" + if network is None: + network = self._network_event.is_set() + if internet is None: + internet = self._connected_event.is_set() + + self.load_plugin_skills(network=network, internet=internet) + for skill_dir in self._get_skill_directories(): replaced_skills = [] # by definition skill_id == folder name skill_id = os.path.basename(skill_dir) + skill_loader = self._get_skill_loader(skill_dir, init_bus=False) + requirements = skill_loader.network_requirements + if not network and requirements.network_before_load: + continue + if not internet and requirements.internet_before_load: + continue # a local source install is replacing this plugin, unload it! if skill_id in self.plugin_skills: @@ -375,18 +478,14 @@ def _load_new_skills(self): if skill_dir not in self.skill_loaders: self._load_skill(skill_dir) + def _get_skill_loader(self, skill_directory, init_bus=True): + bus = None + if init_bus: + bus = self._get_internal_skill_bus() + return SkillLoader(bus, skill_directory) + def _load_skill(self, skill_directory): - if not self.config["websocket"].get("shared_connection", True): - # see BusBricker skill to understand why this matters - # any skill can manipulate the bus from other skills - # this patch ensures each skill gets it's own - # connection that can't be manipulated by others - # https://github.com/EvilJarbas/BusBrickerSkill - bus = MessageBusClient(cache=True) - bus.run_in_thread() - else: - bus = self.bus - skill_loader = SkillLoader(bus, skill_directory) + skill_loader = self._get_skill_loader(skill_directory) try: load_status = skill_loader.load() except Exception: diff --git a/requirements/extra-skills.txt b/requirements/extra-skills.txt index c929add33ed9..63192658ce10 100644 --- a/requirements/extra-skills.txt +++ b/requirements/extra-skills.txt @@ -2,4 +2,5 @@ adapt-parser~=0.5 padacioso~=0.1.2 ovos-lingua-franca~=0.4, >=0.4.2 PyYAML~=5.4 -ovos_workshop~=0.0, >=0.0.10a4 \ No newline at end of file +ovos_workshop~=0.0.10 +ovos-phal-plugin-connectivity-events~=0.0.1 \ No newline at end of file diff --git a/test/unittests/mocks.py b/test/unittests/mocks.py index 6b3ff1f95abe..1fcac062b33f 100644 --- a/test/unittests/mocks.py +++ b/test/unittests/mocks.py @@ -74,3 +74,6 @@ def on(self, event, _): def once(self, event, _): self.event_handlers.append(event) + + def wait_for_response(self, message): + self.emit(message) diff --git a/test/unittests/skills/test_skill_loader.py b/test/unittests/skills/test_skill_loader.py index 509ce8355b1e..4133fe15ca58 100644 --- a/test/unittests/skills/test_skill_loader.py +++ b/test/unittests/skills/test_skill_loader.py @@ -13,15 +13,66 @@ # limitations under the License. # """Unit tests for the SkillLoader class.""" +import unittest from time import time -from unittest.mock import call, MagicMock, Mock, patch +from unittest.mock import call, Mock, patch +from mycroft.skills.mycroft_skill.mycroft_skill import MycroftSkill from mycroft.skills.skill_loader import _get_last_modified_time, SkillLoader +from ovos_workshop.decorators import classproperty +from ovos_workshop.skills.base import SkillNetworkRequirements from ..base import MycroftUnitTestBase ONE_MINUTE = 60 +class OfflineSkill(MycroftSkill): + @classproperty + def network_requirements(self): + return SkillNetworkRequirements(internet_before_load=False, + network_before_load=False, + requires_internet=False, + requires_network=False, + no_internet_fallback=True, + no_network_fallback=True) + + +class LANSkill(MycroftSkill): + @classproperty + def network_requirements(self): + scans_on_init = True + return SkillNetworkRequirements(internet_before_load=False, + network_before_load=scans_on_init, + requires_internet=False, + requires_network=True, + no_internet_fallback=True, + no_network_fallback=False) + + +class TestSkillNetwork(unittest.TestCase): + + def test_class_property(self): + self.assertEqual(OfflineSkill.network_requirements, + SkillNetworkRequirements(internet_before_load=False, + network_before_load=False, + requires_internet=False, + requires_network=False, + no_internet_fallback=True, + no_network_fallback=True) + ) + self.assertEqual(LANSkill.network_requirements, + SkillNetworkRequirements(internet_before_load=False, + network_before_load=True, + requires_internet=False, + requires_network=True, + no_internet_fallback=True, + no_network_fallback=False) + ) + self.assertEqual(MycroftSkill.network_requirements, + SkillNetworkRequirements() + ) + + class TestSkillLoader(MycroftUnitTestBase): mock_package = 'mycroft.skills.skill_loader.' @@ -52,6 +103,7 @@ def _mock_skill_instance(self): """Mock the skill instance, we are not testing skill functionality.""" skill_instance = Mock() skill_instance.name = 'test_skill' + skill_instance.skill_id = None skill_instance.reload_skill = True skill_instance.default_shutdown = Mock() self.skill_instance_mock = skill_instance @@ -94,6 +146,7 @@ def test_skill_reload(self): self.loader.instance = Mock() self.loader.loaded = True self.loader.last_loaded = 0 + self.loader.skill_id = 'test_skill' with patch(self.mock_package + 'time') as time_mock: time_mock.return_value = 100 @@ -116,10 +169,19 @@ def test_skill_reload(self): for log in log_messages))) def test_skill_load(self): - with patch(self.mock_package + 'time') as time_mock: + # Mock to return a known (Mock) skill instance + real_create_skill_instance = self.loader._create_skill_instance + + def _update_skill_instance(*args, **kwargs): + self.loader.instance = self.skill_instance_mock + return True + + self.loader._create_skill_instance = _update_skill_instance + + with patch(self.mock_package + 'time') as time_mock: # mycroft.skills.skill_loader. time_mock.return_value = 100 with patch(self.mock_package + 'SettingsMetaUploader'): - self.loader.load() + self.loader.load() # Correct skill ID self.assertTrue(self.loader.load_attempted) self.assertTrue(self.loader.loaded) @@ -135,6 +197,8 @@ def test_skill_load(self): self.assertTrue(all((log in self.log_mock.method_calls for log in log_messages))) + self.loader._create_skill_instance = real_create_skill_instance + def test_reload_modified(self): self.loader.last_modified = 0 self.loader.reload = Mock() @@ -145,7 +209,12 @@ def test_reload_modified(self): def test_skill_load_blacklisted(self): """Skill should not be loaded if it is blacklisted""" - self.loader.config['skills']['blacklisted_skills'] = ['test_skill'] + config = dict(self.loader.config) + config['skills']['blacklisted_skills'] = ['test_skill'] + self.loader.config = config + self.assertEqual(self.loader.config['skills']['blacklisted_skills'], + ['test_skill']) + self.loader.skill_id = 'test_skill' with patch(self.mock_package + 'SettingsMetaUploader'): self.loader.load() diff --git a/test/unittests/skills/test_skill_manager.py b/test/unittests/skills/test_skill_manager.py index 065b9332c9cf..7e1bc2324064 100644 --- a/test/unittests/skills/test_skill_manager.py +++ b/test/unittests/skills/test_skill_manager.py @@ -97,23 +97,21 @@ def _mock_skill_loader_instance(self): def test_instantiate(self): expected_result = [ - 'mycroft.internet.connected', 'skillmanager.list', 'skillmanager.deactivate', 'skillmanager.keep', 'skillmanager.activate', - # 'mycroft.paired', - # 'mycroft.skills.settings.update', 'mycroft.skills.initialized', 'mycroft.skills.trained', + 'mycroft.network.connected', + 'mycroft.internet.connected', 'mycroft.skills.is_alive', 'mycroft.skills.is_ready', 'mycroft.skills.all_loaded' ] - self.assertListEqual( - expected_result, - self.message_bus_mock.event_handlers - ) + + self.assertListEqual(expected_result, + self.message_bus_mock.event_handlers) def test_unload_removed_skills(self): self.skill_manager._unload_removed_skills() @@ -127,10 +125,11 @@ def test_send_skill_list(self): self.skill_manager.send_skill_list(None) self.assertListEqual( - ['mycroft.skills.list'], + ['ovos.PHAL.internet_check', + 'mycroft.skills.list'], self.message_bus_mock.message_types ) - message_data = self.message_bus_mock.message_data[0] + message_data = self.message_bus_mock.message_data[-1] self.assertIn('test_skill', message_data.keys()) skill_data = message_data['test_skill'] self.assertDictEqual(dict(active=True, id='test_skill'), skill_data)