Plugin class #8

josenavas · 2016-09-27T19:23:04Z

Adds the plugin class so plugins do not need to duplicate a lot of code.

That class includes all the relevant code so the plugins can install themselves.

antgonza

A couple of comments/questions

antgonza · 2016-09-27T20:56:05Z

qiita_client/plugin.py

+        self.fp_types = filepath_types
+
+
+class BaseQiitaPlugin(object):


Do the methods in this object get tested?

This is a BaseClass so I'm not instantiating it directly, but all the methods are being executed through the child clases

antgonza · 2016-09-27T20:57:04Z

qiita_client/plugin.py

+
+        self._register_command(html_cmd)
+
+    def _install(self, qclient):


What about this one?

it is getting executed here: https://github.com/qiita-spots/qiita_client/pull/8/files#diff-259ba12727397257087e45ba8a60f40dR163

wasade

Few comments, looking pretty good :)

wasade · 2016-09-28T01:08:19Z

qiita_client/plugin.py

+    function : callable
+        The function that executes the command. Should be a callable that
+        conforms to the signature:
+            `function(qclient, job_id, job_parameters, output_dir)`


are any of these nullable, and is there a return signature?

No, they can't be nullable. Yup, there is a return signature, I've updated the documentation to note that.

wasade · 2016-09-28T01:09:35Z

qiita_client/plugin.py

+        # `function` will be called with the following Parameters
+        # qclient, job_id, job_parameters, output_dir
+        # Make sure that `function` can receive 4 parameters
+        if function.__code__.co_argcount != 4:


:/ this is still python 2, right? Annotations would be so awesome here

py2/py3 compatible. I want this utility library to be as general as possible so we can use it with QIIME1 and QIIME2, although I expect more devs use this to create the plugins of their own tools (which we don't control if they're py2 or py3).

wasade · 2016-09-28T01:12:02Z

qiita_client/plugin.py

+            The artifact type name
+        description : str
+            The artifact type description
+        can_be_submitted_to_ebi : bool


what's the likelihood that external submission or data exchanges will expand in the future? if it is reasonable, then it may make sense to do a more discoverable structure, something like external_exchange which could be {'ebi': True, 'vamps': False, ...}. if expansion here is unlikely, then I think the attrs make sense

Uhm, not sure. @antgonza any thoughts? If this can expand we may want to rethink also how to change the current DB structure to support this (although we can refer that for later)

It's likely to change but not sure when. I agree with the approach of dealing with it once we know it's actually going to happen. Note that this will also require changes in the main qiita code base ...

wasade · 2016-09-28T01:13:27Z

qiita_client/plugin.py

+        self.name = name
+        self.version = version
+        self.description = description
+        self.publications = dumps(publications) if publications else ""


is there a plugin type which we anticipate wont have a publication or doi? if they don't, we could always require tying a version to zenodo. perhaps unnecessary

I would be surprised if something doesn't have a DOI - I think in previous discussions we agreed to use DOI since it is more common to have a DOI.

There is going to be cases we are going to be using tools without publication, the first one that comes to mind is: KneedData.

wasade · 2016-09-28T01:14:07Z

qiita_client/plugin.py

+        conf_dir = environ.get(
+            'QIITA_PLUGINS_DIR', join(expanduser('~'), '.qiita_plugins'))
+        self.conf_fp = join(conf_dir, "%s_%s.conf" % (self.name, self.version))
+


would it make sense for an assert over os.path.exists?

The conf_fp doesn't necessarily exists at this point (see the function generate_config). The change that I made is that now both Qiita and the plugin can actually use the same config file. There is some information that needs to be known by both of them in order to communicate so at least they need to share a file (which can then be copied over multiple locations and allow plugin execution from anywhere).

wasade · 2016-09-28T01:16:26Z

qiita_client/plugin.py

+        """
+        self.task_dict[command.name] = command
+
+    def _install(self, qclient):


this is semantic, but i think of install as actually putting a program, library, or documentation somewhere in a logical place. in this case, this I think more mirrors a registration, where a call to ._register would establish this plugins existence with qiita?

You're right - changed to _register.

wasade · 2016-09-28T01:17:31Z

qiita_client/plugin.py

+    def _install(self, qclient):
+        """Installs the plugin in Qiita"""
+        # Get the command information from qiita
+        info = qclient.get('/qiita_db/plugins/%s/%s/'


I'm a bit confused here, qiita already knows about the plugin prior to the install?

Should the .status_code be checked to test for 200?

Yes, so when Qiita starts the server, it will load the plugin configuration files. The reason behind this is that in order to communicate the plugin (client) and Qiita (server) they need to both know the credentials. So the config file is generating those credentials, so in order to actually add a plugin to the system you need access to the FS to be able to add this configuration file. However, the configuration file only allows to "register" the plugin itself, but not the commands. The commands get added dynamically using the REST api, which is what is done here.

Is a bit complex, but I think this is the best balance between security and flexibility. This flexibility in the commands is needed mainly for QIIME 2, which is a system that works with plugins itself, so it is possible that there are commands that are no longer available, while new ones may appear.

Regarding the 200, the QiitaClient object (qclient) already performs those checks internally and it will raise a useful error in case that the return code is different from 200. The object returned from the get and post is actually a dictionary, which contains the decoded JSON from the request body.

wasade · 2016-09-28T01:19:19Z

qiita_client/plugin.py

+            if cmd.name in info['commands']:
+                qclient.post('/qiita_db/plugins/%s/%s/commands/%s/activate/'
+                             % (self.name, self.version,
+                                cmd.name.replace(' ', "%20")))


what about

> import urllib > urllib.parse.quote("foo bar") 'foo%20bar'

should this post, and the one on line 181 have their status codes checked?

Thanks for the tip! See my answer above regarding the status codes.

wasade · 2016-09-28T01:22:05Z

qiita_client/plugin.py

+    -----
+    Both `validate_func` and `html_generator_func` should be a callable
+    that conforms to the signature:
+    function(qclient, job_id, job_parameters, output_dir)


is there a return?

Yes, it is actually the same signature than above - changed.

wasade · 2016-09-28T01:23:19Z

qiita_client/plugin.py

+        command: QiitaCommand
+            The command to be added to the plugin
+        """
+        self._register_command(command)


it seems like this should just be public?

I don't want the _register_command function to be public on the QiitaTypePlugin class (although I'm using it internally). The rationale is that the commands in the QiitaTypePlugin are controlled (there are only 2 - 'Validate' and 'Generate HTML summary'), while QiitaPlugin can have any number of commands. However, both are actually represented in the same way, and that's why most of the functionality is added in the base class.

josenavas added 7 commits September 26, 2016 13:00

Adding plugin class

c526dfc

Adding command class

151b20b

Type plugin successfully installed

f431fc3

Finishing up code

905eada

Updating travis to install the new Qiita branch

01178f7

Adding future to setup.py

0ba3041

Fixing py3 compat

1723b2c

antgonza reviewed Sep 27, 2016

View reviewed changes

Fixing failing test

ca7d9aa

wasade reviewed Sep 28, 2016

View reviewed changes

Addressing @wasade's comments

2f45c38

antgonza merged commit d09cf35 into qiita-spots:master Sep 28, 2016

		self.fp_types = filepath_types


		class BaseQiitaPlugin(object):


		self._register_command(html_cmd)

		def _install(self, qclient):

Plugin class #8

Plugin class #8

Uh oh!

Conversation

josenavas commented Sep 27, 2016

Uh oh!

antgonza left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wasade left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josenavas Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josenavas Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josenavas Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josenavas Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

josenavas Sep 28, 2016 •

edited

Loading

josenavas Sep 28, 2016 •

edited

Loading

josenavas Sep 28, 2016 •

edited

Loading

josenavas Sep 28, 2016 •

edited

Loading