Skill states

[ken-mycroft]

Description

Adds states to skills

How to test

Should be backward compatible, use existing tests. For new functionality see example skill here

MycroftAI/skill-npr-news#111

Contributor license agreement signed?

yes

[forslund]

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

[ken-mycroft]

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?

[forslund]

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

[ken-mycroft]

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

[ken-mycroft]

Also note this PR is related to

MycroftAI/skill-npr-news#111

and

MycroftAI/skill-alarm#112

[krisgesling]

It feels like we're trying to achieve two distinct but related things here:

1. Simple state management of Skills
2. Skill categories which are partly a hierarchy but may in the future be different categories at the same level.

In both instances, rather than having Skill developers responsible for defining and updating these, it seems that we should be able to derive them. This may also mean they better reflect the actual state / category, or maybe not...

The state implementation here seems focused on whether the Skill is in the active Skills list. If that's the case would it be sufficient to provide a method for Skills to deactivate themselves when they've finished their task? Eg a make_inactive() method to complement the existing make_active()

From my read of this, if we provide that ability, then the rest of this can be handled in the Skill rather than modifying core. I'm sure that this is only the first piece of a broader piece of functionality but I'd rather get an idea of what we're trying to do and what the scope is of that rather than adding things in a piecemeal fashion. Eg is state going to stay as [active, inactive] or are we looking at [waiting_for_response, thinking, etc]

Given this is going into the Mark II feature branch it's not going to impact other projects and we can pull it back out but we're making the merge of this feature branch more and more difficult by adding things like this. Since it can be done in the Skills for the moment I'm not sure I see the benefits outweighing that cost.

It also seems like the original aim is to improve the stopping functionality so I'd also suggest we look at that method and make it more like converse() rather than using converse as a substitute.

[krisgesling]

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?

[krisgesling]

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?

[krisgesling]

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

[ken-mycroft]

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.

[forslund]

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.

[ken-mycroft]

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.

[ken-mycroft]

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.

[forslund]

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).

[ken-mycroft]

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?

[forslund]

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

[forslund]

Whipped up a quick test

     @intent_handler(IntentBuilder("MyIntent").require("test").require("adapt"))
      def handle_test_adapt(self, message):
          self.speak("Test adapt")
          self.disable_intent("OtherIntent")

    @intent_handler(IntentBuilder("OtherIntent").require("other").require("adapt"))
      def handle_other_adapt(self, message):
          self.speak("Other adapt")

This works. After MyIntent is called OtherIntent no longer gets picked up.

Could it be an issue with automatically named intents?

[forslund]

How is the intent name stored in the state list? does that use the intent name or the skill_id:intent_name format? enable / disable prepends the skill id automatically, maybe it gets doubled?

Edit: or maybe precise intents?

Edit2:
did minimal tests for both automatic/unnamed adapt-intents and padatious-intents and they did work, so maybe something more complex afoot that the minimal tests misses unless it's the inclusion of skill_id when you store the intents for the state...

Also reading the log you posted seems to be matching the stop skill's stop intent and not the alarm skill's?

Can you elaborate on what you mean with active here?