modules icon indicating copy to clipboard operation
modules copied to clipboard

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

Open drpatelh opened this issue 3 years ago • 18 comments

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

New

  • [ ] maltextract (likely just error staging foreign file)
  • [ ] snpeff (No such variable task -> actual problem)
  • [ ] ensemblvep (No such variable task -> actual problem)

Still failing

  • [ ] yara/mapper
  • [ ] unicycler
  • [ ] freebayes
  • [ ] cooler/zoomify
  • [ ] fgbio/fastqtobam
  • [ ] ncbigenomedownload (@JoseEspinosa) (taking for ever and does not throw any error, I have the feeling it downloads everything NCBI has to offer :sweat_smile: )
  • [ ] tbprofiler/profile (@drpatelh) (taking for ever and does not throw any error)

Fixed at module level

  • [x] arriba (@JoseEspinosa)
  • [x] bcftools/query (@JoseEspinosa )
  • [x] bowtie/align (@drpatelh)
  • [x] bowtie2/align (@drpatelh)
  • [x] bwa/aln (@drpatelh)
  • [x] bwa/mem (@drpatelh)
  • [x] bwamem2/mem (@drpatelh)
  • [x] cooler/merge (@JoseEspinosa)
  • [x] cooler/zoomify (@JoseEspinosa)
  • [x] csvtk/split (@JoseEspinosa)
  • [x] delly/call (@JoseEspinosa)
  • [x] fgbio/fastqtobam (@drpatelh)
  • [x] freebayes (@maxulysse)
  • [x] gatk4/intervallisttools (@JoseEspinosa)
  • [x] gatk4/variantfiltration (@JoseEspinosa)
  • [x] glnexus (@drpatelh)
  • [x] gstama/collapse (@JoseEspinosa)
  • [x] hisat2/align (@drpatelh)
  • [x] hisat2/build (@drpatelh)
  • [x] jupyternotebook (@grst)
  • [x] kallistobustools/count (@JoseEspinosa)
  • [x] kallistobustools/ref (@JoseEspinosa)
  • [x] leehom (@JoseEspinosa)
  • [x] metaphlan3 (@JoseEspinosa)
  • [x] pairtools/select (@JoseEspinosa)
  • [x] plink2/vcf (@drpatelh)
  • [x] pydamage/filter (@JoseEspinosa)
  • [x] rmarkdownnotebook (@grst)
  • [x] roary (@rpetit3) (#1418)
  • [x] rsem/calculateexpression (@drpatelh)
  • [x] salmon/quant (@drpatelh)
  • [x] sratools/fasterqdump (@drpatelh)
  • [x] unicycler (@JoseEspinosa)
  • [x] yara/mapper (@drpatelh)
  • [x] imputeme/vcftoprs (@lassefolkersen ) (Singularity timeout)
  • [x] clonalframeml (@drpatelh) (@rpetit3)
  • [x] hicap (@drpatelh) (@rpetit3)
  • [x] pirate (@drpatelh) (@rpetit3) (fixed test data but need to update test.yml)(#1444)

Already passing on local tests

  • [x] chromap/chromap (@drpatelh)
  • [x] gatk4/mergebamalignment (@JoseEspinosa)
  • [x] fgbio/groupreadsbyumi (@drpatelh)
  • [x] nanoplot (@JoseEspinosa)
  • [x] plasmidid (@drpatelh)
  • [x] prokka (@JoseEspinosa) (worked with Docker but conda env was not resolved)
  • [x] shovill (@JoseEspinosa) (These two relative big test files do not stage properly sometimes and tests break then)

Failing at sub-workflow level

  • [x] samtools/flagstat (@drpatelh)
  • [x] samtools/idxstats (@drpatelh)
  • [x] samtools/index (@drpatelh)
  • [x] samtools/sort (@drpatelh)
  • [x] samtools/stats (@drpatelh)
  • [x] sratools/prefetch (@drpatelh)

drpatelh avatar Nov 24 '21 16:11 drpatelh

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

JoseEspinosa avatar Nov 24 '21 18:11 JoseEspinosa

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?

drpatelh avatar Nov 24 '21 19:11 drpatelh

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... 🤔

JoseEspinosa avatar Nov 24 '21 20:11 JoseEspinosa

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

drpatelh avatar Nov 24 '21 20:11 drpatelh

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... 🤔

JoseEspinosa avatar Nov 24 '21 21:11 JoseEspinosa

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.

drpatelh avatar Nov 24 '21 22:11 drpatelh

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`

JoseEspinosa avatar Nov 24 '21 22:11 JoseEspinosa

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.

drpatelh avatar Nov 24 '21 22:11 drpatelh

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?

lassefolkersen avatar Nov 25 '21 11:11 lassefolkersen

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 .

grst avatar Nov 25 '21 11:11 grst

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.

drpatelh avatar Nov 25 '21 11:11 drpatelh

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

grst avatar Nov 25 '21 11:11 grst

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.

mahesh-panchal avatar Nov 25 '21 14:11 mahesh-panchal

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

drpatelh avatar Nov 25 '21 14:11 drpatelh

@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 '] )

rpetit3 avatar Nov 25 '21 17:11 rpetit3

@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?

JoseEspinosa avatar Nov 25 '21 17:11 JoseEspinosa

You're right! Apologies completely missed that.

rpetit3 avatar Nov 25 '21 17:11 rpetit3

Happy to report PIRATE is good again!

rpetit3 avatar Mar 25 '22 02:03 rpetit3

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

muffato avatar Jan 20 '23 04:01 muffato