Move HassIntent handler code into helpers/intent by todschmidt · Pull Request #12181 · home-assistant/core

todschmidt · 2018-02-04T22:50:23Z

Description:

This moves the HassIntent handler code into intent in preparation for adding default intents to other components.

Related issue (if applicable): fixes #12087

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

Example entry for `configuration.yaml` (if applicable):

Checklist:

The code change is tested and works locally.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

If the code does not interact with devices:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
Tests have been added to verify that the new code works.

houndci-bot · 2018-02-04T22:50:33Z


    pass

+@callback


expected 2 blank lines, found 1

…tant into intent-registry-2

MartinHjelmare · 2018-02-05T02:17:08Z

+        hass = intent_obj.hass
+        slots = self.async_validate_slots(intent_obj.slots)
+        name = slots['name']['value']
+        entity = _match_entity(hass, name)


This is retrieving a state not an entity.

It;s a roundabout way of retrieving all entites, by getting all states and using the entities that belong to them. Need to add filtering by domain.

The state machine doesn't store entities, it stores states. You should change the variable name to reflect what it is.

The function returns the entity_id though and not the state.

return hass.states.get(entity_id) if entity_id else None

No, it returns a state:
https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/core.py#L662-L668

Ah, I see what you mean. I didn't write that bit. It would be more confusing to return a state, so rewrote the return

Let's rename it to entity_id, as that is what is returned from _match_entity.

balloob · 2018-02-06T00:08:01Z

 EVENT_LOGBOOK_ENTRY = 'logbook_entry'
 EVENT_THEMES_UPDATED = 'themes_updated'

+# #### INTENTS ####


I think it's fine to keep these defined inside helpers.intent .

That works, components can just import them from intents, just felt a little cleaner to put the in consts but all good.

balloob · 2018-02-06T00:08:19Z

 from homeassistant.loader import bind_hass
+from homeassistant.const import ATTR_ENTITY_ID

+REQUIREMENTS = ['fuzzywuzzy==0.16.0', 'python-Levenshtein==0.12.0']


Helpers can't have requirements.

Yeah, i saw that, but the problem in this particular case is that component/init.py isn't considered a module so doesn't allow requirements. I could put it anywhere under components but that raises a dependency issue.

houndci-bot · 2018-02-06T15:54:37Z

 import asyncio
 import logging

+import fuzzywuzzy


'fuzzywuzzy' imported but unused

todschmidt · 2018-02-06T20:33:25Z

@balloob @MartinHjelmare So I think I am done with this piece of it, I will do separate PRs to add intents for other components.

balloob · 2018-02-07T22:59:26Z

 @asyncio.coroutine
 def async_setup(hass, config):
    """Register the process service."""
-    warnings.filterwarnings('ignore', module='fuzzywuzzy')


We should add this back where we import fuzzywuzzy. I remember it spamming a lot.

I see you've removed fuzzywuzzy, ok too 👍

balloob · 2018-02-07T23:01:09Z

+class ServiceIntentHandler(IntentHandler):
+    """Intent handler registration."""
+
+    domain = None


Would it make sense to take these in the constructor? That way you don't have to specify 3 classes in components/__init__.py.

Do you mean by extending/creating a different async_register?

class ServiceIntentHandler(IntentHandler): def __init__(self, domain, service, response): self.domain = domain self.service = service self.response = response

Well, when you put it that way :-) Thanks again.

balloob · 2018-02-07T23:01:54Z

+    return [x for _, _, x in sorted(matches)]
+
+
+class ServiceIntentHandler(IntentHandler):


Can you specify in both the name and doc of this class that it is only working for services that call entities and that it needs a name slot that will be mapped to entity_id in the service

Updated. I think having the slot schema in the class also makes it clearer.

balloob · 2018-02-07T23:02:14Z

    'pushbullet.py',
    'py-canary',
    'pydispatcher',
+    'python-Levenshtein',


No longer needed

todschmidt · 2018-02-08T00:23:21Z

I think we can get away without using fuzzy, I have a function that does a decent job of matching the entities and removing fuzzy makes things cleaner if a little less tolerant. No intent depends on conversation, or anything odd in __init>>. Take a look and see if this looks better. I still need to add tests for intents to test_init and address your other bits.

houndci-bot · 2018-02-09T14:29:32Z

+    assert call.service == 'toggle'
+    assert call.data == {'entity_id': ['light.test_light']}
+
+@asyncio.coroutine


expected 2 blank lines, found 1

houndci-bot · 2018-02-09T14:29:32Z

+    assert call.service == 'turn_off'
+    assert call.data == {'entity_id': ['light.test_light']}
+
+@asyncio.coroutine


expected 2 blank lines, found 1

houndci-bot · 2018-02-09T14:29:33Z

+    assert call.service == 'turn_on'
+    assert call.data == {'entity_id': ['light.test_light']}
+
+@asyncio.coroutine


expected 2 blank lines, found 1

houndci-bot · 2018-02-09T14:29:33Z

        assert mock_check.called
        assert not mock_stop.called
+
+@asyncio.coroutine


expected 2 blank lines, found 1

houndci-bot · 2018-02-09T14:29:33Z

 from homeassistant.const import (
    STATE_ON, STATE_OFF, SERVICE_TURN_ON, SERVICE_TURN_OFF, SERVICE_TOGGLE)
 import homeassistant.components as comps
+import homeassistant.components.group as group


'homeassistant.components.group' imported but unused

houndci-bot · 2018-02-09T14:29:33Z

 # pylint: disable=protected-access
 import asyncio
 import unittest
+import inspect


'inspect' imported but unused

houndci-bot · 2018-02-09T14:29:33Z

+    assert call.service == 'toggle'
+    assert call.data == {'entity_id': ['light.test_light']}
+
+@asyncio.coroutine


expected 2 blank lines, found 1

houndci-bot · 2018-02-09T14:29:33Z

+    assert call.service == 'turn_off'
+    assert call.data == {'entity_id': ['light.test_light']}
+
+@asyncio.coroutine


expected 2 blank lines, found 1

houndci-bot · 2018-02-09T14:29:33Z

+    assert call.service == 'turn_on'
+    assert call.data == {'entity_id': ['light.test_light']}
+
+@asyncio.coroutine


expected 2 blank lines, found 1

houndci-bot · 2018-02-09T14:29:33Z

        assert mock_check.called
        assert not mock_stop.called
+
+@asyncio.coroutine


expected 2 blank lines, found 1

houndci-bot · 2018-02-09T14:29:34Z

 from homeassistant.const import (
    STATE_ON, STATE_OFF, SERVICE_TURN_ON, SERVICE_TURN_OFF, SERVICE_TOGGLE)
 import homeassistant.components as comps
+import homeassistant.components.group as group


'homeassistant.components.group' imported but unused

houndci-bot · 2018-02-09T14:29:34Z

 # pylint: disable=protected-access
 import asyncio
 import unittest
+import inspect


'inspect' imported but unused

balloob · 2018-02-09T22:16:21Z

+        name = slots['name']['value']
+        entities = {state.entity_id: state.name for state
+                    in hass.states.async_all()}
+        entity_name = name.replace(' ', '_').lower()


Comment didn't stick, but sine we convert spaces to _ in friendly names this is needed to match and entity names will never contain a space. But this is better done in the matching code.

We match against entity name and not id right?

Hmm, should be both since that code should create a dict of id: name but apparently does not for whatever reason and is just creating a list of entity_ids. Flipped it to match on name.

balloob · 2018-02-09T22:17:38Z

+        entities = {state.entity_id: state.name for state
+                    in hass.states.async_all()}
+        entity_name = name.replace(' ', '_').lower()
+        entity_name = entity_name.replace('the_', '')


Please add a comment why this is? Also, it seems very English focused

True, and again with the above this may no longer be necessary but then again it might, but this logic should be moved into that function. But a typical utterance is 'Turn on the living room lights' so we get 'the living room lights' for the entity name. Not that we are trying to match everything but it seemed a common enough use. But I can remove it since there is no need to make it too specific and turn on livign room lights is fine.

This sounds like an issue that should be fixed by things launching intents?

Yep, makes sense that it would be done by extending utterances instead.

balloob · 2018-02-09T22:19:36Z

+        entity_name = entity_name.replace('the_', '')
+
+        matches = fuzzymatch(entity_name, entities)
+        entity_id = next((x for x in matches if self.domain in x), None)


What's with this check? Isn't domain for now always homeassistant and we don't have those entities?

Well my plan is to add things like covers and media players, so an Intent like HassOpenCover declares it's domain we then match against cover.garage_door and not switch.garage_door or sensor.garage_door.

I think that it's:

out of scope for the current implementation (since it only works with homeassistant

Filter should be applied when generating the dictionary of potential entities to match instead of filtering it after matching.

A "limit matching to domain" should be an optional boolean passed into the constructor

Why would we add another passed variable instead of just 'if domain not Homeassistant'? And I don't think it's out of scope since the whole intention of this is to lay some groundwork for adding other intents. Otherwise all of this has just been rearranging things with zero added value beyond cleanup.

Can you 100% guarantee that you can always derive the filter from the service domain? Because if you can't, an extra variable is necessary.

A cleanup so that you can start building on top is perfectly fine to be in a single pr. If you're adding functionality you need to use it and you need to test it. Neither is happening.

The best filter would be to filter by entites that support the given domain.service

Correct. However, one thing is that Home Assistant core should not be aware of the different components. So if we would want something like this, we would have to make it part of service registration, and then the service registry needs to know about which attribute should map to the entity… it's a bit too much. So I think that passing the right filter at the moment we define the intent is great, that way the logic remains inside the components. (light intent will specify light filter, etc)

balloob · 2018-02-09T22:20:47Z

+        yield from hass.services.async_call(
+            self.domain, self.service, {
+                ATTR_ENTITY_ID: entity_id
+            }, blocking=True)


Let's remove blocking, it has been causing trouble for Alexa/Google Assistant as turning on can sometimes take a long time and we don't actually know if a service is successful.

MartinHjelmare · 2018-02-10T22:53:25Z

+        if not entity_id:
+            response.async_set_speech(
+                "Could not find entity id matching {}.".format(name))
+            _LOGGER.error("Could not find entity id matching %s.", name)


Please remove period at the end.

balloob

🐬 🎉

Only thing left is to see if all the additions of result = yield from component.async_setup(hass, {}) to the conversation component tests are necessary and removed if not.

todschmidt added 6 commits February 3, 2018 10:52

Moved TurnOn/Off Intents to component

eedb3d0

Removed unused import

bf015c9

Lint fix which my local runs dont catch apparently...

b3d7bf6

Moved hass intent code into intent

8651566

Added test for toggle to conversation.

c842be2

Fixed toggle tests

04b407a

todschmidt requested a review from a team as a code owner February 4, 2018 22:50

homeassistant added cla-signed component: __init__ integration: conversation core labels Feb 4, 2018

houndci-bot reviewed Feb 4, 2018

View reviewed changes

Comment thread homeassistant/helpers/intent.py Outdated

pass

@callback

Copy link
Copy Markdown

houndci-bot Feb 4, 2018

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

todschmidt changed the title ~~Move HassIntent handler code into helpers/intent~~ [WIP] Move HassIntent handler code into helpers/intent Feb 4, 2018

todschmidt mentioned this pull request Feb 4, 2018

RFC: Adding Default Hass Intents to Components #12087

Closed

Update intent.py

c479075

homeassistant added the cla-signed label Feb 4, 2018

todschmidt added 2 commits February 4, 2018 19:04

Added homeassistant.helpers to gen_requirements script.

13d0d9a

Merge branch 'intent-registry-2' of github.com:tschmidty69/home-assis…

ac5d2e5

…tant into intent-registry-2

todschmidt requested a review from andrey-git as a code owner February 5, 2018 00:05

homeassistant added the cla-signed label Feb 5, 2018

MartinHjelmare reviewed Feb 5, 2018

View reviewed changes

todschmidt added 3 commits February 5, 2018 16:55

Update intent.py

2dcfe18

Update intent.py

861cdc5

Changed return value for _match_entity

2214096

balloob reviewed Feb 6, 2018

View reviewed changes

Moved consts and requirements

90280d7

houndci-bot reviewed Feb 6, 2018

View reviewed changes

Removed unused import

f287689

woof

cb8ab73

balloob reviewed Feb 7, 2018

View reviewed changes

todschmidt changed the title ~~Move HassIntent handler code into helpers/intent~~ [WIP] Move HassIntent handler code into helpers/intent Feb 8, 2018

A few cleanups

2d9becd

homeassistant added the cla-signed label Feb 8, 2018

todschmidt added 4 commits February 8, 2018 10:11

Added domain filtering to entities

0d955d3

Clarified class doc string

1262ba3

Added doc string

ed1bcf2

Added test in test_init

6cfce83

houndci-bot reviewed Feb 9, 2018

View reviewed changes

woof

95d30c6

todschmidt changed the title ~~[WIP] Move HassIntent handler code into helpers/intent~~ Move HassIntent handler code into helpers/intent Feb 9, 2018

balloob reviewed Feb 9, 2018

View reviewed changes

Cleanup entity matching

42273fc

homeassistant added the cla-signed label Feb 10, 2018

MartinHjelmare reviewed Feb 10, 2018

View reviewed changes

Update intent.py

a43bc92

balloob approved these changes Feb 11, 2018

View reviewed changes

removed uneeded setup from tests

b2ad2f9

balloob merged commit 26209de into home-assistant:dev Feb 11, 2018

balloob mentioned this pull request Feb 22, 2018

0.64.0 #12609

Merged

home-assistant locked and limited conversation to collaborators May 29, 2018

		return [x for _, _, x in sorted(matches)]


		class ServiceIntentHandler(IntentHandler):

Uh oh!

Conversation

todschmidt commented Feb 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

todschmidt commented Feb 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

todschmidt commented Feb 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

todschmidt commented Feb 4, 2018 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

balloob Feb 9, 2018 •

edited

Loading