Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added new module lofraq/viterbi; solves new module: lofreq/viterbi #5158 #5197

Merged
merged 9 commits into from
Mar 19, 2024

Conversation

MarieLataretu
Copy link
Contributor

@MarieLataretu MarieLataretu commented Mar 18, 2024

PR checklist

Closes #5158

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the module conventions in the contribution docs
  • If necessary, include test data in your PR.
  • Remove all TODO statements.
  • Emit the versions.yml file.
  • Follow the naming conventions.
  • Follow the parameters requirements.
  • Follow the input/output options guidelines.
  • Add a resource label
  • Use BioConda and BioContainers if possible to fulfil software requirements.
  • Ensure that the test works with either Docker / Singularity. Conda CI tests can be quite flaky:
    • For modules:
      • nf-core modules test <MODULE> --profile docker
      • nf-core modules test <MODULE> --profile singularity
      • nf-core modules test <MODULE> --profile conda
    • For subworkflows:
      • nf-core subworkflows test <SUBWORKFLOW> --profile docker
      • nf-core subworkflows test <SUBWORKFLOW> --profile singularity
      • nf-core subworkflows test <SUBWORKFLOW> --profile conda

@MarieLataretu MarieLataretu requested a review from a team as a code owner March 18, 2024 14:47
@MarieLataretu MarieLataretu requested review from SPPearce and removed request for a team March 18, 2024 14:47
@MarieLataretu
Copy link
Contributor Author

Why is it running so many lofreq-unrelated tests? 😱

@FloWuenne
Copy link
Contributor

Why is it running so many lofreq-unrelated tests? 😱

The reason why it runs unrelated tests is that it compares your PR changes to master and so any others changes done while you made your feature branch will be run as tests as well. Usually, clicking Update branch at the bottom of the PR should bring it to only running your tests!

@FloWuenne FloWuenne self-requested a review March 18, 2024 15:42
@MarieLataretu
Copy link
Contributor Author

okay, the problem with the failing conda test:

The samtools version differs in the conda (1.19.2) environment and the container (1.17). This leads to a different header line in the BAM file, even if the build version is the same.

@FloWuenne
Copy link
Contributor

@MarieLataretu Great catch! I was also looking at the difference in conda vs container environment. This is really strange, as the biocontainers build from bioconda should be exactly the same no? Would this be fixed by using a different build?
Otherwise I see two options:

  1. Disable conda
  2. Generate a custom container image with the samtools version corresponding to the conda environment

@MarieLataretu
Copy link
Contributor Author

I checked to which build the conda env was resolved, and adapted the singularity container accordingly (so far only locally - I don't know how conda decides on the build 🤔 )

My best guess is that in conda, samtools always resolves to the latest version when you build the environment. The same is true for the container, only that it's frozen to the latest samtools version when the container is built.

I checked out a third option, but I don't know if follows the nf-core standards: I added the samtools version from the container as a dependency to the yml:

dependencies:
  - bioconda::lofreq=2.1.5
  - bioconda::samtools=1.17

Locally the tests pass!

@FloWuenne
Copy link
Contributor

I checked to which build the conda env was resolved, and adapted the singularity container accordingly (so far only locally - I don't know how conda decides on the build 🤔 )

My best guess is that in conda, samtools always resolves to the latest version when you build the environment. The same is true for the container, only that it's frozen to the latest samtools version when the container is built.

I checked out a third option, but I don't know if follows the nf-core standards: I added the samtools version from the container as a dependency to the yml:

dependencies:
  - bioconda::lofreq=2.1.5
  - bioconda::samtools=1.17

Locally the tests pass!

I just tested your patch with specifying the samtools version in the environment.yml file and nf-test still produces different md5sums with conda than the ones produced by docker and singularity, which suggests to me also potential other differences than just the samtool version?

Copy link
Contributor

@FloWuenne FloWuenne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spend some time in Gitpod trying to modify the conda dependencies so that they would output the same md5sum tests as docker and singularity, but was also unable to do so. For the PR to pass, this is the main thing to resolve...

modules/nf-core/lofreq/viterbi/environment.yml Outdated Show resolved Hide resolved
@MarieLataretu
Copy link
Contributor Author

(I'm struggling with git 😅 )

Copy link
Contributor

@FloWuenne FloWuenne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ⭐️

@MarieLataretu MarieLataretu added this pull request to the merge queue Mar 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 19, 2024
@MarieLataretu MarieLataretu added this pull request to the merge queue Mar 19, 2024
@MarieLataretu MarieLataretu removed this pull request from the merge queue due to a manual request Mar 19, 2024
@MarieLataretu MarieLataretu added this pull request to the merge queue Mar 19, 2024
Merged via the queue into nf-core:master with commit 26dc61f Mar 19, 2024
11 checks passed
@MarieLataretu MarieLataretu deleted the lofreq_viterbi branch March 19, 2024 16:03
jvfe added a commit to jvfe/modules that referenced this pull request Mar 19, 2024
* master:
  Fix test for FASTQ_FASTQC_UMITOOLS_FASTP which included an absolute path in the snapshot. (nf-core#5278)
  added new module lofraq/viterbi; solves new module: lofreq/viterbi nf-core#5158 (nf-core#5197)
  Update glimpse/phase output channel to phased_variants nf-core#5172 (nf-core#5174)
  Add module: GTFSORT (nf-core#5237)
  add nf-test to bedtools/sort - nf-core#3936 (nf-core#5221)
  addedd optional output channel for lib files (nf-core#5257)
vlebars pushed a commit to vlebars/modules that referenced this pull request Mar 20, 2024
…-core#5158 (nf-core#5197)

* added new module lofraq/viterbi; solves new module: lofreq/viterbi nf-core#5158

* added new module lofraq/viterbi; solves new module: lofreq/viterbi nf-core#5158

* removed yml schema comments

* added samtools as conda dependency; chnaged container build version to py310h47ef89e_10; updated test snapshot
tucano pushed a commit to tucano/modules that referenced this pull request Mar 20, 2024
…-core#5158 (nf-core#5197)

* added new module lofraq/viterbi; solves new module: lofreq/viterbi nf-core#5158

* added new module lofraq/viterbi; solves new module: lofreq/viterbi nf-core#5158

* removed yml schema comments

* added samtools as conda dependency; chnaged container build version to py310h47ef89e_10; updated test snapshot
jennylsmith pushed a commit to RSC-RP/modules that referenced this pull request Mar 20, 2024
…-core#5158 (nf-core#5197)

* added new module lofraq/viterbi; solves new module: lofreq/viterbi nf-core#5158

* added new module lofraq/viterbi; solves new module: lofreq/viterbi nf-core#5158

* removed yml schema comments

* added samtools as conda dependency; chnaged container build version to py310h47ef89e_10; updated test snapshot
alexnater pushed a commit to alexnater/nf-core-modules that referenced this pull request Mar 21, 2024
…-core#5158 (nf-core#5197)

* added new module lofraq/viterbi; solves new module: lofreq/viterbi nf-core#5158

* added new module lofraq/viterbi; solves new module: lofreq/viterbi nf-core#5158

* removed yml schema comments

* added samtools as conda dependency; chnaged container build version to py310h47ef89e_10; updated test snapshot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

new module: lofreq/viterbi
2 participants