search

earth-metabolome-initiative / emi-monorepo

Monorepo for the Earth Metabolome Initiative
GNU General Public License v3.0
5 stars 0 forks source link

Sirius bindings #2

Closed mvisani closed 7 months ago

mvisani commented 7 months ago

Finished binding for Sirius. All the tests pass, the fuzzer has not detected any problems.

We tested with @oolonek an MGF file with both default parameters and specific parameters for the ENPKG pipeline, they both work.

LucaCappelletti94 commented 7 months ago

@oolonek how painful would it be to install Sirius in a Docker? We did that in the enpkg pipeline, right? If I recall correctly, the only issue was regarding the login credentials for one of the routines, am I correct?

oolonek commented 7 months ago

I don't think we used a docker but the follwoing bash scripts https://github.com/enpkg/enpkg_full?tab=readme-ov-file#-install-sirius-locally. @mvisani just told be he was keen to peek in the process of a Docker creation !! Normally login are correctly handled at https://github.com/earth-metabolome-initiative/emi-monorepo/blob/62bf8fb8b502498afd49de1b434e1dae6d8e6c79/bindings/sirius/src/sirius.rs#L48

LucaCappelletti94 commented 7 months ago

Ok so, at this time in the pipeline, we are installing Sirius in the Docker (using that script). This means that we can add to the test suite the execution of Sirius as part of the C.I.

Are the Sirius login credentials always needed or only for some tasks? I seemed to recall they were only needed for a subset of them.

oolonek commented 7 months ago

Ok we had misunderstood that you wanted to create a dedicated Sirius docker image (and not just Sirius install within a Docker as in https://github.com/earth-metabolome-initiative/emikg/blob/09ddc2ebd8dba1746ca9a8b71eed89be48b1d39a/enrichers/enricher-dirty-pipeline/Dockerfile#L28C1-L30C1) I understand that for now this latter option is OK ?

Regarding logins I think that they are required for tasks such as zodiac, canopus and similar workflows used by Sirius. And these are the typical tasks we want to run, so I guess we should login each time.

LucaCappelletti94 commented 7 months ago

If the login credentials need to be in plain text, we cannot use them in the CI. If we cannot use them in the CI but still want to test Sirius in the CI, we can test all of the things that do not require the login.

At this time we are currently always requiring a user of the bindings to login, but most likely we only want for the user to login when the builder configuration includes some routine that requires the login.

If we refactor that part to only require the login when it is strictly needed, we can (partially) test Sirius in the CI.

oolonek commented 7 months ago

If the login credentials need to be in plain text, we cannot use them in the CI. If we cannot use them in the CI but still want to test Sirius in the CI, we can test all of the things that do not require the login.

At this time we are currently always requiring a user of the bindings to login, but most likely we only want for the user to login when the builder configuration includes some routine that requires the login.

If we refactor that part to only require the login when it is strictly needed, we can (partially) test Sirius in the CI.

Couldn't we use Repository Secrets for this ? What is the issue of login each time ?

LucaCappelletti94 commented 7 months ago

I need to look into the repo secrets, it could be an option. Do you know which of the settings exactly require the login?

oolonek commented 7 months ago

I need to look into the repo secrets, it could be an option. Do you know which of the settings exactly require the login?

Will dig into that. Pasting here for the record https://github.com/boecker-lab/sirius/issues/146

LucaCappelletti94 commented 7 months ago

Great job with the documentation, now there is the matter of the README, which should contain the badges for the crate, documentation and GitHub actions, plus an explanation of what the crate is for and a couple of examples showing how to use it (and maybe also an explanation about how to install Sirius itself).

You can use as example the README of HyperLogLog.

LucaCappelletti94 commented 7 months ago

What was the issue that caused the pipeline to get sirius?

mvisani commented 7 months ago

It is in the changes of the bindings/sirius/src/sirius.rs from last commit.

We changed from this : 

command.args(&args).spawn().expect("Sirius failed to start");

To this: 

// Add arguments and spawn the command
let mut child = command.args(&args).spawn().expect("Sirius failed to start");
let status = child.wait().expect("Failed to wait on child");

On my side it still runs super slow but I would be ready to bet that it is because of my computer.

mvisani commented 7 months ago

I've added some examples and errors in the documentation. Tell me if something is missing or needs to be added ! :)