v5.8.6+galaxy0 by RJMW · Pull Request #26 · computational-metabolomics/sirius-csifingerid-galaxy

RJMW · 2024-07-30T14:09:37Z

No description provided.

bernt-matthias · 2024-07-30T14:57:54Z

This registration thing in Sirius is really super annoying. Also sirius being kind of a black box always did not feel right for me.

RJMW · 2024-07-30T15:14:53Z

@bernt-matthias - Thank you for informing me about the Sirius OpenMS implementation. Although the registration process isn't ideal, the tool appears to be popular within the Metabolomics community. I have received several requests, including W4M.

While I managed to get the testing to work locally using Planemo, integrating it with the Galaxy GitHub workflow has proven to be somewhat tricky. I am still keen to see if I can get the CI tests to work.

I am using a similar approach to the one used for genome annotation tools, specifically Apollo. See here

Your review or input is more than welcome.

bgruening · 2024-07-30T16:06:41Z

tools/sirius_csifingerid/sirius_csifingerid.py

+cmd_login = "sirius login " \
+            "--user='{}' " \
+            "--password='{}'".format(credentials["username"],
+                                     credentials["password"])


Suggested change

cmd_login = "sirius login " \

"--user='{}' " \

"--password='{}'".format(credentials["username"],

credentials["password"])

cmd_login = f"sirius login --user='{credentials["username"]}' --password='{credentials["password"]}'"

f-string are the new cool stuff :)

bgruening · 2024-07-30T16:07:03Z

tools/sirius_csifingerid/sirius_csifingerid.py

-                       paramd["cli"]["--profile"],
-                       paramd["cli"]["--database"]
-          )
+          "fingerprint " \


maybe also use f-string here?

bgruening · 2024-07-30T16:08:38Z

.github/styler.R

Those are IUC updates, correct? Maybe put them into a separate PR so we know what belongs to this PR?

(I assume you need to change CI for testing as well)

bgruening · 2024-07-30T16:09:52Z

tools/sirius_csifingerid/sirius_csifingerid.xml

+            echo "Credentials for Sirius are available via: User -> Preferences -> Manage Information"
+        #else:
+            echo "Running Sirius as user: $username" &&
+            export _JAVA_OPTIONS=-Duser.home=$__new_file_path__ &&


You should not write into __new_file_path__ random stuff. Can you use there \$PWD?

bgruening · 2024-07-30T16:10:16Z

tools/sirius_csifingerid/sirius_csifingerid.xml

+                --candidates $candidates
+                --ppm_max $ppm_max
+                --polarity $polarity
+                --out_dir $__new_file_path__


Suggested change

--out_dir $__new_file_path__

--out_dir ./

Better?

bgruening · 2024-07-30T16:10:32Z

tools/sirius_csifingerid/sirius_csifingerid.xml

+                --meta_select_col $meta_select_col
+                --min_MSMS_peaks $min_MSMS_peaks
+                --schema $schema
+                --temp_dir $__new_file_path__


Suggested change

--temp_dir $__new_file_path__

--temp_dir ./

bernt-matthias · 2024-07-31T07:54:26Z

.github/workflows/pr.yaml

+  # group runs by PR, but keep runs on main separate
  # because we do not want to cancel toolshed uploads
-  group: pr-${{ (github.ref == 'refs/heads/master' || github.ref == 'refs/heads/main') && github.run_number || github.ref }}
+  group: pr-${{ (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/main') && github.run_number || github.ref }}


You can leave this as it is. It covers main and master

RJMW · 2024-07-31T08:20:26Z

@bernt-matthias @bgruening - Thank you for your swift reviews.

Could you help me understand why the arguments for Planemo are resulting in the incorrect order, see here

I am using the additional-planemo-options argument for planemo to include a specific job_config that includes env variables, see here. This approach seems to work for this particular tool. Although the ci logs are not available anymore.

bernt-matthias · 2024-07-31T07:55:42Z

.github/workflows/pr.yaml

        galaxy-branch: ${{ env.GALAXY_BRANCH }}
        chunk: ${{ matrix.chunk }}
        chunk-count: ${{ needs.setup.outputs.chunk-count }}
+        additional-planemo-options: --job_config_file "${{ github.workspace }}/config/job_conf_planemo_subst.xml"


Test currently fails with

Error: Invalid value for '--job_config_file': File '"/home/runner/work/sirius-csifingerid-galaxy/sirius-csifingerid-galaxy/config/job_conf_planemo_subst.xml"' does not exist.

In case you want to debug danielr1996/envsubst-action@1.0.0 you could just execute the command used in the action: https://github.com/danielr1996/envsubst-action/blob/b10d6e6eb5dba1c22527571460ceb83bc17c0b28/entrypoint.sh#L3

bernt-matthias · 2024-07-31T07:59:35Z

config/job_conf_planemo.xml

+            <param id="docker_sudo">false</param>
+            <param id="docker_sudo_cmd">sudo</param>
+            <param id="docker_cmd">docker</param>
+            <param id="docker_run_extra_arguments">-e USERNAME_SIRIUS='${USERNAME_SIRIUS}' -e PASSWORD_SIRIUS='${PASSWORD_SIRIUS}'</param>


You can set env vars for containers like so:

<param id="docker_env_USERNAME_SIRIUS">'${USERNAME_SIRIUS}'</param> <param id="docker_env_PASSWORD_SIRIUS">'${USERNAME_SIRIUS}'</param>

galaxyproject/galaxy#16666

bernt-matthias · 2024-07-31T08:41:37Z

tools/sirius_csifingerid/sirius_csifingerid.py

 parser.add_argument('--schema', default='msp')
 parser.add_argument('-a', '--adducts', action='append', nargs=1,
                    required=False, default=[], help='Adducts used')
+parser.add_argument('--credentials')


help would be nice.

bernt-matthias · 2024-07-31T08:42:27Z

tools/sirius_csifingerid/sirius_csifingerid.py

+            "--user='{}' " \
+            "--password='{}'".format(credentials["username"],
+                                     credentials["password"])
+os.system(cmd_login)


Using subprocess might be better: https://docs.python.org/3/library/subprocess.html

bernt-matthias · 2024-07-31T08:44:26Z

tools/sirius_csifingerid/sirius_csifingerid.xml

+            #set $username = $__user__.extra_preferences.get('sirius_account|username', "")
+        #end if
+        #if $username == "":
+            echo "Credentials for Sirius are available via: User -> Preferences -> Manage Information"


Suggested change

echo "Credentials for Sirius are available via: User -> Preferences -> Manage Information"

>&2 echo "Credentials for Sirius are available via: User -> Preferences -> Manage Information";

exit 1

bernt-matthias · 2024-07-31T08:46:41Z

tools/sirius_csifingerid/sirius_csifingerid.xml

+        #if $username == "":
+            echo "Credentials for Sirius are available via: User -> Preferences -> Manage Information"
+        #else:
+            echo "Running Sirius as user: $username" &&


Should we expose this info to the user (e.g. in the case it's set by the admin via env vars)?

bernt-matthias · 2024-07-31T08:52:35Z

Could you help me understand why the arguments for Planemo are resulting in the incorrect order, see here

What do you mean with wrong order?

My guess would be that the use of single and double quotes in --job_config_file '"/home/runner/work/sirius-csifingerid-galaxy/sirius-csifingerid-galaxy/config/job_conf_planemo_subst.xml"' is the problem.

RJMW and others added 18 commits July 16, 2024 15:33

Bump version Sirius to v5.8.6 and add credentials for login

ef1ca7d

Add example config file for user preferences

ab15e3d

Update tests

a1232b7

Remove Travis testing

e6ba3db

Fix pylint

189364e

Update github workflow

694d36f

Update github workflow (2)

43e4fad

Update github workflow (2, lintr)

66132b9

Add styler.R (to complete set of scripts for testing)

dcfb320

Chmod 755 styler.R

3b15038

Add write-summaries to sirius call (cmd)

acfa1e0

Add .tt_biocontainer_skip

da0d081

Update pr.yaml

d8611a5

Update job_config and ENVs

98a88fe

Update .tt_biocontainer_skip

e50ba06

Update github workflow

d5761fe

Rename job config

54d97b9

Update additional-planemo-options

abcb97f

RJMW mentioned this pull request Jul 30, 2024

Galaxy modules not functionnal after Sirius 5.0 release? #24

Closed

RJMW requested review from bernt-matthias and bgruening July 30, 2024 15:50

bgruening reviewed Jul 30, 2024

View reviewed changes

bernt-matthias reviewed Jul 31, 2024

View reviewed changes

Update pr.yaml (additional-planemo-options)

778685e

	echo "Credentials for Sirius are available via: User -> Preferences -> Manage Information"
	>&2 echo "Credentials for Sirius are available via: User -> Preferences -> Manage Information";
	exit 1

Conversation

RJMW commented Jul 30, 2024

Uh oh!

bernt-matthias commented Jul 30, 2024

Uh oh!

RJMW commented Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

RJMW commented Jul 31, 2024

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!

bernt-matthias commented Jul 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RJMW commented Jul 30, 2024 •

edited

Loading