Modules installer #174

alexsiri7 · 2014-09-14T16:27:04Z

This is a simple modules installer (can't handle multiple files, or requirements yet)

It connects to http://jaspermoduleshub.herokuapp.com/

I added a walk_packages on brain.py so that modules can be installed in subfolders.

Holzhaus · 2014-09-15T18:56:25Z

I didn't have the time to take a closer look yet, but I already have to nag (silly me 😜 ):

Please do not use the minus sign (-) in a module name (which will make it very hard to import). According to PEP 8 there may be an underscore (_), but IMHO we should try to avoid that, too.

I'd suggest...uhmmm..... modinstaller.py (?)

alexsiri7 · 2014-09-15T19:49:49Z

Ok, I changed the name to the module.

alexsiri7 · 2014-09-23T06:43:43Z

@Holzhaus @crm416 @shbhrsaha were you able to take a look at this? What do you think?

alexsiri7 · 2014-09-24T10:53:49Z

Holzhaus · 2014-09-24T11:02:57Z

IMHO we can add sth like this to Jasper, but we can also wait for v2. Depending in what @crm416 and @shbhrsaha say, we can either start testing for v1 or we change the code to play nice with v2's Plugin system and postpone merging.

Holzhaus · 2014-09-24T12:37:54Z

client/brain.py

@@ -43,10 +43,10 @@ def get_modules(cls):
        """

        module_locations = [jasperpath.PLUGIN_PATH]
-        module_names = [name for loader, name, ispkg in pkgutil.iter_modules(module_locations)]
+        module_names = [name for loader, name, ispkg in pkgutil.walk_packages(module_locations, prefix='modules.')]


This should be part of a separate PR and can be merged regardless of the plugin installer.

Yes it could. But it was necessary for this one.

Just make a new PR containing just the changes to brain.py and I'll merge it today.

alexsiri7 · 2014-09-24T12:41:24Z

I think when v2 comes out, with it's plugin system, the module site will still be pretty much the same (downloading the code from a location, and setting it up in a directory). So the plugin installing would remain the same, the only change would be the code inside of those plugins

Holzhaus · 2014-09-24T12:45:29Z

Yes, but the site would contain a lot of v2 incompatible plugins.

Holzhaus · 2014-09-24T12:49:29Z

client/modinstaller.py

+            print "Installed module: %s" % self.module
+
+    def create_module_folder(self):
+        self.module_path = os.path.abspath('modules/'+self.module)


You should use jasperpath.PLUGIN_PATH. This won't work it's executed from outside the client folder, e.g:

cd / /home/pi/jasper/client/modinstaller.py --install foo

In this case, it will try to create /modules/foo (in the root fs).

Holzhaus · 2014-09-24T13:17:50Z

client/modinstaller.py

+            os.makedirs(self.module_path)
+        open(os.path.join(self.module_path, '__init__.py'), 'a').close()
+
+    def get_module_metadata(self):


This method name is very misleading. It does not return metadata but checks if a plugin of this name existis in the remote repository. Therefore it should be renamed.

Fixed. I made then two methods, one to get the metadata and another to check if the metadata exists.

charliermarsh · 2014-09-29T14:50:14Z

Will do my best to read through this today... Sorry for the delay.

charliermarsh · 2014-09-30T22:24:01Z

Don't have a ton of comments on the actual code, but I'd be fine with merging this in the near future. Seems reasonably useful. However, it's going to need some documentation. For example, it wasn't clear to me that you have to the Git route if your module has dependencies (i.e., because you need to include a folder with a requirements.txt... right?).

Additionally, why do you need to give the site full access to your GitHub "personal user data"?

charliermarsh · 2014-09-30T22:25:19Z

client/modinstaller.py

+
+if __name__ == '__main__':
+    parser = argparse.ArgumentParser(description='Jasper modules installer')
+    parser.add_argument('--install', nargs=1,


Why is nargs=1 if we give the Modules to install description? Shouldn't it be nargs='+'?

alexsiri7 · 2014-10-01T20:36:43Z

I took some ideas from the code you wrote, and examples for the comments you added to the PR.

Holzhaus · 2014-10-02T13:23:25Z

client/modinstaller.py

+        self.module = module
+
+    def __str__(self):
+        return self.message + ": " + self.module


Please use one of these syntaxes:

"%s: %s" % (self.message, self.module) # or "{message}: {module}".format(message=self.message, module=self.module)

Holzhaus · 2014-10-02T13:50:12Z

Nice.

PS: I hope I didn't appear rough or something, but in case, I just wanted to say: Sorry, I didn't mean it. Text-only communication can be a bit ambiguous sometimes.

alexsiri7 · 2014-10-02T20:28:05Z

@Holzhaus No worries. I know it's hard to communicate things using text-only, and I suppose the same counts for me (so, sorry if I offended/annoyed you in any way). Anyways, thanks for that message.

alexsiri7 · 2014-10-06T17:25:48Z

@Holzhaus do you think this is ready to be merged?

Holzhaus · 2014-10-06T17:44:52Z

client/modinstaller.py

+        finally:
+            shutil.rmtree(temp_path)
+    else:
+        raise ModuleInstallationError("Module not found", module)


I'd rather do something like:

raise ModuleInstallationError("Module '%s' not found" % module)

and then replace the custom ModuleInstallationError with this:

class ModuleInstallationError(Exception): pass

But this way, you'll have to interpolate the name of the module everywhere, and there is no requirement of a module name when instantiating the class. Wouldn't you think that, as the Error pertains a module, that the module is part of the parameters of the object being instantiated?

Well, You know the module name anyway, because you call the install() function with this argument. And IMHO, the module name should be part of it's message.
Also, the current implementation overrides the __init__() method, but does not call the superclass' __init__(), which should not happen.

Holzhaus · 2014-10-06T18:04:23Z

Sorry, some more comments, but hopefully can merge soon.

coveralls · 2014-10-06T21:20:55Z

Changes Unknown when pulling da72882 on alexsiri7:installer into * on jasperproject:master*.

korniza · 2016-01-16T21:30:48Z

I get the following error if I use AT&T STT. I have already copied the APP_KEY, Secret_key from site (Basic free access).

INFO:requests.packages.urllib3.connectionpool:Starting new HTTPS connection (1): api.att.com
DEBUG:requests.packages.urllib3.connectionpool:"POST /oauth/v4/token HTTP/1.1" 400 None
ERROR:jasper.mic:Transcription failed!
Traceback (most recent call last):
File "/root/jasper/jasper/mic.py", line 117, in check_for_keyword
transcribed = self.passive_stt_engine.transcribe(f)
File "/root/jasper/plugins/stt/att-stt/att.py", line 57, in transcribe
r = self._get_response(data)
File "/root/jasper/plugins/stt/att-stt/att.py", line 97, in _get_response
headers = {'Authorization': 'Bearer %s' % self.token,
File "/root/jasper/plugins/stt/att-stt/att.py", line 52, in token
self._token = r.json()['access_token']
KeyError: 'access_token'

Do I need to upgrade to premium account so to get STT access or there is something wrong with code/keys?

alexsiri7 mentioned this pull request Sep 14, 2014

Better system for sharing modules #163

Open

alexsiri7 force-pushed the installer branch from 635b014 to cceaf05 Compare September 14, 2014 21:06

alexsiri7 force-pushed the installer branch from cceaf05 to 8070250 Compare September 15, 2014 19:49

Holzhaus added the enhancement label Sep 17, 2014

alexsiri7 force-pushed the installer branch from 8070250 to b77648c Compare September 23, 2014 08:27

Holzhaus reviewed Sep 24, 2014
View reviewed changes

Holzhaus mentioned this pull request Sep 24, 2014

Allow brain to load submodules #195

Merged

Holzhaus reviewed Sep 24, 2014
View reviewed changes

alexsiri7 force-pushed the installer branch 3 times, most recently from 60cfedf to 9d43499 Compare September 25, 2014 05:57

alexsiri7 added 2 commits September 25, 2014 07:08

Modules installer

7934397

Use pip module to install modules

e047a15

alexsiri7 force-pushed the installer branch from 7e6e780 to e047a15 Compare September 25, 2014 06:08

Made installer's methods private

e21be5e

charliermarsh reviewed Sep 30, 2014
View reviewed changes

Installer: Fixes

26f3b76

Holzhaus reviewed Oct 2, 2014
View reviewed changes

alexsiri7 added 4 commits October 2, 2014 14:53

Optionally install requirements

8658432

Fixed syntax of Error

2226d36

Renamed _move function to _copy

68769e9

Fixed MODULES_URL

d33ed13

Holzhaus reviewed Oct 6, 2014
View reviewed changes

Merged get_module_metadata and module_exists

da72882

edouardpoitras mentioned this pull request Dec 29, 2015

How to run an operation in Jasper's main loop #417

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modules installer #174

Modules installer #174

alexsiri7 commented Sep 14, 2014

Holzhaus commented Sep 15, 2014

alexsiri7 commented Sep 15, 2014

alexsiri7 commented Sep 23, 2014

alexsiri7 commented Sep 24, 2014

Holzhaus commented Sep 24, 2014

Holzhaus Sep 24, 2014

alexsiri7 Sep 24, 2014

Holzhaus Sep 24, 2014

alexsiri7 Sep 25, 2014

alexsiri7 commented Sep 24, 2014

Holzhaus commented Sep 24, 2014

Holzhaus Sep 24, 2014

alexsiri7 Sep 25, 2014

Holzhaus Sep 24, 2014

alexsiri7 Sep 25, 2014

charliermarsh commented Sep 29, 2014

charliermarsh commented Sep 30, 2014

charliermarsh Sep 30, 2014

alexsiri7 Oct 1, 2014

alexsiri7 commented Oct 1, 2014

Holzhaus Oct 2, 2014

alexsiri7 Oct 2, 2014

Holzhaus commented Oct 2, 2014

alexsiri7 commented Oct 2, 2014

alexsiri7 commented Oct 6, 2014

Holzhaus Oct 6, 2014

alexsiri7 Oct 6, 2014

Holzhaus Oct 10, 2014

Holzhaus commented Oct 6, 2014

coveralls commented Oct 6, 2014

korniza commented Jan 16, 2016

Modules installer #174

Are you sure you want to change the base?

Modules installer #174

Conversation

alexsiri7 commented Sep 14, 2014

Holzhaus commented Sep 15, 2014

alexsiri7 commented Sep 15, 2014

alexsiri7 commented Sep 23, 2014

alexsiri7 commented Sep 24, 2014

Holzhaus commented Sep 24, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexsiri7 commented Sep 24, 2014

Holzhaus commented Sep 24, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliermarsh commented Sep 29, 2014

charliermarsh commented Sep 30, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexsiri7 commented Oct 1, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Holzhaus commented Oct 2, 2014

alexsiri7 commented Oct 2, 2014

alexsiri7 commented Oct 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Holzhaus commented Oct 6, 2014

coveralls commented Oct 6, 2014

korniza commented Jan 16, 2016