Skip to content
This repository was archived by the owner on Sep 8, 2024. It is now read-only.

Skill states #2901

Closed
ken-mycroft wants to merge 1 commit into from
Closed

Skill states #2901

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions mycroft/audio/services/simple/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ def play(self, repeat=False):

def stop(self):
LOG.info('SimpleAudioServiceStop')
self.bus.emit(Message('SimpleAudioServiceStop', {'data': None}))
Copy link
Contributor

@krisgesling krisgesling May 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this meant to be in this PR?

If we want to add messages to each audio service then I think we need to namespace them appropriately

Copy link
Contributor Author

@ken-mycroft ken-mycroft May 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm tired from haggling. This system has been in this state for many years now. If someone had a better idea they should have fixed it by now. I can appreciate the attitude some of our community members and why they ultimately decide to fork. While there are many different ways these issues could be addressed I am not interested in altering this PR to work around any. Your ideas sound fine, please squash this PR get your ideas working and add me to the review list after you submit your new PR with the working code.

Copy link
Collaborator

@forslund forslund May 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Ken, as a Swede I don't generally interfere (neutrality and all that), I respect you and I'm sure this came off more aggressive than you intended but this comes of rather rude and I must speak out since I don't think Kris deserves this bite.

(This thread in particular is valid since it's a query about something that's not mentioned in the PR or comment at all.)

The general discussion about the approach is probably the more important thing, we need the open discussion to make better designs. That is one of the strengths of open source, sharing and building upon each other's ideas. Instead of biting back, answer his concerns and make your thoughts on the issue clear to the rest of us. I'm sure you've thought it through. I think the skill state may be an interesting feature, this is the initial commit and will need to be streamlined in someway to fit into the system but will likely be very convenient.

Will now go back to the Swedish way of "making a fist in my pocket", and be silent.

P.S. I think the general frustration in the community is when no response is given, not when response is received. D.S.

Copy link
Contributor Author

@ken-mycroft ken-mycroft May 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My message was simply if we have a better approach why have we waited so long to implement it? I have already put the effort into solving this problem, however, I am always open to alternative solutions and look forward to reviewing any contributions which solve the problem in a different manner. There is, in this case, about a 40 hour difference between a good idea and a working solution. I have other concerns regarding how we handle contributions from the community in general but that is a different topic.

Copy link
Contributor Author

@ken-mycroft ken-mycroft May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the active skills array we don't store intents, we store skill tuples (skill_id, timestamp, category). One thing I should explain is I have not verified if the stop intent handlers are still being called, I was just surprised they showed up in the intent results at all but maybe that is by design and they are not actually getting called.

Copy link
Collaborator

@forslund forslund May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the active_skills are only used for converse calls. where active skills may short-circuit the intent service. The active skills list will not in any way block the normal intent handling through adapt, padatious and fallback services unless you changed the behavior (I don't see that here, but may be wrong).

Copy link
Contributor Author

@ken-mycroft ken-mycroft May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if I disable an intent I would not expect to see its handler fire; converse or otherwise. Is that true or am I missing something?

Copy link
Collaborator

@forslund forslund May 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will not fire if it's disabled, see my example below. In that case nothing is firing for the OtherAdapt and the "unknown" skill is triggered instead, as expected.

Edit: Converse is a separate system so that may fire disable_intent() will only disable single intent (adapt or padatious), which would not make a difference to the converse

self._stop_signal = True
while self._is_playing:
sleep(0.1)
Expand Down
3 changes: 2 additions & 1 deletion mycroft/skills/common_play_skill.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,8 @@ def __handle_play_start(self, message):
# "... on the chromecast"
self.play_service_string = phrase

self.bus.emit(Message('active_skill_request', {'skill_id': self.skill_id}))
self.bus.emit(Message('active_skill_request',
{'skill_id': self.skill_id, 'skill_cat':self.category}))

# Invoke derived class to provide playback data
self.CPS_start(phrase, data)
Expand Down
51 changes: 45 additions & 6 deletions mycroft/skills/intent_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
AdaptService, AdaptIntent, FallbackService, PadatiousService, IntentMatch
)
from .intent_service_interface import open_intent_envelope
from mycroft.messagebus.message import Message
import json


def _get_message_lang(message):
Expand Down Expand Up @@ -82,6 +84,7 @@ def __init__(self, bus):
self.bus = bus

self.skill_names = {}
self.skill_categories = {}
config = Configuration.get()
self.adapt_service = AdaptService(config.get('context', {}))
try:
Expand All @@ -104,12 +107,24 @@ def __init__(self, bus):
# 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('mycroft.skills.list', self.update_skill_list)


def add_active_skill_handler(message):
self.add_active_skill(message.data['skill_id'])
category = 'undefined'
if message.data.get("skill_cat", None) is not None:
category = message.data['skill_cat']

self.add_active_skill(message.data['skill_id'], category)

def remove_active_skill_handler(message):
self.remove_active_skill(message.data['skill_id'])

self.bus.on('active_skill_request', add_active_skill_handler)
self.bus.on('deactivate_skill_request', remove_active_skill_handler)
self.active_skills = [] # [skill_id , timestamp]

# BUG fix this!
self.converse_timeout = 5 # minutes to prune active_skills

# Intents API
Expand Down Expand Up @@ -138,6 +153,11 @@ def registered_intents(self):
def update_skill_name_dict(self, message):
"""Messagebus handler, updates dict of id to skill name conversions."""
self.skill_names[message.data['id']] = message.data['name']
self.bus.emit(Message('skillmanager.list'))

def update_skill_list(self, message):
for skill in message.data:
self.skill_categories[message.data[skill]['id']] = message.data[skill]['cat']

def get_skill_name(self, skill_id):
"""Get skill name from skill ID.
Expand Down Expand Up @@ -167,6 +187,7 @@ def do_converse(self, utterances, skill_id, lang, message):
lang (str): current language
message (Message): message containing interaction info.
"""
LOG.debug("do_converse()-utterances:%s, active_skills=%s" % (utterances, self.active_skills))
converse_msg = (message.reply("skill.converse.request", {
"skill_id": skill_id, "utterances": utterances, "lang": lang}))
result = self.bus.wait_for_response(converse_msg,
Expand Down Expand Up @@ -198,11 +219,11 @@ def remove_active_skill(self, skill_id):
Arguments:
skill_id (str): skill to remove
"""
for skill in self.active_skills:
for skill in copy(self.active_skills):
if skill[0] == skill_id:
self.active_skills.remove(skill)

def add_active_skill(self, skill_id):
def add_active_skill(self, skill_id, category='undefined'):
"""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
Expand All @@ -216,11 +237,13 @@ def add_active_skill(self, skill_id):
if skill_id != '':
self.remove_active_skill(skill_id)
# add skill with timestamp to start of skill_list
self.active_skills.insert(0, [skill_id, time.time()])
self.active_skills.insert(0, [skill_id, time.time(), category])
else:
LOG.warning('Skill ID was empty, won\'t add to list of '
'active skills.')

LOG.debug("Exit add active skill_id:%s, active_skills=%s" % (skill_id, self.active_skills))

def send_metrics(self, intent, context, stopwatch):
"""Send timing metrics to the backend.

Expand Down Expand Up @@ -272,6 +295,7 @@ def handle_utterance(self, message):
Arguments:
message (Message): The messagebus data
"""
LOG.debug("Enter handle utterance: message.data:%s, active_skills:%s" % (message.data, self.active_skills))
try:
lang = _get_message_lang(message)
set_active_lang(lang)
Expand All @@ -297,9 +321,13 @@ def handle_utterance(self, message):
match = match_func(combined, lang, message)
if match:
break

if match:
if match.skill_id:
self.add_active_skill(match.skill_id)
if self.skill_categories[match.skill_id] == 'undefined':
# note - new style will activate themselves
LOG.warning("adding old style skill to active_skills array")
self.add_active_skill(match.skill_id, self.skill_categories[match.skill_id])
# If the service didn't report back the skill_id it
# takes on the responsibility of making the skill "active"

Expand Down Expand Up @@ -333,12 +361,23 @@ def _converse(self, utterances, lang, message):
if time.time() - skill[
1] <= self.converse_timeout * 60]


# first check for system levl skills
tmp_active_skills = copy(self.active_skills)
for skill in tmp_active_skills:
if skill[2] == 'system':
Copy link
Contributor

@krisgesling krisgesling May 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the primary reason for having a category at the moment - to give system Skills priority access to converse?

Any Skill could then list themselves as a "system" Skill to gain the same priority access.

It looks like the active_skill list inserts any new item to the start of the list, so theoretically the most recently active Skill gets the first attempt at the utterance. If that's not working I'd be curious to know why but we could also the sort tmp_active_skills based on the time.time in skill[1].

So if we have a way to hit the mostly recently active Skill first, for the Alarm Skill could we reactivate it ensuring it's at the top of the active list each time the beep is going to get played?

if self.do_converse(utterances, skill[0], lang, message):
# update timestamp, or there will be a timeout where
# intent stops conversing whether its being used or not
return IntentMatch('Converse', None, None, skill[0])

# check if any skill wants to handle utterance
for skill in copy(self.active_skills):
for skill in tmp_active_skills:
Copy link
Collaborator

@forslund forslund May 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this exclude the entries where skill[2] == system? seems like those gets called twice now...

Copy link
Contributor Author

@ken-mycroft ken-mycroft May 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the above loop would cause us to exit the method on a match, are you saying if line 372 gets executed " return IntentMatch('Converse', None, None, skill[0])" we would still fall thru to the below loop?

Copy link
Collaborator

@forslund forslund May 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be reading the code wrong but I mean if none of the active_skills with the system category has a match (do_converse() returns False for all of them)

We would continue and run the whole list (including the skills with a system category) since they are still in the list, It wouldn't make any practical error since they'd return False again, but it'd take some extra time and isn't really needed to be performed again

Copy link
Contributor Author

@ken-mycroft ken-mycroft May 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I was just saying they would not run twice as the above loop would exit if a match occurred however you are correct that we could check if they were system level in the below loop to avoid wasting time checking them again, so yes i agree

Copy link
Contributor Author

@ken-mycroft ken-mycroft May 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also note this PR is related to

MycroftAI/skill-npr-news#111

and

MycroftAI/skill-alarm#112

if self.do_converse(utterances, skill[0], lang, message):
# update timestamp, or there will be a timeout where
# intent stops conversing whether its being used or not
return IntentMatch('Converse', None, None, skill[0])

return None

def send_complete_intent_failure(self, message):
Expand Down
42 changes: 40 additions & 2 deletions mycroft/skills/mycroft_skill/mycroft_skill.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,43 @@ def __init__(self, name=None, bus=None, use_settings=True):
# Skill Public API
self.public_api = {}

# added for backward compatability
# skills that are not state aware
# now magically have these attributes
self.state = 'inactive'
self.states = None
self.category = 'undefined'

def change_state(self, new_state):
self.log.debug("skill:%s - changing state from %s to %s" % (self.skill_id, self.state, new_state))

if self.states is None:
return

if new_state not in self.states:
self.log.warning("invalid state change, from %s to %s" % (self.state, new_state))
return

if new_state != self.state:

for intent in self.states[self.state]:
self.disable_intent(intent)

self.state = new_state

for intent in self.states[self.state]:
self.enable_intent(intent)

if new_state == "inactive":
self.log.debug("send msg: deactivate %s" % (self.skill_id,))
self.bus.emit(Message('deactivate_skill_request',
{'skill_id': self.skill_id}))

if new_state == "active":
self.log.debug("send msg: activate %s" % (self.skill_id,))
self.bus.emit(Message('active_skill_request',
{'skill_id': self.skill_id, 'skill_cat':self.category}))

def _init_settings(self):
"""Setup skill settings."""

Expand Down Expand Up @@ -678,8 +715,9 @@ def make_active(self):
This enables converse method to be called even without skill being
used in last 5 minutes.
"""
self.bus.emit(Message('active_skill_request',
{'skill_id': self.skill_id}))
if self.category == 'undefined':
self.bus.emit(Message('active_skill_request',
{'skill_id': self.skill_id}))

def _handle_collect_resting(self, _=None):
"""Handler for collect resting screen messages.
Expand Down
2 changes: 2 additions & 0 deletions mycroft/skills/skill_loader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ def _get_last_modified_time(path):
class SkillLoader:
def __init__(self, bus, skill_directory):
self.bus = bus
self.category = 'undefined'
self.skill_directory = skill_directory
self.skill_id = os.path.basename(skill_directory)
self.load_attempted = False
Expand Down Expand Up @@ -305,6 +306,7 @@ def _create_skill_instance(self, skill_module):
self.instance._register_decorated()
self.instance.register_resting_screen()
self.instance.initialize()
self.category = self.instance.category
Copy link
Contributor

@krisgesling krisgesling May 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the possible Skill categories? Are they verified anywhere or any Skill can provide whatever category they wish?

If we're looking at "common_play" as the News Skill example shows, can we not infer that from the class that Skill inherits from?

except Exception as e:
# If an exception occurs, make sure to clean up the skill
self.instance.default_shutdown()
Expand Down