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

Snips ASR and NLU component #8156

Merged
merged 14 commits into from Jul 1, 2017

Conversation

michaelfester
Copy link
Contributor

@michaelfester michaelfester commented Jun 22, 2017

Description:

Snips on-device ASR and NLU custom component.

For installation and setup of the Snips SDK, check out the Snips Installation Guide.

The Snips SDK must be installed and running locally, and Home Assistant should use the Snips SDK as an MQTT broker for simplicity. For instance, if the Snips platform is running locally on port 9898, the following should be added to configuration.yaml:

mqtt:
  broker: 127.0.0.1
  port: 9898

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#2892

Example entry for configuration.yaml:

Just like Alexa and API.ai, Snips transforms voice and text into intents. Intents are created via the Snips Console, and can then be handled in Home Assistant by adding the intents, and accompanying actions, to the snips entry in configuration.yaml:

snips:
  intents:
    Lights:
      action:
        - service_template: >
            {%- if light_state == "on" -%}
              light.turn_on
            {%- else -%}
              light.turn_off
            {%- endif -%}
          data_template:
            entity_id: light.living_room

@homeassistant
Copy link
Contributor

Hi @michaelfester,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

except: pass
try: return slot["value"]
except: pass
return None

Choose a reason for hiding this comment

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

no newline at end of file

try: return slot["value"]["value"]
except: pass
try: return slot["value"]
except: pass

Choose a reason for hiding this comment

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

multiple statements on one line (colon)

except: pass
try: return slot["value"]["value"]
except: pass
try: return slot["value"]

Choose a reason for hiding this comment

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

multiple statements on one line (colon)

try: return slot["value"]["value"]["value"]
except: pass
try: return slot["value"]["value"]
except: pass

Choose a reason for hiding this comment

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

multiple statements on one line (colon)

def get_value(self, slot):
try: return slot["value"]["value"]["value"]
except: pass
try: return slot["value"]["value"]

Choose a reason for hiding this comment

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

multiple statements on one line (colon)

if not payload: return
response = json.loads(payload)
if not response: return

Choose a reason for hiding this comment

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

blank line contains whitespace

def handle_intent(self, payload):
if not payload: return
response = json.loads(payload)
if not response: return

Choose a reason for hiding this comment

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

multiple statements on one line (colon)

self.intents = intents

def handle_intent(self, payload):
if not payload: return

Choose a reason for hiding this comment

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

multiple statements on one line (colon)

for name, intent in intents.items():
if CONF_ACTION in intent:
a = intent[CONF_ACTION]
intent[CONF_ACTION] = script.Script(hass, intent[CONF_ACTION], "Snips intent {}".format(name))

Choose a reason for hiding this comment

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

line too long (110 > 79 characters)


for name, intent in intents.items():
if CONF_ACTION in intent:
a = intent[CONF_ACTION]

Choose a reason for hiding this comment

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

local variable 'a' is assigned to but never used

for name, intent in intents.items():
if CONF_ACTION in intent:
intent[CONF_ACTION] = script.Script(hass, intent[CONF_ACTION],
"Snips intent {}".format(name))

Choose a reason for hiding this comment

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

continuation line under-indented for visual indent


return True

class IntentHandler(object):

Choose a reason for hiding this comment

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

expected 2 blank lines, found 1

}
}, extra=vol.ALLOW_EXTRA)

def setup(hass, config):

Choose a reason for hiding this comment

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

expected 2 blank lines, found 1

from homeassistant.helpers import template, script, config_validation as cv
import homeassistant.loader as loader
import voluptuous as vol
import asyncio

Choose a reason for hiding this comment

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

'asyncio' imported but unused

@balloob
Copy link
Member

balloob commented Jun 24, 2017

Very cool! Looking forward to Snips support.

This will also require a test to be written. Could be something like this (untested):

import asyncio

from homeassistant.bootstrap import async_setup_component
from tests.common import async_fire_mqtt_message, mock_service

EXAMPLE_MSG = """
{
  // snips message from hermes/nlu/intentParsed for intent Lights
}
"""

@asyncio.coroutine
def test_snips_call_action(hass, mqtt_mock):
    """Test calling action via Snips."""
    calls = mock_service(hass, 'test', 'service')

    result = yield from async_setup_component(hass, 'snips', {
        'snips': {
            'intents': {
                'Lights': {
                    'action': {
                        'service': 'test.service'
                    }
                }
            }
        }
    })
    assert result

    async_fire_mqtt_message(hass, 'hermes/nlu/intentParsed',
                            EXAMPLE_MSG)
    yield from hass.async_block_till_done()

    assert len(calls) == 1

Copy link
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

Off to a good start, will need some more work. There is also a bunch of lint issues that need to be addressed https://travis-ci.org/home-assistant/home-assistant/jobs/245793758#L282

DOMAIN = 'snips'
DEPENDENCIES = ['mqtt']
CONF_TOPIC = 'topic'
DEFAULT_TOPIC = '#'
Copy link
Member

Choose a reason for hiding this comment

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

Since message_received only cares about messages from hermes/nlu/intentParsed, better to just listen to that topic instead and not allow it to be configured?


def message_received(topic, payload, qos):
if topic == 'hermes/nlu/intentParsed':
LOGGER.info("New intent: {}".format(payload))
Copy link
Member

Choose a reason for hiding this comment

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

Please change this to debug.



def setup(hass, config):
LOGGER.info("The 'snips' component is ready!")
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this.

LOGGER.info("New intent: {}".format(payload))
handler.handle_intent(payload)

LOGGER.info("Subscribing to topic " + str(topic))
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this.

def handle_intent(self, payload):
if not payload:
return
response = json.loads(payload)
Copy link
Member

Choose a reason for hiding this comment

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

This can raise a TypeError if invalid JSON.


def get_name(self, response):
try:
return response['intent']['intentName'].split('__')[-1]
Copy link
Member

Choose a reason for hiding this comment

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

It's probably the easiest to write a voluptuous schema to validate the schema that Snips is sending. That way you can get rid of all other checks if some piece of data is available.

def get_name(self, response):
try:
return response['intent']['intentName'].split('__')[-1]
except:
Copy link
Member

Choose a reason for hiding this comment

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

Catch KeyError. Bare exceptions are not allowed.


def parse_slots(self, response):
slots = response["slots"]
if not slots:
Copy link
Member

Choose a reason for hiding this comment

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

This is not possible. Python will raise if a key does not exists. Unless the JSON value of slots can be set to null ? Again, probably easier to write a voluptuous schema to validate this info.


def get_value(self, slot):
try:
return slot["value"]["value"]["value"]
Copy link
Member

Choose a reason for hiding this comment

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

Errrrrr, what is happening here? 🤔


@asyncio.coroutine
def test_snips_call_action(hass, mqtt_mock):
"""Test calling action via Snips."""

Choose a reason for hiding this comment

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

indentation contains mixed spaces and tabs

}
"""

@asyncio.coroutine

Choose a reason for hiding this comment

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

expected 2 blank lines, found 1

"value": "green"
},
}
]

Choose a reason for hiding this comment

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

indentation contains tabs

"kind": "Custom",
"value": "green"
},
}

Choose a reason for hiding this comment

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

indentation contains tabs

"value": {
"kind": "Custom",
"value": "green"
},

Choose a reason for hiding this comment

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

indentation contains tabs

"intent_name": "Lights",
"probability": 1
},
"slots": [

Choose a reason for hiding this comment

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

indentation contains tabs

"intent": {
"intent_name": "Lights",
"probability": 1
},

Choose a reason for hiding this comment

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

indentation contains tabs

"text": "turn the lights green",
"intent": {
"intent_name": "Lights",
"probability": 1

Choose a reason for hiding this comment

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

indentation contains tabs

{
"text": "turn the lights green",
"intent": {
"intent_name": "Lights",

Choose a reason for hiding this comment

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

indentation contains tabs

EXAMPLE_MSG = """
{
"text": "turn the lights green",
"intent": {

Choose a reason for hiding this comment

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

indentation contains tabs

@bastshoes
Copy link
Contributor

Hello.
In most cases HASS users already have MQTT installed in their setup. Currently HASS do not support multiple MQTT brokers.
As I understand from documentation Snips Dockers image includes MQTT broker and it available on port 9898.
Is it possible to use existing broker with Snips component?

@michaelfester
Copy link
Contributor Author

michaelfester commented Jul 1, 2017

Hi @bastshoes! Currently this is not possible but we are fixing this :)

@balloob
Copy link
Member

balloob commented Jul 1, 2017

Component never worked because the name extraction code was wrong. It referenced intentName instead of intent_name (as per Snips docs). Slot parsing had the same problem. You referenced slotName instead of slot_name (as per Snips docs).

Another issue was that mock_service was no longer allowed to be called in an async context. Rebased the branch and imported async_mock_service instead.

Converted it to async because there was no reason to use threads.

I've added some schema validation to validate incoming data instead of having to check each value individually.

Now all tests should pass and this is good to go.

@balloob balloob added this to the 0.48 milestone Jul 1, 2017
@balloob balloob force-pushed the snips-component-integration branch from 858c354 to ffab784 Compare July 1, 2017 20:27
@balloob balloob merged commit b82003a into home-assistant:dev Jul 1, 2017
@balloob
Copy link
Member

balloob commented Jul 1, 2017

Cherry-picked for 0.48.

balloob pushed a commit that referenced this pull request Jul 1, 2017
* Snips ASR and NLU component

* Fix warning

* Fix warnings

* Fix lint issues

* Add tests

* Fix tabs

* Fix newline

* Fix quotes

* Fix docstrings

* Update tests

* Remove logs

* Fix lint warning

* Update API

* Fix Snips
@balloob balloob mentioned this pull request Jul 1, 2017
@michaelfester
Copy link
Contributor Author

Excellent stuff. Thanks @balloob!

@Mika56
Copy link

Mika56 commented Jul 2, 2017

Are you certain about the intentName and slotName keys?
I had multiple errors (Intent has invalid schema: required key not provided @ data['slots'][0]['slot_name'], Intent has invalid schema: required key not provided @ data['text'], ...)

I had to build a MQTT bridge that adapts the output from Snips:

var mqtt = require('mqtt');

var snipsClient = mqtt.connect('mqtt://localhost:9898');
var hassClient = mqtt.connect('mqtt://192.168.1.247:1884');

snipsClient.on('connect', function() { 
	console.log('Connected to Snips broker'); 
});

hassClient.on('connect', function() {
	console.log('Connected to HASS broker');
});

snipsClient.on('message', function(topic, message) {
	console.log(' < '+topic);
	message = message.toString();
	if(message) {
		message = JSON.parse(message);
		//console.log(message);
		if(message.hasOwnProperty('intent')) {
			message.intent.intent_name = message.intent.intentName;
		}
		if(message.hasOwnProperty('slots')) {
			for(var i=0;i<message.slots.length;i++) {
				message.slots[i].slot_name = message.slots[i].slotName;
			}
		}
		if(message.hasOwnProperty('input')) {
			message.text = message.input;
		}
		//console.log(message);
		hassClient.publish(topic, JSON.stringify(message));
	}
	else {
		hassClient.publish(topic, message);
	}
});

snipsClient.subscribe('#');

@balloob
Copy link
Member

balloob commented Jul 2, 2017

@Mika56 that's weird. I see intent_name and slot_name used all over the docs https://github.com/snipsco/snips-platform-documentation/wiki/5.-Learn-more:-Key-Concepts#3-natural-language-understanding and https://medium.com/snips-ai/integrating-snips-with-home-assistant-314723645c77#6bac

I do admit that I didn't test it with an actual Snips instance.

@michaelfester can you chime in what the right variable names are ?

  • text or input
  • slot_name or slotName
  • intent_name or intentName

@adrienball adrienball mentioned this pull request Jul 3, 2017
@michaelfester
Copy link
Contributor Author

Yes, change of API. Update throughout :)

@balloob balloob mentioned this pull request Jul 13, 2017
dethpickle pushed a commit to dethpickle/home-assistant that referenced this pull request Aug 18, 2017
* Snips ASR and NLU component

* Fix warning

* Fix warnings

* Fix lint issues

* Add tests

* Fix tabs

* Fix newline

* Fix quotes

* Fix docstrings

* Update tests

* Remove logs

* Fix lint warning

* Update API

* Fix Snips
@home-assistant home-assistant locked and limited conversation to collaborators Oct 20, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants