[Fix] SiriusAdapter adaptations to Sirius 5
Description
With Sirius 5 some adaptations to the SiriusAdapter where necessary.
Calling CSI:FingerID is done differently now, instead of the fingerid command the commands fingerprint and structures calculate molecular fingerprints and structures now. The number of candidates for CSI:FingerID can not be set individually any more, instead the number of candidates is taken from the sirius formula prediction parameters.
The output gets zipped by default, which lead to the failing of AssayGeneratorMetabo. In Sirius 5.6.0 a new --no-compression flag was added to not compress the output. This version is available as a snapshot right now and will be released soon.
Also changed the handling of Sirius parameters. The Sirius 5.6.0 original parameters were added as comments. By default, SiriusAdapter will use all the original parameters now and add parameters the the CLI call only when they have been changed by the user to declutter the CLI command.
All changes pass locally with the new sirius 5.6 snapshot, once the official version is released I will update the THIRDPARTY repository again.
Checklist
- [ ] Make sure that you are listed in the AUTHORS file
- [ ] Add relevant changes and new features to the CHANGELOG file
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] New and existing unit tests pass locally with my changes
- [ ] Updated or added python bindings for changed or new classes (Tick if no updates were necessary.)
How can I get additional information on failed tests during CI
Click to expand
If your PR is failing you can check out- The details of the action statuses at the end of the PR or the "Checks" tab.
- http://cdash.openms.de/index.php?project=OpenMS and look for your PR. Use the "Show filters" capability on the top right to search for your PR number. If you click in the column that lists the failed tests you will get detailed error messages.
Advanced commands (admins / reviewer only)
Click to expand
/reformat(experimental) applies the clang-format style changes as additional commit. Note: your branch must have a different name (e.g., yourrepo:feature/XYZ) than the receiving branch (e.g., OpenMS:develop). Otherwise, reformat fails to push.- setting the label "NoJenkins" will skip tests for this PR on jenkins (saves resources e.g., on edits that do not affect tests)
- commenting with
rebuild jenkinswill retrigger Jenkins-based CI builds
:warning: Note: Once you opened a PR try to minimize the number of pushes to it as every push will trigger CI (automated builds and test) and is rather heavy on our infrastructure (e.g., if several pushes per day are performed).
One small comment: I would set the default compound+tree timeout to 100 (s) instead of infinite (-1), otherwise Sirius runs for a day for very simple files (If you have many compounds with very high mass). This is also recommended by the Sirius team. Otherwise, you can mention it at the documentation perhaps as a suggestion?
I think I agree with @eeko-kon @axelwalter Ready to review? Then convert to normal non-draft PR.
Good point with the time outs! Since the official Sirius 5.6. will be released soon I would wait for that to update our thirdparty repository. The changes in this branch are compatible only with the 5.6 version. What do you think @jpfeuffer ?
Yes, I guess we have to wait. We still need to solve SQL problems for release anyway.
rebuild jenkins
So, at this point I'm doubtful that these tests are adding probative value, given how stochastic the outputs are. I think it's time to re-evaluate whether we should keep the failing ones, rather than playing cat-and-mouse with whitelisting.
I would like to see a diff of the files of CI and a local machine. Is it really indeterministic on the same platform?? I would vote for checking the versions of Sirius directly on the machine and the input that we write for Sirius then. And if it is Sirius to report it as a bug and disable the test for now. Did anyone ever check if the results still make sense? E.g. if the compounds at least match the mass?
True, at this point it doesn't make sense any more. So I would take the CI test results instead of the local ones and check if the results are reproducible. If not, discard the tests.
rebuild jenkins
Using the test files generated by the CI macOS works well and I guess it's okay to test the fluctuating tests only on one system. With the passing tests it would be ready for merging from my side.