Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 77 additions & 34 deletions homeassistant/components/image_processing/facebox.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@
from homeassistant.components.image_processing import (
PLATFORM_SCHEMA, ImageProcessingFaceEntity, ATTR_CONFIDENCE, CONF_SOURCE,
CONF_ENTITY_ID, CONF_NAME, DOMAIN)
from homeassistant.const import (CONF_IP_ADDRESS, CONF_PORT)
from homeassistant.const import (
CONF_IP_ADDRESS, CONF_PORT, CONF_PASSWORD, CONF_USERNAME,
HTTP_BAD_REQUEST, HTTP_OK, HTTP_UNAUTHORIZED)

_LOGGER = logging.getLogger(__name__)

Expand All @@ -36,6 +38,8 @@
PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Required(CONF_IP_ADDRESS): cv.string,
vol.Required(CONF_PORT): cv.port,
vol.Optional(CONF_USERNAME): cv.string,
vol.Optional(CONF_PASSWORD): cv.string,
})

SERVICE_TEACH_SCHEMA = vol.Schema({
Expand Down Expand Up @@ -75,15 +79,36 @@ def parse_faces(api_faces):
return known_faces


def post_image(url, image):
def post_image(url, username, password, image):
"""Post an image to the classifier."""
try:
response = requests.post(
url,
auth=requests.auth.HTTPBasicAuth(username, password),
json={"base64": encode_image(image)},
timeout=TIMEOUT
)
return response
if response.status_code == HTTP_UNAUTHORIZED:
_LOGGER.error("AuthenticationError on %s", CLASSIFIER)
else:
return response

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since we return something here, the other places that this function can exit at are lacking a return statement. If a function returns something, all exits need to return something. Consistency is good.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK

except requests.exceptions.ConnectionError:
_LOGGER.error("ConnectionError: Is %s running?", CLASSIFIER)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok


def teach_file(url, username, password, name, file_path):
"""Teach the classifier a name associated with a file."""
try:
with open(file_path, 'rb') as open_file:
response = requests.post(
url,
auth=requests.auth.HTTPBasicAuth(username, password),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What if username or password is None?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

They are by default and its OK

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't send in auth if username and password are None, that triggers a deprecation warning.

kwargs = {}

if username:
    kwargs['auth'] = requests.auth.HTTPBasicAuth(username, password)

response = request.post(url, **kwargs)

data={ATTR_NAME: name, 'id': file_path},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As this api is not part of home assistant, we shouldn't use constants that are used internally and across home assistant, and might change, ie ATTR_NAME. Using the lone constant also looks weird.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's ok to define and use local constants to be used with the facebox api in this module though. But then do so consistently.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK. My plan is to refactor these back into the image_processing platform

files={'file': open_file})
if response.status_code == HTTP_UNAUTHORIZED:
_LOGGER.error("AuthenticationError on %s", CLASSIFIER)
else:
return response

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See about consistent return.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK

except requests.exceptions.ConnectionError:
_LOGGER.error("ConnectionError: Is %s running?", CLASSIFIER)

Expand All @@ -104,11 +129,22 @@ def setup_platform(hass, config, add_devices, discovery_info=None):
if DATA_FACEBOX not in hass.data:
hass.data[DATA_FACEBOX] = []

ip = config[CONF_IP_ADDRESS]
port = config[CONF_PORT]
username = None
if CONF_USERNAME in config:
username = config[CONF_USERNAME]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

username = config.get(CONF_USERNAME)

This will default username to None if the option is missing from the config.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good to know..!

password = None
if CONF_PASSWORD in config:
password = config[CONF_PASSWORD]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See above.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok


entities = []
for camera in config[CONF_SOURCE]:
facebox = FaceClassifyEntity(
config[CONF_IP_ADDRESS],
config[CONF_PORT],
ip,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please continue the line as far as possible before breaking the line. But keep the first break after the first parenthesis.

This is more in line (😉) with home assistant style.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok

port,
username,
password,
camera[CONF_ENTITY_ID],
camera.get(CONF_NAME))
entities.append(facebox)
Expand Down Expand Up @@ -138,11 +174,13 @@ def service_handle(service):
class FaceClassifyEntity(ImageProcessingFaceEntity):
"""Perform a face classification."""

def __init__(self, ip, port, camera_entity, name=None):
def __init__(self, ip, port, username, password, camera_entity, name=None):
"""Init with the API key and model id."""
super().__init__()
self._url_check = "http://{}:{}/{}/check".format(ip, port, CLASSIFIER)
self._url_teach = "http://{}:{}/{}/teach".format(ip, port, CLASSIFIER)
self._username = username
self._password = password
self._camera = camera_entity
if name:
self._name = name
Expand All @@ -154,7 +192,10 @@ def __init__(self, ip, port, camera_entity, name=None):

def process_image(self, image):
"""Process an image."""
response = post_image(self._url_check, image)
response = post_image(self._url_check,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See above about line break style.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok

self._username,
self._password,
image)
if response is not None:
response_json = response.json()
if response_json['success']:
Expand All @@ -173,34 +214,36 @@ def teach(self, name, file_path):
if (not self.hass.config.is_allowed_path(file_path)
or not valid_file_path(file_path)):
return
with open(file_path, 'rb') as open_file:
response = requests.post(
self._url_teach,
data={ATTR_NAME: name, 'id': file_path},
files={'file': open_file})

if response.status_code == 200:
self.hass.bus.fire(
EVENT_CLASSIFIER_TEACH, {
ATTR_CLASSIFIER: CLASSIFIER,
ATTR_NAME: name,
FILE_PATH: file_path,
'success': True,
'message': None
})

elif response.status_code == 400:
_LOGGER.warning(
"%s teaching of file %s failed with message:%s",
CLASSIFIER, file_path, response.text)
self.hass.bus.fire(
EVENT_CLASSIFIER_TEACH, {
ATTR_CLASSIFIER: CLASSIFIER,
ATTR_NAME: name,
FILE_PATH: file_path,
'success': False,
'message': response.text
})
response = teach_file(self._url_teach,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See above about line break style.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok

self._username,
self._password,
name,
file_path)

if response is not None:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use a guard clause here, by inverting this check and returning if it's true.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok

if response.status_code == HTTP_OK:
self.hass.bus.fire(
EVENT_CLASSIFIER_TEACH, {
ATTR_CLASSIFIER: CLASSIFIER,
ATTR_NAME: name,
FILE_PATH: file_path,
'success': True,
'message': None
})

elif response.status_code == HTTP_BAD_REQUEST:
_LOGGER.warning(
"%s teaching of file %s failed with message:%s",
CLASSIFIER, file_path, response.text)
self.hass.bus.fire(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is the benefit of firing an event also for failure?

@robmarkcole robmarkcole Jul 13, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have an automation to send a notification on teaching, but perhaps I just drop both events (success & fail) here, and just use the logger?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You know better how the events are useful than me. What is the workflow for teaching you are using or imagining for the future? If we lay that down, it'll probably be more clear what is useful.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

One of the issues with the facebox free tier is that if 'forgets' faces if you restart the docker container, so I imagine using the service for allowing automations to reteach a folder of faces. I now think that a log message is fine if teaching fails for some reason

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

persistent notification will be better then a log entry :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Have you got an example of notification?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hass.components.persistent_notification.create(
'Error: no model loaded on your facebox container!',
title=NOTIFICATION_TITLE,
notification_id=NOTIFICATION_ID)

EVENT_CLASSIFIER_TEACH, {
ATTR_CLASSIFIER: CLASSIFIER,
ATTR_NAME: name,
FILE_PATH: file_path,
'success': False,
'message': response.text
})

@property
def camera_entity(self):
Expand Down
17 changes: 15 additions & 2 deletions tests/components/image_processing/test_facebox.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@

from homeassistant.core import callback
from homeassistant.const import (
ATTR_ENTITY_ID, ATTR_NAME, CONF_FRIENDLY_NAME,
CONF_IP_ADDRESS, CONF_PORT, STATE_UNKNOWN)
ATTR_ENTITY_ID, ATTR_NAME, CONF_FRIENDLY_NAME, CONF_PASSWORD,
CONF_USERNAME, CONF_IP_ADDRESS, CONF_PORT, STATE_UNKNOWN)
from homeassistant.setup import async_setup_component
import homeassistant.components.image_processing as ip
import homeassistant.components.image_processing.facebox as fb
Expand All @@ -33,6 +33,8 @@
"faces": [MOCK_FACE]}

MOCK_NAME = 'mock_name'
MOCK_USERNAME = 'mock_username'
MOCK_PASSWORD = 'mock_password'

# Faces data after parsing.
PARSED_FACES = [{ATTR_NAME: 'John Lennon',
Expand Down Expand Up @@ -114,6 +116,17 @@ async def test_setup_platform(hass):
assert hass.states.get(VALID_ENTITY_ID)


async def test_setup_platform_with_auth(hass):
"""Setup platform with one entity and auth."""

valid_config_auth = VALID_CONFIG.copy()
valid_config_auth[ip.DOMAIN][CONF_USERNAME] = MOCK_USERNAME
valid_config_auth[ip.DOMAIN][CONF_PASSWORD] = MOCK_PASSWORD

await async_setup_component(hass, ip.DOMAIN, valid_config_auth)
assert hass.states.get(VALID_ENTITY_ID)


async def test_process_image(hass, mock_image):
"""Test processing of an image."""
await async_setup_component(hass, ip.DOMAIN, VALID_CONFIG)
Expand Down