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

TensorFlow image_processing component #17795

Merged
merged 8 commits into from Nov 2, 2018

Conversation

@hunterjm
Contributor

hunterjm commented Oct 25, 2018

Description:

Adding a TensorFlow Object Detection image_processing component to run either pre-trained or custom built machine learning object detection models through TensorFlow and expose the results to Home Assistant.

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

Example entry for configuration.yaml (if applicable):

# Basic configuration.yaml entry
image_processing:
  - platform: tensorflow
    source:
      - entity_id: camera.local_file
    model:
      graph: /home/homeassistant/.homeassistant/frozen_inference_graph.pb
      labels: /home/homeassistant/.homeassistant/tensorflow/object_detection/data/mscoco_label_map.pbtxt
      model_dir: /home/homeassistant/.homeassistant/tensorflow

# Advanced configuration.yaml entry
image_processing:
  - platform: tensorflow
    source:
      - entity_id: camera.local_file
    file_out:
      - "/tmp/{{ camera_entity.split('.')[1] }}_latest.jpg"
      - "/tmp/{{ camera_entity.split('.')[1] }}_{{ now().strftime('%Y%m%d_%H%M%S') }}.jpg"
    model:
      graph: /home/homeassistant/.homeassistant/frozen_inference_graph.pb
      labels: /home/homeassistant/.homeassistant/tensorflow/object_detection/data/mscoco_label_map.pbtxt
      model_dir: /home/homeassistant/.homeassistant/tensorflow
      categories:
        - category: person
          area:
            # Exclude top 10% of image
            top: 0.1
            # Exclude right 15% of image
            right: 0.85
        - car
        - truck

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:

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

  • New dependencies have been added to the REQUIREMENTS variable.
  • New dependencies are only imported inside functions that use them.
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

@hunterjm hunterjm requested a review from home-assistant/docker as a code owner Oct 25, 2018

@wafflebot wafflebot bot added the in progress label Oct 25, 2018

@hunterjm hunterjm referenced this pull request Oct 25, 2018

Merged

TensorFlow image_processing component #7083

2 of 2 tasks complete

@hunterjm hunterjm force-pushed the hunterjm:feature/tensorflow branch from 15b2029 to dc517b8 Oct 26, 2018

@robmarkcole

This comment has been minimized.

Contributor

robmarkcole commented Oct 26, 2018

@hunterjm great work on this. Couple of comments:

  1. Re bounding box, in Facebox we are using bounding_box rather than box, My plan was to roll this const into the image_processing component. I think it would be good to decide a convention. Also is the score use here different in intent from the confidence used by Facebox etc?

  2. On a related point, it would be good to breakout the bounding box display as a service which all image processing components could use, I was actually about to start work on a PR to do this :-)

  3. On macosx mojave, I get the log error Your CPU supports instructions that this TensorFlow binary was not compiled to use: AVX2 FMA. This error can be suppressed by:

import os
os.environ['TF_CPP_MIN_LOG_LEVEL'] = '2'

ps accidentally pushed commit to address that, meant to create a PR

Cheers

@robmarkcole

This comment has been minimized.

Contributor

robmarkcole commented Oct 26, 2018

Currently the state data will require parsing:

{ "dog": [ { "score": 99.44924116134644, "box": [ 0.0563804991543293, 0.18027611076831818, 0.6681657433509827, 0.6643975973129272 ] } ], "chair": [ { "score": 96.00539803504944, "box": [ 0.015583686530590057, 0, 0.35551923513412476, 0.29479289054870605 ] } ], "potted plant": [ { "score": 94.95822787284851, "box": [ 0.10585738718509674, 0.5190292596817017, 0.5562731027603149, 0.9891135692596436 ] } ], "bowl": [ { "score": 86.03321313858032, "box": [ 0.43573233485221863, 0.16289281845092773, 0.8424050807952881, 0.7881014943122864 ] } ] }

In Facebox I use a little trick to breakout the keys and format the confidence to 2dp here.

@robmarkcole

This comment has been minimized.

Contributor

robmarkcole commented Oct 26, 2018

With file_out configured I get the exception:

  File "/Users/robincole/Documents/Github/home-assistant/homeassistant/components/image_processing/tensorflow.py", line 334, in process_image
    self._save_image(image, matches, paths)
  File "/Users/robincole/Documents/Github/home-assistant/homeassistant/components/image_processing/tensorflow.py", line 240, in _save_image
    if self._category_areas[category] != [0, 0, 1, 1]:
KeyError: 'dog'
    file_out:
      - "/Users/robincole/.homeassistant/images/tensorflow_latest.jpg"

If I use the camera entity_id in the template I get an error: Error rendering template: UndefinedError: 'camera' is undefined

@hunterjm

This comment has been minimized.

Contributor

hunterjm commented Oct 26, 2018

@robmarkcole - There is a lot here, so I will break it down by section:

in Facebox we are using bounding_box rather than box ... is the score use here different in intent from the confidence used by Facebox etc?

Score has the same intent. I have updated the code to reflect and keep standard. If we want to standardize bounding box, we can as well. TensorFlow returns the bounding_box as [top, left, bottom, right]. Is that how Facebox defines it as well?

It would be good to breakout the bounding box display as a service which all image processing components could use.

I agree, but I think we can do that (potentially together) as a refactor PR instead of making this one larger in scope.

I get the log error Your CPU supports instructions that this TensorFlow binary was not compiled to use: AVX2 FMA

Thanks for the fix. It's a harmless warning, but can see how it could confuse the standard user.

In Facebox I use a little trick to breakout the keys and format the confidence to 2dp here.

I looked at what you were doing in Facebox, and drew some inspiration from there. This one is a little peculiar since each category could have multiple detections. With Facebox, you are breaking out a known matched face, and there can only be one of those. In this scenario, there can be more than one person or car detected. I'm happy to revisit the data structure, but I went with what I thought was most intuitive at the time.

With file_out configured I get [an] exception

Great catch! I forgot to test file_out without filtered categories. Fixed in 8bb9cd3

If I use the camera entity_id in the template I get an error: Error rendering template: UndefinedError: 'camera' is undefined.

You are probably using camera.entity_id, which isn't known by the template. The documentation lets people know to use camera_entity which is the entity string (i.e. camera.backyard). I'm happy to change if you think there will be confusion.

@robmarkcole

This comment has been minimized.

Contributor

robmarkcole commented Oct 27, 2018

Bounding box

@hunterjm I like the convention for bounding box you propose ([top, left, bottom, right]) relative distance from image boundaries, and suggest we stick with that and refactor the other components later. We can then just provide a link to the convention for future reference.

Data display

Re displaying the returned data, you currently provide:

{'dog': [{'confidence': 99.45,
   'bounding_box': [0.0563804991543293,
    0.18027611076831818,
    0.6681657433509827,
    0.6643975973129272]}],
 'chair': [{'confidence': 96.01,
   'bounding_box': [0.015583686530590057,
    0,
    0.35551923513412476,
    0.29479289054870605]}],

What I propose is that in addition to this full info, we provide a summary:

{'dog': : 99.45,
 'chair': 96.01,

This is the approach in Facebox, and makes it easy to see whats been identified from the HA front end:

Templating

Re the templating of the filename, I'm still confused and getting exceptions, what should my config be for the following?

image

The description for the snapshot service I find much clearer, and I have:

    file_out:
       - '/Users/robincole/.homeassistant/images/my_camera_{{ now().strftime("%Y%m%d-%H%M%S") }}.jpg'
@arsaboo

This comment has been minimized.

Contributor

arsaboo commented Oct 27, 2018

@robmarkcole We should try to be close to Facebox, but it is not a perfect match as there can be more than one object detected. We can add a summary, but I feel that it is redundant. We can use confidence: 75 in the configuration to exclude matches below a certain threshold (@hunterjm we should add that as an optional configuration setting).

For file_out, I just tried the following and it works fine for me:

    file_out:
      - "/home/homeassistant/.homeassistant/downloads/camera/test_{{ now().strftime('%Y%m%d_%H%M%S') }}.jpg"

Although, ideally you would like to include camera_entity to identify the camera in the filename (especially if you have multiple cameras).

@robmarkcole

This comment has been minimized.

Contributor

robmarkcole commented Oct 27, 2018

DEFAULT_CONFIDENCE is already part of the platform :)

I think a summary is required, as something like this isn't very helpful

@hunterjm

This comment has been minimized.

Contributor

hunterjm commented Oct 28, 2018

@robmarkcole - I hear you on the summary. I still don't think having key = category and value = confidence is the right take though. In the example you provided, there were 2 cups detected. What about quantity detected, like this (taken from your example here):

{
  "cup": 2,
  "fork": 1,
  "dining table": 1,
  "pizza": 1
}
@robmarkcole

This comment has been minimized.

Contributor

robmarkcole commented Oct 28, 2018

@hunterjm yes I see what you mean by 'multiple objects being detected' now, your suggestion looks great then.

Next people will want to create automations based on detections, the way to do this is to add events, like here. Possibly beyond the scope of this PR.

@hunterjm hunterjm force-pushed the hunterjm:feature/tensorflow branch from 8bb9cd3 to 5f83b00 Oct 29, 2018

@houndci-bot

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'
@houndci-bot

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'
@houndci-bot

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'
@hunterjm

This comment has been minimized.

Contributor

hunterjm commented Oct 29, 2018

@robmarkcole - Updated to show the summary and resolve merge conflicts.

What are the next steps here?

@robmarkcole

This comment has been minimized.

Contributor

robmarkcole commented Oct 29, 2018

@hunterjm did you decide to drop os.environ['TF_CPP_MIN_LOG_LEVEL'] = '2' then?
Next step is to add unit tests, and hit 95% coverage. You should be able to steal from existing tests.

@hunterjm

This comment has been minimized.

Contributor

hunterjm commented Oct 29, 2018

@robmarkcole - I did not mean to. It got dropped in the rebase.

Dockerfile Outdated
# Uninstall enum34 because some dependencies install it but breaks Python 3.4+.
# See PR #8103 for more info.
RUN pip3 install --no-cache-dir -r requirements_all.txt && \
pip3 install --no-cache-dir mysqlclient psycopg2 uvloop cchardet cython
pip3 install --no-cache-dir mysqlclient psycopg2 uvloop cchardet cython opencv-python-headless

This comment has been minimized.

@balloob

balloob Nov 2, 2018

Member

I'm always a bit wary if we need to install things for every single user of Home Assistant. Why can't we install this on demand?

This comment has been minimized.

@balloob

balloob Nov 2, 2018

Member

It looks like it will also work without opencv. We should remove this.

This comment has been minimized.

@hunterjm

hunterjm Nov 2, 2018

Contributor

The thought here was to make it as easy as possible for those using the official docker image. OpenCV is pre-built in the Hass.io images here, though from source. The pip package makes it easier to install.

I was aiming for consistency.

model_dir = model_config.get(CONF_MODEL_DIR)
# append custom model path to sys.path
sys.path.append(model_dir)

This comment has been minimized.

@balloob

balloob Nov 2, 2018

Member

Ehhhrrrrr

We should be able to specify the paths to look to tensorflow?

This comment has been minimized.

@hunterjm

hunterjm Nov 2, 2018

Contributor

There are some requirements that can not be installed via pip. The object detection portion of the library is not included in the standard TensorFlow wheel. This is to include the protobuf models and util functions created as a part of the tensorflow install shell script.

@michaelarnauts

This comment has been minimized.

Contributor

michaelarnauts commented Nov 2, 2018

These binaries/tensorflow should really be in a different container if you want to use them. Home Assistant should just contain the glue to interface with it, but not the actual code.

Show resolved Hide resolved Dockerfile Outdated
@homeassistant

This comment has been minimized.

homeassistant commented Nov 2, 2018

Hi @rv-jhunter,

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!

@hunterjm hunterjm force-pushed the hunterjm:feature/tensorflow branch from da02698 to c04a105 Nov 2, 2018

@hunterjm hunterjm force-pushed the hunterjm:feature/tensorflow branch from 4736c4d to 6005ddd Nov 2, 2018

hunterjm added some commits Nov 2, 2018

do not use deps folder as default, as it should only be managed by HA…
…. Update to have tensorflow in root config directory
@balloob

balloob approved these changes Nov 2, 2018

@balloob balloob merged commit 45484ba into home-assistant:dev Nov 2, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.002%) to 93.063%
Details

@wafflebot wafflebot bot removed the in progress label Nov 2, 2018

@hunterjm hunterjm deleted the hunterjm:feature/tensorflow branch Nov 7, 2018

@balloob balloob referenced this pull request Nov 9, 2018

Merged

0.82 #18335

@michaelarnauts michaelarnauts referenced this pull request Nov 10, 2018

Open

Docker size #103

@loxK

This comment has been minimized.

loxK commented Nov 14, 2018

It is great to see this added, well done. But it won't replace my setup for security.

Here is what would be awesome:

  • first do a simple (cpu friendly) motion detection
  • if motion is detected, try a person detection (tensorflow)
  • if person (or other object) is detected wait x configurable seconds before doing any tensorflow object detection
  • add option to stop tensorflow processing when at least x (configurable) person is detected

I have motion + a custom script (PHP) that does that, would love to replace it with home assistant.

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Nov 14, 2018

Please open an issue if you suspect a bug.

If you want to suggest an enhancement please open a feature request in the Feature Requests section of our community forum.

Merged PRs should not be used for enhancement discussion or bug reports. If you've found a bug it's ok to make a review with inline comments and link to an issue that reports the bug.

Thanks!

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Nov 14, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.