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

Use the Assistant Library for OK Google hotwording #64

Merged
merged 4 commits into from May 31, 2017

Conversation

Projects
None yet
7 participants
@drigz
Member

drigz commented May 25, 2017

This requires installation of the wheel for the Google Assistant
Library, which includes a closed-source hotword implementation.

drigz added some commits May 25, 2017

Add a new option: --assistant-always-responds
This allows the user to configure responses on IFTTT, so that local
actions have an IFTTT response with the Assistant's voice.
Use the Assistant Library for OK Google hotwording
This requires installation of the wheel for the Google Assistant
Library, which includes a closed-source hotword implementation.

@drigz drigz requested review from proppy and ensonic May 25, 2017

@googlebot googlebot added the cla: yes label May 25, 2017

Show outdated Hide outdated src/main.py
@ensonic

Thanks for giving it a try!

Show outdated Hide outdated scripts/install-deps.sh
Show outdated Hide outdated config/voice-recognizer.ini.default
Show outdated Hide outdated src/actionbase.py
Show outdated Hide outdated src/main.py
Show outdated Hide outdated src/main.py
@@ -204,6 +273,45 @@ def do_recognition(args, recorder, recognizer, player):
time.sleep(1)
class StatusUi(object):

This comment has been minimized.

@ensonic

ensonic May 26, 2017

Collaborator

remove blank line before doc string

@ensonic

ensonic May 26, 2017

Collaborator

remove blank line before doc string

This comment has been minimized.

@drigz

drigz May 30, 2017

Member

The blank-line-before-docstring style is used consistently at the project. However, it seems it's based on an old version of PEP8: hhatto/autopep8#194

I'd prefer to fix in a another PR.

@drigz

drigz May 30, 2017

Member

The blank-line-before-docstring style is used consistently at the project. However, it seems it's based on an old version of PEP8: hhatto/autopep8#194

I'd prefer to fix in a another PR.

Show outdated Hide outdated src/main.py
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io May 30, 2017

Codecov Report

Merging #64 into master will increase coverage by 0.19%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #64      +/-   ##
=========================================
+ Coverage    9.76%   9.95%   +0.19%     
=========================================
  Files           3       3              
  Lines        1720    1727       +7     
  Branches      295     297       +2     
=========================================
+ Hits          168     172       +4     
- Misses       1542    1543       +1     
- Partials       10      12       +2
Impacted Files Coverage Δ
src/actionbase.py 90.32% <66.66%> (-9.68%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 250b8cb...7fd0903. Read the comment docs.

codecov-io commented May 30, 2017

Codecov Report

Merging #64 into master will increase coverage by 0.19%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #64      +/-   ##
=========================================
+ Coverage    9.76%   9.95%   +0.19%     
=========================================
  Files           3       3              
  Lines        1720    1727       +7     
  Branches      295     297       +2     
=========================================
+ Hits          168     172       +4     
- Misses       1542    1543       +1     
- Partials       10      12       +2
Impacted Files Coverage Δ
src/actionbase.py 90.32% <66.66%> (-9.68%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 250b8cb...7fd0903. Read the comment docs.

@drigz

This comment has been minimized.

Show comment
Hide comment
@drigz

drigz May 30, 2017

Member

PTAL

Member

drigz commented May 30, 2017

PTAL

@sheridat

This comment has been minimized.

Show comment
Hide comment
@sheridat

sheridat May 30, 2017

I am a newcomer - so this question might not make a lot of sense but the sentence above "This requires installation of the wheel for the Google Assistant Library, which includes a closed-source hotword implementation" has me worried.

I associate wheel with updating python libraries via pip.

People are aware that there is a new version of the google assistant SDK is available BUT that it is for armv7 capable processors. People are also aware that it supports "hotword"

Many of us are running the AIY software on Pi Zero's and Pi W's which are armv6 based and therefore we cannot use that new version of the the assistant SDK.

My worry is that the change you are making here means that once it is merged - if I was to git pull this repository and run the "install deps script" the google assistant software will be updated and my installation will stop working because the google assistant will want an armv7 processor.

But as we say in england - I may be (entirely) barking up the wrong tree.

sheridat commented May 30, 2017

I am a newcomer - so this question might not make a lot of sense but the sentence above "This requires installation of the wheel for the Google Assistant Library, which includes a closed-source hotword implementation" has me worried.

I associate wheel with updating python libraries via pip.

People are aware that there is a new version of the google assistant SDK is available BUT that it is for armv7 capable processors. People are also aware that it supports "hotword"

Many of us are running the AIY software on Pi Zero's and Pi W's which are armv6 based and therefore we cannot use that new version of the the assistant SDK.

My worry is that the change you are making here means that once it is merged - if I was to git pull this repository and run the "install deps script" the google assistant software will be updated and my installation will stop working because the google assistant will want an armv7 processor.

But as we say in england - I may be (entirely) barking up the wrong tree.

@eduncan911

This comment has been minimized.

Show comment
Hide comment
@eduncan911

eduncan911 May 30, 2017

You have chased the cat up the correct tree.

The SDK update in this PR will restrict installation in ARMv7 going forward.

The binary is specific to detecting the hot words, OK Google and Hey Google. They have rigorously tested it for false positives. But why they kept it closed sourced we don't know. For ARMv7 though, they mention they are leveraging some of the newer CPU instructions to speed things up.

To address your concern though, just don't upgrade. Or, fork it and use your on branch. You can merge changes in, but kept the wheel out of your branch.

eduncan911 commented May 30, 2017

You have chased the cat up the correct tree.

The SDK update in this PR will restrict installation in ARMv7 going forward.

The binary is specific to detecting the hot words, OK Google and Hey Google. They have rigorously tested it for false positives. But why they kept it closed sourced we don't know. For ARMv7 though, they mention they are leveraging some of the newer CPU instructions to speed things up.

To address your concern though, just don't upgrade. Or, fork it and use your on branch. You can merge changes in, but kept the wheel out of your branch.

@drigz

This comment has been minimized.

Show comment
Hide comment
@drigz

drigz May 30, 2017

Member

We could remove google-assistant-library from requirements.txt so that it's still usable on ARMv6, (maybe ok_google_requirements.txt?) and amend the error message to say "ok-google triggering is not available on your platform" for ARMv6 etc, and "Please pip install -r ok_google_requirements.txt" for ARMv7. What do you think?

Member

drigz commented May 30, 2017

We could remove google-assistant-library from requirements.txt so that it's still usable on ARMv6, (maybe ok_google_requirements.txt?) and amend the error message to say "ok-google triggering is not available on your platform" for ARMv6 etc, and "Please pip install -r ok_google_requirements.txt" for ARMv7. What do you think?

@sheridat

This comment has been minimized.

Show comment
Hide comment
@sheridat

sheridat May 30, 2017

ummmmmhhhhhhhhhhh. Is it realistic for the newcomers to start using Git to maintain their own fork/branch and maintain it. We are struggling with how to update it. A compromise such as that outlined by Drigz seems a better solution and only a little inconvenient for people using the armv7 processor. I would hate to think that some kids using this on a Zero or W suddenly need to become Git gurus.

sheridat commented May 30, 2017

ummmmmhhhhhhhhhhh. Is it realistic for the newcomers to start using Git to maintain their own fork/branch and maintain it. We are struggling with how to update it. A compromise such as that outlined by Drigz seems a better solution and only a little inconvenient for people using the armv7 processor. I would hate to think that some kids using this on a Zero or W suddenly need to become Git gurus.

@eduncan911

This comment has been minimized.

Show comment
Hide comment
@eduncan911

eduncan911 May 30, 2017

I know the struggles quite well, as I am not a Python guru myself and stumble through most of this.

I think this discussion really belongs over at the Assistant SDK repo though, not in this PR:

https://github.com/googlesamples/assistant-sdk-python

You can read a marketing overview of what the Assistant does (or will do soon):

https://developers.google.com/assistant/sdk/

Basically, the Assistant SDK does a lot more than just hot wording. It is what does the interrupting of the vocal commands, for example. Removing it would pretty much render this "Voice Recognizer" project dead and useless. The "hot wording" is just one feature in a set of a much larger feature set that this repo is leveraging.

This repo is basically just a wrapper framework with some triggers and actions to leverage the Assistant SDK core library.

eduncan911 commented May 30, 2017

I know the struggles quite well, as I am not a Python guru myself and stumble through most of this.

I think this discussion really belongs over at the Assistant SDK repo though, not in this PR:

https://github.com/googlesamples/assistant-sdk-python

You can read a marketing overview of what the Assistant does (or will do soon):

https://developers.google.com/assistant/sdk/

Basically, the Assistant SDK does a lot more than just hot wording. It is what does the interrupting of the vocal commands, for example. Removing it would pretty much render this "Voice Recognizer" project dead and useless. The "hot wording" is just one feature in a set of a much larger feature set that this repo is leveraging.

This repo is basically just a wrapper framework with some triggers and actions to leverage the Assistant SDK core library.

@sheridat

This comment has been minimized.

Show comment
Hide comment
@sheridat

sheridat May 30, 2017

I see - and I can see the clamour for new features.

sheridat commented May 30, 2017

I see - and I can see the clamour for new features.

@drigz

This comment has been minimized.

Show comment
Hide comment
@drigz

drigz May 31, 2017

Member

@ensonic, the library is now optional. Users who git pull and try to use -T ok-google will get a message saying the dep is only for Pi 2/3, and how to install it.

WDYT?

Member

drigz commented May 31, 2017

@ensonic, the library is now optional. Users who git pull and try to use -T ok-google will get a message saying the dep is only for Pi 2/3, and how to install it.

WDYT?

@drigz drigz merged commit 5b2cd9c into google:master May 31, 2017

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sheridat

This comment has been minimized.

Show comment
Hide comment
@sheridat

sheridat Jun 1, 2017

Hi, I've closed my issue above

sheridat commented Jun 1, 2017

Hi, I've closed my issue above

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