-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
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
Snips ASR and NLU component #8156
Conversation
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! |
homeassistant/components/snips.py
Outdated
except: pass | ||
try: return slot["value"] | ||
except: pass | ||
return None |
There was a problem hiding this comment.
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
homeassistant/components/snips.py
Outdated
try: return slot["value"]["value"] | ||
except: pass | ||
try: return slot["value"] | ||
except: pass |
There was a problem hiding this comment.
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)
homeassistant/components/snips.py
Outdated
except: pass | ||
try: return slot["value"]["value"] | ||
except: pass | ||
try: return slot["value"] |
There was a problem hiding this comment.
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)
homeassistant/components/snips.py
Outdated
try: return slot["value"]["value"]["value"] | ||
except: pass | ||
try: return slot["value"]["value"] | ||
except: pass |
There was a problem hiding this comment.
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)
homeassistant/components/snips.py
Outdated
def get_value(self, slot): | ||
try: return slot["value"]["value"]["value"] | ||
except: pass | ||
try: return slot["value"]["value"] |
There was a problem hiding this comment.
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)
homeassistant/components/snips.py
Outdated
if not payload: return | ||
response = json.loads(payload) | ||
if not response: return | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blank line contains whitespace
homeassistant/components/snips.py
Outdated
def handle_intent(self, payload): | ||
if not payload: return | ||
response = json.loads(payload) | ||
if not response: return |
There was a problem hiding this comment.
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)
homeassistant/components/snips.py
Outdated
self.intents = intents | ||
|
||
def handle_intent(self, payload): | ||
if not payload: return |
There was a problem hiding this comment.
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)
homeassistant/components/snips.py
Outdated
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)) |
There was a problem hiding this comment.
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)
homeassistant/components/snips.py
Outdated
|
||
for name, intent in intents.items(): | ||
if CONF_ACTION in intent: | ||
a = intent[CONF_ACTION] |
There was a problem hiding this comment.
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
homeassistant/components/snips.py
Outdated
for name, intent in intents.items(): | ||
if CONF_ACTION in intent: | ||
intent[CONF_ACTION] = script.Script(hass, intent[CONF_ACTION], | ||
"Snips intent {}".format(name)) |
There was a problem hiding this comment.
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
homeassistant/components/snips.py
Outdated
|
||
return True | ||
|
||
class IntentHandler(object): |
There was a problem hiding this comment.
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
homeassistant/components/snips.py
Outdated
} | ||
}, extra=vol.ALLOW_EXTRA) | ||
|
||
def setup(hass, config): |
There was a problem hiding this comment.
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
homeassistant/components/snips.py
Outdated
from homeassistant.helpers import template, script, config_validation as cv | ||
import homeassistant.loader as loader | ||
import voluptuous as vol | ||
import asyncio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'asyncio' imported but unused
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 |
There was a problem hiding this 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
homeassistant/components/snips.py
Outdated
DOMAIN = 'snips' | ||
DEPENDENCIES = ['mqtt'] | ||
CONF_TOPIC = 'topic' | ||
DEFAULT_TOPIC = '#' |
There was a problem hiding this comment.
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?
homeassistant/components/snips.py
Outdated
|
||
def message_received(topic, payload, qos): | ||
if topic == 'hermes/nlu/intentParsed': | ||
LOGGER.info("New intent: {}".format(payload)) |
There was a problem hiding this comment.
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.
homeassistant/components/snips.py
Outdated
|
||
|
||
def setup(hass, config): | ||
LOGGER.info("The 'snips' component is ready!") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this.
homeassistant/components/snips.py
Outdated
LOGGER.info("New intent: {}".format(payload)) | ||
handler.handle_intent(payload) | ||
|
||
LOGGER.info("Subscribing to topic " + str(topic)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this.
homeassistant/components/snips.py
Outdated
def handle_intent(self, payload): | ||
if not payload: | ||
return | ||
response = json.loads(payload) |
There was a problem hiding this comment.
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.
homeassistant/components/snips.py
Outdated
|
||
def get_name(self, response): | ||
try: | ||
return response['intent']['intentName'].split('__')[-1] |
There was a problem hiding this comment.
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.
homeassistant/components/snips.py
Outdated
def get_name(self, response): | ||
try: | ||
return response['intent']['intentName'].split('__')[-1] | ||
except: |
There was a problem hiding this comment.
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.
homeassistant/components/snips.py
Outdated
|
||
def parse_slots(self, response): | ||
slots = response["slots"] | ||
if not slots: |
There was a problem hiding this comment.
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.
homeassistant/components/snips.py
Outdated
|
||
def get_value(self, slot): | ||
try: | ||
return slot["value"]["value"]["value"] |
There was a problem hiding this comment.
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? 🤔
tests/components/test_snips.py
Outdated
|
||
@asyncio.coroutine | ||
def test_snips_call_action(hass, mqtt_mock): | ||
"""Test calling action via Snips.""" |
There was a problem hiding this comment.
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
tests/components/test_snips.py
Outdated
} | ||
""" | ||
|
||
@asyncio.coroutine |
There was a problem hiding this comment.
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
tests/components/test_snips.py
Outdated
"value": "green" | ||
}, | ||
} | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation contains tabs
tests/components/test_snips.py
Outdated
"kind": "Custom", | ||
"value": "green" | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation contains tabs
tests/components/test_snips.py
Outdated
"value": { | ||
"kind": "Custom", | ||
"value": "green" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation contains tabs
tests/components/test_snips.py
Outdated
"intent_name": "Lights", | ||
"probability": 1 | ||
}, | ||
"slots": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation contains tabs
tests/components/test_snips.py
Outdated
"intent": { | ||
"intent_name": "Lights", | ||
"probability": 1 | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation contains tabs
tests/components/test_snips.py
Outdated
"text": "turn the lights green", | ||
"intent": { | ||
"intent_name": "Lights", | ||
"probability": 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation contains tabs
tests/components/test_snips.py
Outdated
{ | ||
"text": "turn the lights green", | ||
"intent": { | ||
"intent_name": "Lights", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation contains tabs
tests/components/test_snips.py
Outdated
EXAMPLE_MSG = """ | ||
{ | ||
"text": "turn the lights green", | ||
"intent": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation contains tabs
Hello. |
Hi @bastshoes! Currently this is not possible but we are fixing this :) |
c3bc81c
to
4241d4e
Compare
Component never worked because the name extraction code was wrong. It referenced Another issue was that 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. |
4241d4e
to
858c354
Compare
858c354
to
ffab784
Compare
Cherry-picked for 0.48. |
* 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
Excellent stuff. Thanks @balloob! |
Are you certain about the 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('#'); |
@Mika56 that's weird. I see 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 ?
|
Yes, change of API. Update throughout :) |
* 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
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
: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 inconfiguration.yaml
: