MycroftAI / skill-homeassistant

Mycroft Skill/Integration for Homeassistant
GNU Lesser General Public License v3.0
114 stars 62 forks source link

Intent conflicts for what and where #64

Closed krisgesling closed 1 year ago

krisgesling commented 3 years ago

Submitting the latest Skill to the Marketplace showed there are some small intent conflicts with default Skills: PR - https://github.com/MycroftAI/mycroft-skills/pull/1459 Voight Kampff report - https://reports.mycroft.ai/skills/PR-1459/

Primarily the utterances:

It would be good to look at why this Skill is catching these utterances and improving intent handling so that we aren't trying to respond to utterances that don't relate to this Skill.

We could also add Voight Kampff tests for phrases that Home Assistant should not trigger on, like the above. Here's an example of that in the Timer Skill: https://github.com/MycroftAI/mycroft-timer/blob/20.08/test/behave/timer.feature#L430

Tony763 commented 3 years ago

Hello @krisgesling , for VK, please check: Travis Allure report Its based on real home assistant installation (pip package) created for each build with same settings. I would recommend that instead of mocking. Home assistant is also continuously improved (tons of braking changes) so if we create mocking that works with current build, it will likely stop working in future. This way we can also test not just a intent handling but also real skill function. Its based on Travis but it should be portable to other CIs.

"Where is" is catched by tracking intent and "what is the status" is catched by sensor intent. There is little room to improve intents as these utterances are common and you will face it more often in future as Mycroft and number of it's skills grow. And add name of skill to beginning of utterance is not so user friendly.

I am not sure when entity is not found in HA it should jump to next skill, but I cannot test it as I do not have a working setup right now.

Tony763 commented 3 years ago

Managed to make Mycroft working again. I tested these utterances but sadly when utterance is catched by HA skill and entity is not found, Mycroft will respond with HA error and not try other skill.

@krisgesling Is there any workaround in other skills (music skills for example) If I say play artist and song and it's not found by first skill so Mycroft will try another enabled skill?

Tony763 commented 3 years ago

For timer skill and utterance: "what is the status of the chicken timer": Timer have both Adapt and Padatious intents:

Padatious: how (much time|long) (is|) (left|remaining) how (much|much time|long) (is|) (left|remaining) on (my|the|) {name} timer how's (my|the) {name} timer how's (my|the) timer when (does|will) (my|the) timer (end|finish) timer status status of timer

Adapt query: tell are there what what's do you have do I have when when's how how's

Adapt status: status statuses left remaining list active running have exist are there created check

HA skill uses only Padatious intents: (What is|Give me|Tell me) (the|) (value|state|status|read out) of thermostat {Entity} (please|). (What is|Give me|Tell me) (the|) (value|state|status|read out) of {Entity} (please|).

For both, utterances are correct and do exactly as user want. Only difference is that for timer skill there is "timer" at end of utterance. We could easily remove "status" from the intent in HA but it will likely only be play with words and only limit user. Second option would be if we can pass utterance to next skill if we do not find a match in HA. Third option would be if we can "blacklist" word in Padatious. Let say: (What is|Give me|Tell me) (the|) (value|state|status|read out) of {Entity} (!timer) (please|). (!timer) would mean that word "timer" is not allowed and utterance is not matched if contain it.

For "where is morocco" utterance, Yes it is way too common. My PR #53 is still pending. It rewrite tracker intent to Padatious. It contain: Where is (the|) {Entity}. (Give|Find) me (location|position) of (the|) {Entity}.

Where is could be removed, other one should be sufficient for users and it's not so common.

Any options?

krisgesling commented 3 years ago

Hey Tony, with the music and media Skills we use the Common Play Framework which allows multiple Skills to trigger on things like "Play Metallica". The Common IoT Framework is largely there, it just has some incompatibilities with Python3.5 that will be dropped in the February release and can be utilised from then on.

It's interesting to think of whether "what is the status of {entity}" should be handled by this Common IoT Framework. Preferably we can train intents based on the registered entities.

As to your specific suggestions:

  1. Agree this isn't ideal to remove "status". It doesn't address the root issue and just limits interaction.
  2. It has been a broader question of whether we should allow Skills to say they couldn't handle an utterance and allow the next Skill to have a go. There are however good and bad things about this approach. The biggest downside is that it could really slow down responses.
  3. This would address this situation but ideally the HA Skill would only respond for its registered entities. I'd need to do some testing to see how this could work at the moment.
Tony763 commented 1 year ago

After merging PR #112, this will be completed.