Client: Separate plugin file system #57

sjanssen2 · 2025-08-27T21:27:06Z

This is the companion PR to qiita-spots/qiita#3480 where the heavy lifting is implemented to transfer files between qiita main and plugins via a) "filesystem", i.e. direct access as is or b) via "https" get and post tornado methods. In the feature we can extend by S3 or other protocols.

I want to re-use the existing _request_retry mechanism, but didn't know how to return the binary content in https://github.com/jlab/qiita_client/blob/c2d597647b3a22792a7dcd88408895e81d33ceb6/qiita_client/qiita_client.py#L394 without adding the new "rettype" parameter. Is there an easier way?

…incoupling"

antgonza

A few suggestions.

antgonza · 2025-08-28T14:14:56Z

qiita_client/plugin.py

@@ -136,13 +136,46 @@ def __init__(self, name, description, can_be_submitted_to_ebi,


 class BaseQiitaPlugin(object):
-    def __init__(self, name, version, description, publications=None):
+    # default must be first element
+    _ALLOWED_PLUGIN_COUPLINGS = ['filesystem', 'https']


What about adding another global variable: _DEFAULT_PLUGIN_COUPLINGS = 'filesystem'; then change this to _ALLOWED_PLUGIN_COUPLINGS = [_DEFAULT_PLUGIN_COUPLINGS, 'https'].

stupid me. Should have done this already :-) Thanks!

antgonza · 2025-08-28T14:15:28Z

qiita_client/plugin.py

@@ -151,7 +184,8 @@ def __init__(self, name, version, description, publications=None):
            'QIITA_PLUGINS_DIR', join(expanduser('~'), '.qiita_plugins'))
        self.conf_fp = join(conf_dir, "%s_%s.conf" % (self.name, self.version))

-    def generate_config(self, env_script, start_script, server_cert=None):
+    def generate_config(self, env_script, start_script, server_cert=None,
+                        plugin_coupling=_ALLOWED_PLUGIN_COUPLINGS[0]):


then here you can replace _ALLOWED_PLUGIN_COUPLINGS[0] for _DEFAULT_PLUGIN_COUPLINGS

antgonza · 2025-08-28T14:16:25Z

qiita_client/plugin.py

+        # the plugin coupling protocoll can be set in three ways
+        # 1. default is always "filesystem", i.e. first value in
+        #    _ALLOWED_PLUGIN_COUPLINGS. This is to be downward compatible.
+        # 2. the plugin configuration can hold a section 'network' with an
+        #    option 'PLUGINCOUPLING'. For old config files, this might not
+        #    (yet) be the case. Therefore, we are double checking existance
+        #    of this section and parameter here.
+        # 3. you can set the environment variable QIITA_PLUGINCOUPLING
+        # Precedence is 3, 2, 1, i.e. the environment variable overrides the
+        # other two ways.


Should this be part of the readme in the repo? I think moving these lines there will give them more visibility for future developers; what do you think?

I happily promoted this section into a new paragraph in the readme.md

antgonza · 2025-08-28T14:17:07Z

qiita_client/plugin.py

@@ -165,6 +199,9 @@ def generate_config(self, env_script, start_script, server_cert=None):
            If the Qiita server used does not have a valid certificate, the
            path to the Qiita certificate so the plugin can connect over
            HTTPS to it
+        plugin_coupling : str
+            Type of coupling of plugin to central for file exchange.
+            Valid values are 'filesystem' and 'https'.


--> Valid values are _see _ALLOWED_PLUGIN_COUPLINGS

antgonza · 2025-08-28T14:17:23Z

qiita_client/plugin.py

+        # 3. you can set the environment variable QIITA_PLUGINCOUPLING
+        # Precedence is 3, 2, 1, i.e. the environment variable overrides the
+        # other two ways.
+        plugincoupling = self._ALLOWED_PLUGIN_COUPLINGS[0]


_DEFAULT_PLUGIN_COUPLINGS

much cleaner now.

antgonza · 2025-08-28T14:22:09Z

qiita_client/qiita_client.py

@@ -714,3 +743,116 @@ def _process_files_per_sample_fastq(self, files, prep_info,
        prep_info = prep_info.filter(items=sample_names.keys(), axis=0)

        return sample_names, prep_info
+
+    def fetch_file_from_central(self, filepath, prefix=''):


Wondering if the default should be for prefix to be None? Maybe this is clear that it will be ignored ...

agreed. I thought testing for this could be done via If not prefix:, however, this does not seem to work with None?! Therefore, if conditions are slightly longer now

antgonza

A couple of questions.

antgonza · 2025-08-29T13:14:59Z

.github/workflows/qiita-ci.yml

@@ -104,7 +104,8 @@ jobs:
          mkdir -p ${CONDA_PREFIX}/var/run/nginx/
          export NGINX_FILE=`pwd`/qiita-dev/qiita_pet/nginx_example.conf
          export NGINX_FILE_NEW=`pwd`/qiita-dev/qiita_pet/nginx_example_local.conf
-          sed "s#/home/runner/work/qiita/qiita#${PWD}/qiita-dev/#g" ${NGINX_FILE} > ${NGINX_FILE_NEW}
+          sed "s#/home/runner/work/qiita/qiita#${PWD}/qiita-dev#g" ${NGINX_FILE} > ${NGINX_FILE_NEW}
+          sed -i "s#/Users/username/qiita#${PWD}/qiita-dev#g" ${NGINX_FILE_NEW}


Is this like required?

line 107 is to removed // from some of the urls
line 108 is necessary for the nginx test instance to point to the actual BASE_DATA_DIR within the github action: https://github.com/qiita-spots/qiita/blob/a34dcebc44ea6408340d31ecaf0efd1f78e3cc6b/qiita_pet/nginx_example.conf#L57-L63

antgonza · 2025-08-29T13:22:17Z

README.md

+2. the plugin configuration can hold a section `network` with an
+   option `PLUGINCOUPLING`. For old config files, this might not
+   (yet) be the case. Therefore, we are double checking existance
+   of this section and parameter here.


This confuses me, why have the config-file and the environmental variable? I think just leaving the env var should suffice, but I might be missing something ....

it CAN all be done just by the environment variable. However, qiita collects the plugin configuration files in the PLUGIN_DIR directory. I thought it would be handy to store the protocol in these files, such that a DB reset won't hurt - which is common during testing

I see what you are saying but then this will set that variable for that plugin, meaning that we might need multiple plugins for the same command based on the configuration; right? For example, deblur would need a plugin that uses filesystem and another for https; which I believe we decided it was not desirable but I also know that we are still figuring out this. What do you think?

I think it is fine as is for your Slurm setup, but agree the config file and env seem a bit redundant.

In a container scenario, the config file would also live within the container and let the plugin know how to communicate with main - no need to obtain this value from the DB. Thus you can have two different container, holding the same plugin, but communicating differently (maybe interesting for long / short distances around the globe?) The DB would then just need to decide which URL to use for triggering, but doesn't need to worry about the protocol

Got it - sorry that this is taking longer than expected, but just to confirm, then wouldn't the container always have the env variable set to https; thus, making the config file redundant?

env variable would NOT be set by default, so container reads from config file, except you forcefully want to make the container change its behavior: in this case you can override via env variable

OK, thank you.

sjanssen2 added 26 commits August 15, 2025 13:44

prepare another setting variable to store plugin coupling thightness

461f69f

provide two functions to retrieve and push file content to qiita master

fefc02b

add a configurable parameter to define plugincoupling for plugin

a04beeb

codestyle

8f9fbec

adapting test to new configuration option

8d0844d

start working on tests

47a4551

check spelling

8d22021

fixing existing test

e032053

wrong list index

7d5b098

initial tests + docstrings

349ab44

use a prefix user has access to

1fdf1e5

remove /

748f727

capitalize comments

8f26804

use qiita branch

509b3d5

specify directory name

0cc6a50

add testing https protocol

738ab79

fix protected

0849ca7

operate on file copy

e1cb5b6

debug

ba929a2

avoid //

1d26830

remove trailing /

85c40f2

remove debug, activate endpoint, reactivate all tests

7c46442

set path to / of only filename is given

d9543c9

fix typo

b9ade44

enable environment variable, configuration file and default for "plug…

e336fbc

…incoupling"

revert to qiita dev and remove ENABLE_HTTPS_PLUGIN_FILETRANSFER

c2d5976

antgonza requested changes Aug 28, 2025

View reviewed changes

sjanssen2 added 3 commits August 29, 2025 08:23

address Antonio's great suggestions

65e6f48

promoting comment to part of readme

7b6ca63

fix formatting

296e32c

"not prefix" did not match properly

384e97d

sjanssen2 requested a review from antgonza August 29, 2025 06:59

antgonza reviewed Aug 29, 2025

View reviewed changes

antgonza merged commit ddebae1 into qiita-spots:master Aug 29, 2025
2 checks passed

sjanssen2 deleted the uncouplePlugins branch August 31, 2025 16:30

Client: Separate plugin file system #57

Client: Separate plugin file system #57

Uh oh!

Conversation

sjanssen2 commented Aug 27, 2025

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!

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!

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!

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!

Uh oh!

Uh oh!