Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

converse, intent_parser, external shutdown/reload, priority and blacklist skills from config #783

Closed
wants to merge 8 commits into from

Conversation

JarbasAI
Copy link
Contributor

@JarbasAI JarbasAI commented May 23, 2017

I messed up and accidentaly closed #756 this is a duplicate with clean and pretty commit history, sorry

  • blacklisted skills now read from config
  • added priority skills to be loaded first (any skill that needs to listen to register messages should load first) read from config
  • skills now have unique ID
  • converse method
  • intent service now listens for external messages
  • intent parser helper class, get intent from utterance, source skill from intent
  • skills main now listens for external messages for skill_manifest, skill_shutdown and skill_reload
  • enable and disable intent can be requested from outside

allows continuous dialog by using converse #539 method and giving intent parsing tools inside any skill

enable and disable intent can be requested from outside skill, allows for more tools like intent layers class

external shutdown and skill reload allow for run levels, i am implementing a control center skill where i already do this, i have a "core" run level, a "offline" run level and a "dev" run level

priority skills is needed for example in my objectives skills that needs to listen for register_objective messages from other skills, any "manifest" or "context" skill would also need this

intent also listens for messages to bump to active skill list, so passive skills can ensure converse method is called in them

@augustnmonteiro augustnmonteiro self-requested a review May 23, 2017 18:06
// blacklisted skills to not load
"blacklisted_skills": ["send_sms", "media"],
// priority skills to be loaded first
"priority_skills": [],
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need priority_skills anymore : D

Copy link
Contributor Author

@JarbasAI JarbasAI May 23, 2017

Choose a reason for hiding this comment

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

yes we do... not in intent class but in other skills, i can give some personal use cases

my objectives skill, needs to listen to messages that can be emitted by any skill

some manifest skill that wants to listen to register messages of anything needs this

control center skill to enforce run levels at start up

@forslund forslund self-requested a review May 23, 2017 20:13
@forslund
Copy link
Collaborator

forslund commented Jun 3, 2017

I've been looking at the converse part and I'm afraid it will be a bit limiting doing it this way or require much manual labour from the skill creator. I've been looking into the context management of adapt and it provides some support for this kind of thing. Injecting a context to trigger some intents. It would also allow other skills to use context from previous skills.

Have you looked into adapt anything?

@JarbasAI
Copy link
Contributor Author

JarbasAI commented Jun 5, 2017

context is a different matter, i have not looked much into adapt, but i have read the discussions about context in particular

converse method is a different thing, example use case that needs it, parrot skill, it needs to intercept all utterances from being processed by intent service

@JarbasAI
Copy link
Contributor Author

https://github.com/JarbasAI/JarbasAI/tree/jarbas-core/Jarbas_docs

i made some documentation for these changes and what they are used for

@forslund
Copy link
Collaborator

Thanks @JarbasAI, that documentation is much appreciated. I'll get a move on with this (this time I promise)

@JarbasAI
Copy link
Contributor Author

JarbasAI commented Jun 24, 2017

fixed some stuff i noticed when making my stable branch, like error when priority skill didnt exist, also merged new changes

if desired i can propose PRs for changes one by one, maybe you want to merge some changes but not all of them?

@forslund
Copy link
Collaborator

The weather and the wiki skill doesn't seem work with this. datetime and speak seem to be working as expected.

@forslund
Copy link
Collaborator

Lots of stuff to get my head around but a couple of things get's my attention:

  • converse(): reading the parrot example the converse() name is not really suitable for what it does. It's more of an override. is it intended to handle conversations?

  • the skill_container hasn't been updated to use this and the current method may cause collisions

The PR is a bit weird since it does a couple of unrelated things. Modification to the blacklisting is a very simple thing that absolutely should be merged, while I think the architecture of the converse part needs some thought. (It might be that I don't fully grasp the code completely yet and everything is nice and simple)

@JarbasAI
Copy link
Contributor Author

i will split this into smaller PRs, i agree its too much stuff to be merged at once

about converse, im not sure what the best name would be, i went with converse @penrods suggestion, it is a good place to pre-process utterances and change internal stuff in the skill (prepare the conversation), it may or not over-ride intentservice, and it kinda is like if you are conversing with that skill instead of all skills... what would a good name be?

@forslund
Copy link
Collaborator

Thanks for taking the time to split it up.

Naming is the hardest part of coding =) preferred_parser maybe? For most of the skill conversations I personally would much rather have a solution using adapt but this is very good for the extreme cases such as the repeat exactly what I say example.

Do you have any idea why the weather and wolfram skills won't trigger for me using the branch?

@@ -240,9 +247,17 @@ def initialize(self):
"""
raise Exception("Initialize not implemented for skill: " + self.name)

def converse(self, transcript, lang="en-us"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't transcript be utterances to keep it in line with the wording that has been used previously?

self.emitter.on('converse_status_response', self.handle_conversation_response)
self.emitter.on('intent_request', self.handle_intent_request)
self.emitter.on('intent_to_skill_request', self.handle_intent_to_skill_request)
self.emitter.on('active_skill_request', self.handle_active_skill_request)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should try to group these into mycroft.intent.[name] to keep them grouped. (The older ones should be renamed as well but that's not required since that's outside the scope of this PR)

@penrods penrods added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Aug 17, 2017
@JarbasAI JarbasAI closed this Aug 23, 2017
@JarbasAI JarbasAI deleted the patch-10 branch August 23, 2017 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants