OpenMS icon indicating copy to clipboard operation
OpenMS copied to clipboard

[Fix] SiriusAdapter adaptations to Sirius 5

Open axelwalter opened this issue 3 years ago • 4 comments

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 jenkins will 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).

axelwalter avatar Sep 28 '22 11:09 axelwalter

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?

eeko-kon avatar Oct 01 '22 10:10 eeko-kon

I think I agree with @eeko-kon @axelwalter Ready to review? Then convert to normal non-draft PR.

jpfeuffer avatar Oct 01 '22 10:10 jpfeuffer

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 ?

axelwalter avatar Oct 04 '22 07:10 axelwalter

Yes, I guess we have to wait. We still need to solve SQL problems for release anyway.

jpfeuffer avatar Oct 04 '22 09:10 jpfeuffer

rebuild jenkins

axelwalter avatar Oct 25 '22 13:10 axelwalter

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.

poshul avatar Oct 28 '22 20:10 poshul

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?

jpfeuffer avatar Oct 29 '22 06:10 jpfeuffer

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.

axelwalter avatar Oct 31 '22 07:10 axelwalter

rebuild jenkins

axelwalter avatar Oct 31 '22 07:10 axelwalter

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.

axelwalter avatar Oct 31 '22 11:10 axelwalter