Skip to content

TensorFlow image_processing component#17795

Merged
balloob merged 8 commits intohome-assistant:devfrom
hunterjm:feature/tensorflow
Nov 2, 2018
Merged

TensorFlow image_processing component#17795
balloob merged 8 commits intohome-assistant:devfrom
hunterjm:feature/tensorflow

Conversation

@hunterjm
Copy link
Copy Markdown
Member

@hunterjm 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.

Comment thread homeassistant/components/image_processing/tensorflow.py Outdated
Comment thread homeassistant/components/image_processing/tensorflow.py Outdated
Comment thread homeassistant/components/image_processing/tensorflow.py Outdated
Comment thread homeassistant/components/image_processing/tensorflow.py Outdated
Comment thread homeassistant/components/image_processing/tensorflow.py Outdated
Comment thread homeassistant/components/image_processing/tensorflow.py Outdated
Comment thread homeassistant/components/image_processing/tensorflow.py Outdated
Comment thread homeassistant/components/image_processing/tensorflow.py Outdated
Comment thread homeassistant/components/image_processing/tensorflow.py Outdated
Comment thread homeassistant/components/image_processing/tensorflow.py Outdated
@robmarkcole
Copy link
Copy Markdown
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

@ghost ghost assigned robmarkcole Oct 26, 2018
@robmarkcole
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Member Author

@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
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Member Author

@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
Copy link
Copy Markdown
Contributor

@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.

Copy link
Copy Markdown

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

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

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'

Copy link
Copy Markdown

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

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

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'

Copy link
Copy Markdown

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

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

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
Copy link
Copy Markdown
Member Author

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

What are the next steps here?

@robmarkcole
Copy link
Copy Markdown
Contributor

@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
Copy link
Copy Markdown
Member Author

hunterjm commented Oct 29, 2018

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

Comment thread Dockerfile Outdated
# 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
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.

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?

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 looks like it will also work without opencv. We should remove this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Comment thread homeassistant/components/image_processing/tensorflow.py Outdated
model_dir = model_config.get(CONF_MODEL_DIR)

# append custom model path to sys.path
sys.path.append(model_dir)
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.

Ehhhrrrrr

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Comment thread virtualization/Docker/setup_docker_prereqs Outdated
@michaelarnauts
Copy link
Copy Markdown
Contributor

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.

Comment thread Dockerfile Outdated
@homeassistant
Copy link
Copy Markdown
Contributor

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!

Comment thread homeassistant/components/image_processing/tensorflow.py Outdated
Comment thread homeassistant/components/image_processing/tensorflow.py Outdated
Comment thread homeassistant/components/image_processing/tensorflow.py Outdated
@balloob balloob merged commit 45484ba into home-assistant:dev Nov 2, 2018
@ghost ghost removed the in progress label Nov 2, 2018
@hunterjm hunterjm deleted the feature/tensorflow branch November 7, 2018 01:30
@balloob balloob mentioned this pull request Nov 9, 2018
@loxK
Copy link
Copy Markdown

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
Copy link
Copy Markdown
Member

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants