search

nf-core / modules

Repository to host tool-specific module files for the Nextflow DSL2 community!
https://nf-co.re/modules
MIT License
280 stars 716 forks source link

[UPDATE] Fix failing tests after new NF DSL2 syntax update #1094

Closed drpatelh closed 1 year ago

drpatelh commented 2 years ago

Please submit PRs fixing these modules to my fork: https://github.com/drpatelh/nf-core-modules/tree/update

You should be able to run pytest as normal but the options are now in a nextflow.config like here instead of using the addParams

Failing at module level

Still failing

Fixed at module level

Already passing on local tests

Failing at sub-workflow level

JoseEspinosa commented 2 years ago

kallistobustools/ref was giving this error (actually also kallistobustools/count):

Launching `./tests/modules/kallistobustools/ref/main.nf` [sad_marconi] - revision: 59ac0a19e6
[-        ] process > test_kallistobustools_ref:KALLISTOBUSTOOLS_REF -
No such variable: containerEngine

I fixed this by changing the container directive see here Actually, I populate the module template in the same way see here and I was wondering if we should update all the modules again with this... 😢 ping @drpatelh

drpatelh commented 2 years ago

Hmmm....why is it not working for that module in particular when it is for all of the others? Think we need to figure that out. Could you post what the syntax would look like before and after so we can see how it is different?

JoseEspinosa commented 2 years ago

The difference can be seen in the commit I provided in the comment above: The original code was:

container "${ workflow.containerEngine == 'singularity' && !task.ext.singularity_pull_docker_container ?
     'https://depot.galaxyproject.org/singularity/kb-python:0.26.3--pyhdfd78af_0' :
     'quay.io/biocontainers/kb-python:0.26.3--pyhdfd78af_0' }"

The code that solves the problem is:

container workflow.containerEngine == 'singularity' && !task.ext.singularity_pull_docker_container ?
     'https://depot.galaxyproject.org/singularity/kb-python:0.26.3--pyhdfd78af_0' :
     'quay.io/biocontainers/kb-python:0.26.3--pyhdfd78af_0'

The only difference I can spot with other modules is the dash in kb-python but shouldn't make any difference since is within single quotes... 🤔

drpatelh commented 2 years ago

This is really weird. @mahesh-panchal any idea why having the closure would cause this problem?

JoseEspinosa commented 2 years ago

The reason is that there is an input value channel defined as workflow in the module main.nf: https://github.com/drpatelh/nf-core-modules/blob/3819cbb20fa9094ef3926b66a9dda623cca2ae32/modules/kallistobustools/ref/main.nf#L13 However, I don't understand why changing the container directive was solving the problem... 🤔

drpatelh commented 2 years ago

Ahhh. So it's a scope issue The closure evaluates any variables in the local scope as priority? So we can either change the input value to something else or remove the closure if it isn't required?

May be able to test by artificially creating an input variable called task to see if you get a similar error.

JoseEspinosa commented 2 years ago

I updated the input value to workflow_mode. Regarding defining an input value called task I made the test and seems to work but gives a warning:

[a9/b8a539] process > test_kallistobustools_count:KALLISTOBUSTOOLS_COUNT (test) [100%] 1 of 1 ✔
WARN: Process test_kallistobustools_count:KALLISTOBUSTOOLS_COUNT overrides reserved variable `task`
drpatelh commented 2 years ago

I would still be in favour of removing the closure if we don't need it for simplicity. Let's see what @mahesh-panchal thinks.

lassefolkersen commented 2 years ago

I checked theimputeme/vcftoprs module on an AWS instance, using latest nextflow (21.10.3.5655)

PROFILE=docker nextflow run tests/modules/imputeme/vcftoprs -entry test_imputeme_vcftoprs -c tests/config/nextflow.config

and this

NF_CORE_MODULES_TEST=1 PROFILE=docker pytest --tag imputeme/vcftoprs --symlink --wt 2 --keep-workflow-wd

And there's no failures. Run time 6m 38s. Is it too slow?

grst commented 2 years ago

It took 21:15 on my system, but I think most of the time was used to pull the singularity container.

On Thu, 25 Nov 2021 at 12:02, Lasse Folkersen @.***> wrote:

I checked theimputeme/vcftoprs module on an AWS instance, using latest nextflow, pullling like this

PROFILE=docker nextflow run tests/modules/imputeme/vcftoprs -entry test_imputeme_vcftoprs -c tests/config/nextflow.config

and this

NF_CORE_MODULES_TEST=1 PROFILE=docker pytest --tag imputeme/vcftoprs --symlink --wt 2 --keep-workflow-wd

And there's no failures. Run time 6m 38s. Is it too slow?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nf-core/modules/issues/1094#issuecomment-979095920, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABVZRVYICI5JJQO43VXAWYDUNYJSZANCNFSM5IWOX6LA .

drpatelh commented 2 years ago

Yes, I saw the same thing:

Caused by:
  Failed to pull singularity image
  command: singularity pull  --name containers.biocontainers.pro-s3-SingImgsRepo-imputeme-vv1.0.7_cv1-imputeme_vv1.0.7_cv1.img.img.pulling.1637839133887 https://containers.biocontainers.pro/s3/SingImgsRepo/imputeme/vv1.0.7_cv1/imputeme_vv1.0.7_cv1.img > /dev/null
  status : 143
  message:
    INFO:    Downloading network image

Which is weird because you can manually download by clicking the link.

grst commented 2 years ago

anyway, vcftoprs seems to work, it was just a timeout issue during the bulk tests.

mahesh-panchal commented 2 years ago

I would still be in favour of removing the closure if we don't need it for simplicity. Let's see what @mahesh-panchal thinks.

There's no closure there. It's only an expression evaluation. If it works outside the string then leave it out. I only tested the string version because task.ext couldn't be seen in an if statement, but I knew it worked it that context.

drpatelh commented 2 years ago

We couldn't get it working consistently without the evaluation anyway so will leave things as they are and update later if required.

rpetit3 commented 2 years ago

@JoseEspinosa you are correct on ncbigenomedownload, the test is downloading everything. The addParams was important for the test, we'll have add them in an alternate way or modify the recipe a little.

Originally the test looked like this

include { NCBIGENOMEDOWNLOAD } from '../../../modules/ncbigenomedownload/main.nf' addParams( options: [ args: '-A GCF_000013425.1 --formats genbank,fasta,assembly-stats bacteria '] )
JoseEspinosa commented 2 years ago

@rpetit3 they are now passed with this nextflow.config. I just checked the .command.sh and they are actually being passed:

$ cat .command.sh
#!/bin/bash -ue
ncbi-genome-download \
    -A GCF_000013425.1 --formats genbank,fasta,assembly-stats bacteria \
     \
    --output-folder ./ \
    --flat-output

cat <<-END_VERSIONS > versions.yml
test_ncbigenomedownload:NCBIGENOMEDOWNLOAD:
    ncbigenomedownload: $( ncbi-genome-download --version )
END_VERSIONS

Any ideas?

rpetit3 commented 2 years ago

You're right! Apologies completely missed that.

rpetit3 commented 2 years ago

Happy to report PIRATE is good again!

muffato commented 1 year ago

All but unicycler actually got fixed at some point since this issue was open. Since unicycler is already reported as needing a fix in #2154 , I'll close this issue here. No need to track it in two places