Skip to content

Conversation

@NeonJarbas
Copy link
Collaborator

@NeonJarbas NeonJarbas commented Feb 3, 2020

IntentAPI, rewrite of MycroftAI/mycroft-core#859 + MycroftAI/mycroft-core#1351

Allows to query intent service from the messagebus

the use case is permissions per user, for example we should not allow chat users to shutdown, or might implement premium skills this way

usage

from pprint import pprint
from mycroft.intent_service import IntentQueryApi

intents = IntentQueryApi()

# loaded skills
pprint(intents.get_skills_manifest())
pprint(intents.get_active_skills())

# intent parsing
pprint(intents.get_adapt_intent("tell me a joke"))
pprint(intents.get_padatious_intent("tell me a joke"))
pprint(intents.get_intent("create a 10 minutes timer"))  # intent that will trigger

# skill from utterance
pprint(intents.get_skill("say a joke"))
pprint(intents.get_skill("do you rhyme"))

# registered intents
pprint(intents.get_adapt_manifest())
pprint(intents.get_padatious_manifest())
pprint(intents.get_intent_manifest())  # all of the above

# registered vocab
pprint(intents.get_entities_manifest()) # padatious entities / .entity files
pprint(intents.get_vocab_manifest()) # adapt vocab / .voc files
pprint(intents.get_regex_manifest()) # adapt regex / .rx files
pprint(intents.get_keywords_manifest())  # all of the above

@NeonJarbas NeonJarbas added feature New feature or request jarbas-core/ovos feature from jarbas-core / OVOS labels Feb 3, 2020
@NeonJarbas NeonJarbas requested a review from NeonDaniel February 3, 2020 19:37
@NeonJarbas
Copy link
Collaborator Author

together with #18 this allows to blacklist intents/skills per user

if not padatious_intent and norm != utterance:
padatious_intent = PadatiousService.instance.calc_intent(norm)
if intent is None or (
padatious_intent and padatious_intent.conf >= 0.95):
Copy link
Member

Choose a reason for hiding this comment

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

Confidence threshold should be read from config so we can make adjustments and keep it in sync with handle_utterance.. Maybe at least put it in a self parameter in this class for now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this doesnt make sense here, we want it work exactly like in handle_utterance, the whole point is that the intent api behaves exactly like the intent service

notice that this check is only when we have both an adapt intent AND a padatious intent, we only want padatious to take precedence over adapt if it is absolutely sure

if adapt intent is None it always returns the padatious intent, in that case maybe it makes sense to make confidence configurable, which should be done also in the padatious service, that would basically mean a fallback skill is triggered.

Note that padatious service checks for >= 0.8 intents first, then allows fallback skills to try, and then tries again with conf >= 0.5 with lower fallback priority

Either way i think this is out of scope, if we change how padatious works i'd rather do that in a different PR, I'm personally not a big fan of the way mycroft is handling this and want to change it later on. In chatterbox i rewrote this whole piece, we have been talking of open sourcing our "intentBox" repo, in which case it would make sense to also use it here in Neon.

In chatterbox padatious is no longer a fallback skill, IntentBox package takes both kinds of intent into account in a single step

Copy link
Member

Choose a reason for hiding this comment

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

Right, but if we want to set the padatious_intent.conf threshold to something like 0.8, we would want that to change in both handle_utterance and handle_get_intent, right? I only bring it up because I noticed that we have this confidence level set to 0.8 in neon-core (not sure if/why we made that change) and it could be problematic if handle_get_intent returns something different than handle_utterance..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i think you are confusing things, padatious is using 0.8 for thresh

this 0.95 is only to take priority over adapt

also keep in mind the intent api does not take into account the fallback skills, so if padatious conf < 0.8 a fallback skill might trigger, the intent api will still return the padatious match, consumers need to check the conf manually

we do not need to change anything in here, when the IntentQueryApi checks for a padatious intent it will use the padatious service directly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i don't think this 0.95 thresh should be configurable at all, but if thats something you want i can make it read from config

Copy link
Member

Choose a reason for hiding this comment

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

Our core has 0.8 hard-coded in intent_service where this PR has 0.95, which why I think this might need to be changed (I can't remember exactly why we made that change, but I think it was related to our i-like-brands skill and some of the brands entities). I just want to make sure that nobody accidentally breaks something in skills by changing 0.95 in one place but misses the other (either by referencing a single var or a comment stating where else the value needs to be constant).

I will do some testing in our core with that value reset to 0.95 because there's a good chance that our change was compensating for some other problem that has since been resolved. I agree with the concept that Adapt will generally yield fewer false positive matches and should take priority unless there's a high confidence Adapt match. I think you're right about leaving it out of the config; opening this up to variable values could make intent matching/troubleshooting across different installations a nightmare..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

well, mycroft changed a bit since then and now padatious is given 3 chances to match

  • super confidence, > 0.95 takes priority over adapt
  • high confidence, > 0.8 takes priority over fallback skills
  • low confidence, > 0.5 is a medium low priority fallback skill

I think it makes more sense to look into that value once i start porting all the skills, then if something is not matching properly we should diagnose if it's bad/old intent design or if we need to tweak these 3 confidences

Adapt is always right, in the sense that it is rule-based, so unless a rule is badly written we always want adapt to take precedence, the very high confidence in padatious also means an exact match, in that case since it is matching a full utterance exactly it makes sense to have priority over adapt

@NeonJarbas
Copy link
Collaborator Author

with this and #24 i can now make a follow up PR allowing to blacklist intents/skills for individual users, this follow up PR would use #18 and add a module that would mutate the utterance to "trigger NOT AUTHORIZED skill"

example use case, you could be authorized to reboot a server mycroft, but other user would not

@NeonDaniel NeonDaniel merged commit 96b0f02 into dev Feb 14, 2020
@NeonDaniel NeonDaniel deleted the feat/intent_api branch July 1, 2021 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature or request jarbas-core/ovos feature from jarbas-core / OVOS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants