Add Facebox teach service by robmarkcole · Pull Request #14998 · home-assistant/core

robmarkcole · 2018-06-16T18:07:35Z

Description:

This PR adds the service image_processing.facebox_teach_face that can be used to teach Facebox faces, as described in this blog post. Example valid service data is:

{
  "entity_id": "image_processing.facebox_local_file",
  "name": "superman",
  "file_path": "/Users/robincole/.homeassistant/images/superman_1.jpeg"
}

An image_processing.teach_classifier event is fired for each service call, providing feedback on whether teaching has been successful or unsuccessful. In the unsuccessful case, the message field of the event_data will contain info on the cause of failure, and a warning is also published in the HA logs.

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

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

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

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

robmarkcole · 2018-06-16T18:58:09Z

This PR adds functions to both validate that a file_path is valid, and that a file is an image file. Are these methods available already in the HA API?

robmarkcole · 2018-06-17T05:58:10Z

OK I removed the text of image type as the API can actually handle a wide range of image types, and will return an error if an invalid type is posted, to that case is handled.

houndci-bot · 2018-06-17T06:13:30Z

+    try:
+        cv.isfile(file_path)
+        return True
+    except:


do not use bare except'

houndci-bot · 2018-06-17T06:13:30Z

 """
 import base64
 import logging
+import os


'os' imported but unused

houndci-bot · 2018-06-17T06:29:25Z

 import pytest
 import requests
 import requests_mock
+from unittest import mock


'unittest.mock' imported but unused

pvizeli · 2018-06-18T07:57:26Z

You need use: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/core.py#L1064
hass.config.is_allowed_path

robmarkcole · 2018-06-18T18:50:03Z

ping @pvizeli comment addressed

MartinHjelmare · 2018-06-20T08:35:06Z

+            facebox.teach(name, file_path)
+            return True
+
+        hass.services.register(


This won't work. Only one service is registered per domain and service name key. Move this out of the for loop.

MartinHjelmare · 2018-06-20T08:36:29Z

+            """Teach a facebox a name."""
+            name = call.data.get(ATTR_NAME)
+            file_path = call.data.get(FILE_PATH)
+            facebox.teach(name, file_path)


We should lookup which entity should be used. See other platforms that register services for examples.

Move the service handler out of the for loop.

Something like hass.data[DOMAIN].facebox.teach(name, file_path) I'm guessing

More like this:
https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/media_player/sonos.py#L200

Appears you use SonosData to access all SonosDevice objects, so I gather I will need an equivalent to access my FaceClassifyEntity objects in the service handler?

Yes, you need to store them in hass.data since a platform can be setup multiple times, adding more entities each time.

MartinHjelmare · 2018-06-20T08:39:21Z

 CLASSIFIER = 'facebox'
+EVENT_CLASSIFIER_TEACH = 'image_processing.teach_classifier'
+FILE_PATH = 'file_path'
+SERVICE_FACEBOX_TEACH_FACE = 'facebox_teach_face'


Simplify constant name to SERVICE_TEACH_FACE. Don't change the string though.

MartinHjelmare · 2018-06-23T10:29:24Z

We can replace the decorators with these fixtures:

@pytest.fixture
def mock_isfile():
    """Mock os.path.isfile."""
    with patch('os.path.isfile', Mock(return_value=True)) as _mock_isfile:
        yield _mock_isfile


@pytest.fixture
def mock_access():
    """Mock os.access."""
    with patch('os.access', Mock(return_value=True)) as _mock_access:
        yield _mock_access

robmarkcole · 2018-06-24T18:54:29Z

@MartinHjelmare I don't understand why but test_teach_service fails to setup the component, despite being identical to the other tests which do successfully perform setup, any ideas?

robmarkcole · 2018-06-30T19:57:20Z

@MartinHjelmare the line which is causing the setup to fail on the final test is:

async def test_teach_service(hass, mock_image, mock_isfile, mock_access):

If I remove mock_isfile, mock_access then setup is successful, but only when I also move that test before test_setup_platform_with_name. So it appears there are a couple of funny things going on?

@dgomes @OttoWinter

houndci-bot · 2018-06-30T19:57:54Z

    assert state.attributes.get('matched_faces') == {}


+async def test_teach_service(hass, mock_image): # mock_isfile, mock_access


at least two spaces before inline comment

Removes the fixtures which were causing `setup` to fail, replace with `@patch`

Stale

robmarkcole · 2018-07-09T19:46:10Z

ping @pvizeli @MartinHjelmare

MartinHjelmare · 2018-07-09T22:56:44Z


+@patch('os.access', Mock(return_value=True))
+@patch('os.path.isfile', Mock(return_value=True))
+async def test_teach_service(hass, mock_image):


This test is never awaited. Please run the tests locally, eg with script/lazytox.py. You'll see the warning in the pytest warning summary.

MartinHjelmare

I've fixed the test. We were patching the wrong things and didn't patch the call to open file.

MartinHjelmare · 2018-07-09T23:40:55Z

Can be merged when build passes.

MartinHjelmare · 2018-07-10T01:11:27Z

Merging. Coverage calc is not accurate.

robmarkcole · 2018-07-10T04:52:41Z

Many thanks @MartinHjelmare :-)

* Adds service * Address pylint * Update facebox.py * patch tests * Update facebox.py * Update test_facebox.py * Update facebox.py * Update facebox.py * Update facebox.py * Update test_facebox.py * Update test_facebox.py * Update facebox.py * Update facebox.py * Update facebox.py * Update facebox.py * Adds total_matched_faces * Update test_facebox.py * Update facebox.py * Update test_facebox.py * Update test_facebox.py * Remove fixtures Removes the fixtures which were causing `setup` to fail, replace with `@patch` * Fix teach service test and lint issues

Adds service

0a85a53

homeassistant added cla-signed platform: image_processing.facebox labels Jun 16, 2018

ghost assigned robmarkcole Jun 16, 2018

ghost added the in progress label Jun 16, 2018

robmarkcole requested review from dgomes and pvizeli June 16, 2018 18:10

robmarkcole mentioned this pull request Jun 16, 2018

Adds facebox teach service home-assistant/home-assistant.io#5553

Merged

2 tasks

Address pylint

c9d68f3

homeassistant added the cla-signed label Jun 16, 2018

robmarkcole added 5 commits June 16, 2018 22:07

Update facebox.py

d503b4c

patch tests

f529b7d

Update facebox.py

d9292a7

Update test_facebox.py

d4ef2ff

Update facebox.py

2eed0d9

Update facebox.py

8b1791e

houndci-bot reviewed Jun 17, 2018

View reviewed changes

robmarkcole added 2 commits June 17, 2018 07:29

Update facebox.py

a1751ec

Update test_facebox.py

4664c93

houndci-bot reviewed Jun 17, 2018

View reviewed changes

Update test_facebox.py

d87bddd

robmarkcole requested a review from fabaff June 18, 2018 06:06

Update facebox.py

0179d37

homeassistant added the cla-signed label Jun 18, 2018

MartinHjelmare previously requested changes Jun 20, 2018

View reviewed changes

Update facebox.py

863ebd6

Update test_facebox.py

cf6497a

homeassistant added the cla-signed label Jun 24, 2018

Update test_facebox.py

6a23b19

homeassistant added the cla-signed label Jun 30, 2018

houndci-bot reviewed Jun 30, 2018

View reviewed changes

Remove fixtures

5bf7a13

Removes the fixtures which were causing `setup` to fail, replace with `@patch`

MartinHjelmare requested changes Jul 9, 2018

View reviewed changes

Fix teach service test and lint issues

49a33f5

homeassistant added the cla-signed label Jul 9, 2018

ghost assigned MartinHjelmare Jul 9, 2018

MartinHjelmare approved these changes Jul 9, 2018

View reviewed changes

MartinHjelmare changed the title ~~Adds Facebox teach service~~ Add Facebox teach service Jul 9, 2018

MartinHjelmare merged commit df8c594 into home-assistant:dev Jul 10, 2018

ghost removed the in progress label Jul 10, 2018

robmarkcole deleted the facebox-teach-service branch July 10, 2018 04:52

balloob mentioned this pull request Jul 20, 2018

0.74 #15570

Merged

home-assistant locked and limited conversation to collaborators Dec 10, 2018

ghost added integration: facebox and removed platform: image_processing.facebox labels Mar 21, 2019

		assert state.attributes.get('matched_faces') == {}


		async def test_teach_service(hass, mock_image): # mock_isfile, mock_access

Uh oh!

Conversation

robmarkcole commented Jun 16, 2018 • edited by fabaff Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Checklist:

Uh oh!

robmarkcole commented Jun 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robmarkcole commented Jun 17, 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!

pvizeli commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robmarkcole commented Jun 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Jun 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

MartinHjelmare commented Jun 23, 2018

Uh oh!

robmarkcole commented Jun 24, 2018

Uh oh!

robmarkcole commented Jun 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robmarkcole commented Jul 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare commented Jul 9, 2018

Uh oh!

MartinHjelmare commented Jul 10, 2018

Uh oh!

robmarkcole commented Jul 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

robmarkcole commented Jun 16, 2018 •

edited by fabaff

Loading

robmarkcole commented Jun 16, 2018 •

edited

Loading

pvizeli commented Jun 18, 2018 •

edited

Loading

MartinHjelmare Jun 20, 2018 •

edited

Loading