Skip to content
This repository has been archived by the owner on Feb 9, 2023. It is now read-only.

Manage a directory of user scripts that respond to voice commands. #78

Closed
wants to merge 24 commits into from

Conversation

pjbroad
Copy link

@pjbroad pjbroad commented Jun 4, 2017

I think I've responded and made appropriate changes for all but one comment from @ensonic. The remaining discussion point was about whether to limit user scripts to python and load them as modules, or keep the current implementation that uses a sub-processes. I've kept the sub-process method for now.

@codecov-io
Copy link

codecov-io commented Jun 4, 2017

Codecov Report

Merging #78 into master will increase coverage by 2.81%.
The diff coverage is 74.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
+ Coverage   65.83%   68.65%   +2.81%     
==========================================
  Files           2        3       +1     
  Lines         161      252      +91     
  Branches       16       33      +17     
==========================================
+ Hits          106      173      +67     
- Misses         52       66      +14     
- Partials        3       13      +10
Impacted Files Coverage Δ
src/action.py 59.09% <33.33%> (-0.91%) ⬇️
src/user_scripts.py 71.83% <71.83%> (ø)
src/actionbase.py 89.79% <89.47%> (-0.53%) ⬇️

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 8024b60...ad49b0f. Read the comment docs.

@sheridat
Copy link

Hi, Is this near to being merged into the master? Will something be needed to explain to people how they should go about adding their own actions once this is introduced?

@pjbroad
Copy link
Author

pjbroad commented Jul 28, 2017

@sheridat I'd be happy to provide the explanation but I'm not sure this change will get merged. The change has sat here quietly for sometime, I've just kept it up to date with other changes. We really need a comment from @ensonic. I think I answered all their questions except the question of making the user scripts plugin python modules, more directly integrated. If that is the reason this change has not been excepted then I will happily change the approach as suggested. What do you think @ensonic ?

@sheridat
Copy link

@pjbroad Thanks for replying - given your flexibility on how this is implemented it would be great if @ensonic could agree a way forward - I for one have held off implementing some of my home automation commands until they can be implemented outside of /src/action.py in a supported manner.
@ensonic is there a way forward that you could agree to?

@eduncan911
Copy link

@pjbroad my only feedback would to be to include more instructions to how to use the framework along with full examples and/or framework code someone can just copy and paste (cause most hackers may just want copy-n-paste, and tweak a command line call).

@pjbroad @sheridat though I am a big fan of polyglot code when it comes to plugins and don't like being restricted to only Python, my initial opinion would be to keep it all Python with the minimal plugin example framework, as it is pretty common. Also, it keeps it all in a single thread, minimizing overhead.

However: I do see a valid argument for shelling out to a process for each plugin: requirements.txt. we don't want people polluting the Google requirements file with their own, as they may have other requirements that conflict. Trigging a plug-in to shell out afford the flexibility for someone to create their own VIRTUALENV to load and use.

Could we have the option to do both? Instead of forcing a plug-in to execute a command, they could just call an existing module?

This is the approach I've taken with my plugins that do utilize major "process overhead" by running VLC and making several web requests.

The ISS version I am working on uses Golang code I've written and calls several other APIs (not checked in yet though).

I also suspect most people will also be "going out of process" to run processes like this anyways. But, I am free to use the existing python code or shell out and do my own.


With that said, I have developed a few plugins myself with the existing code using a few simple rules:

  • Place all new code in src/actions/ (will have to create it)
  • Add two lines of code per plugin to src/action.py: one to import the file, and one to activate.

Following this simple one liner approach has not conflicted yet with any of the merges from the origin when updating.

For example, here's how I added YouYube playback:

eduncan911@d19921d

You can see the plugins I've written here:

https://github.com/eduncan911/aiyprojects-raspbian/tree/master/src/actions

To see how to implement them, view my commits to the master branch. I was writing a few draft posts to explain how to implement them, just haven't gotten around to finishing them.

@sheridat
Copy link

@eduncan911 no argument from me - i just want it to be kept simple - some of us are very inexperienced. I've looked at your repo a couple of times for ideas regarding how to do this.
@pjbroad - I have to admit to having been at your github site as well. Seems we share some additional interests - I am about to go and steal your pi sensors/kodi and AIY scripts to drive them. Great stuff.

@ensonic
Copy link
Contributor

ensonic commented Aug 1, 2017

Sorry for not giving feedback on this sooner. We've been holding off on merging this as we worked out the plan for this project. Our main concern was that we didn't want to add complexity - we originally intended this project as a simple example for makers. However, @enetor has been working on modules that replace the monolithic Voice Recognizer, which we'll upload to PyPI. With these, @pjbroad would be able to keep the plugin system in a his repo, while the main repo hosts the module source and some very simple examples.

Any thoughts?

@pjbroad
Copy link
Author

pjbroad commented Aug 8, 2017

@ensonic Thanks for the information. It's difficult to understand how this would work from what you said but I guess it will become clear when you publish the changes.

@sheridat
Copy link

sheridat commented Aug 9, 2017

@eduncan911 Eric I've been successfully using @pjbroad's solution (as well as some of Paul's other repos). I've copied your AIY repo but all three scripts in there need additional software installed which I want to avoid.
When you have a few minutes could you sketch out a simple example that I can create that embodies your technique?

@sheridat
Copy link

@eduncan911 Eric managed to knock together something to test this so forget above request

@drigz
Copy link
Member

drigz commented Oct 10, 2017

Since the new direction of this project is to provide a support library (along with demos of how to use it) rather than a full-featured solution, I'm going to close this PR. There are still users interested in plugin functionality (eg mpember's comment here), so if anyone wants to provide plugin support, they could have another project that uses the aiy module (eg via fork or submodule, since it's not on PyPI).

PS: @pjbroad, thanks for the time you spent on this PR! I'm sorry we were unclear about the direction of the project before.

@drigz drigz closed this Oct 10, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants