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

DRAFT: Update nf-core-modules 2 #686

Closed
wants to merge 4 commits into from

Conversation

asp8200
Copy link
Contributor

@asp8200 asp8200 commented Jul 25, 2022

It seems that are some problems with getting Sarek to use the updated nf-core-modules, in particular, BCFTOOLS_STATS. I made some changes in Sarek in order to get a vcf-tbi-file (for manta-tumor-only) to BCFTOOLS_STATS.

With this PR, I got the following test working:

nextflow run main.nf -profile test,tools_tumoronly,singularity --tools manta

which was failing in the previous PR #685, however, before continuing with this approach, I'd like some feedback.

If this approach is usable, then it will have to be extended to get all the tests working.

PR checklist

  • 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 pipeline conventions in the contribution docs
    • If necessary, also make a PR on the nf-core/sarek branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@@ -1000,7 +1000,7 @@ workflow SAREK {
ch_versions = ch_versions.mix(TUMOR_ONLY_VARIANT_CALLING.out.versions)

//QC
VCF_QC(vcf_to_annotate, intervals_bed_combined)
VCF_QC(vcf_to_annotate, TUMOR_ONLY_VARIANT_CALLING.out.manta_vcf_tbi, intervals_bed_combined)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know that command only works for the test for manta-tumor-only. I guess one would have to do something like the "mixed" vcf_to_annotate for the tbi-files.

@github-actions
Copy link

github-actions bot commented Jul 25, 2022

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit b46ecd9

+| ✅ 145 tests passed       |+
#| ❔   4 tests were ignored |#
!| ❗   1 tests had warnings |!

❗ Test warnings:

  • readme - README did not have a Nextflow minimum version badge.

❔ Tests ignored:

  • files_unchanged - File ignored due to lint config: assets/nf-core-sarek_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-sarek_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-sarek_logo_dark.png
  • files_unchanged - File ignored due to lint config: lib/NfcoreTemplate.groovy

✅ Tests passed:

Run details

  • nf-core/tools version 2.4.1
  • Run at 2022-07-25 16:52:11

target_bed

main:

ch_versions = Channel.empty()

BCFTOOLS_STATS(vcf)
BCFTOOLS_STATS(vcf, tbi, target_bed)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
BCFTOOLS_STATS(vcf, tbi, target_bed)
BCFTOOLS_STATS(vcf, [])

I would try just this actually

@@ -9,6 +9,8 @@ process BCFTOOLS_STATS {

input:
tuple val(meta), path(vcf)
tuple val(meta), path(vcf_tbi)
Copy link
Member

Choose a reason for hiding this comment

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

Why suggesting changes in the modules?

@@ -9,6 +9,8 @@ process BCFTOOLS_STATS {

input:
tuple val(meta), path(vcf)
Copy link
Contributor

@SusiJo SusiJo Jul 26, 2022

Choose a reason for hiding this comment

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

Suggested change
tuple val(meta), path(vcf)
tuple val(meta), path(vcf), path(tbi)

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep the naming as in the other modules (i.e. bcftools/consensus).
When everything is working please update the bcftools/stats module upstream.

Copy link
Contributor

@SusiJo SusiJo Jul 26, 2022

Choose a reason for hiding this comment

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

I had also thought about trying out sth like this to find the right index file in the script section:
INDEX=find -L ./ -name "${prefix}.vcf.tbi"`` and then passing the \$INDEX to the `bcftools/stats` command, but i could not get it to run due to nextflow/bash problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maxulysse : I'm including the tbi-file in the input of the module bcftools/stats because, without it, the following test

nextflow run main.nf -profile test,tools_tumoronly,singularity --tools manta

resulted in the following error:

  Process `NFCORE_SAREK:SAREK:VCF_QC:BCFTOOLS_STATS (sample2)` terminated with an error exit status (255)

Command executed:

  bcftools stats \
       \
      --regions-file multi_intervals.bed \
      sample2.manta.tumor_sv.vcf.gz > sample2.manta.tumor_sv.bcftools_stats.txt

  cat <<-END_VERSIONS > versions.yml
  "NFCORE_SAREK:SAREK:VCF_QC:BCFTOOLS_STATS":
      bcftools: $(bcftools --version 2>&1 | head -n1 | sed 's/^.*bcftools //; s/ .*$//')
  END_VERSIONS

Command exit status:
  255

Command output:
  (empty)

Command error:
  [E::idx_find_and_load] Could not retrieve index file for 'sample2.manta.tumor_sv.vcf.gz'
  Failed to read from sample2.manta.tumor_sv.vcf.gz: could not load index

Work dir:
  /home/aspe/dev/fork_sarek/work/9b/0c3268ad4e17ee489871c6849292ff

@asp8200
Copy link
Contributor Author

asp8200 commented Jul 26, 2022

Hi @SusiJo and @maxulysse : Sorry about this mess and thanks a lot for all your comments and links to previous discussions. I'll take a careful look at it all.

@asp8200
Copy link
Contributor Author

asp8200 commented Jul 26, 2022

@maxulysse and @SusiJo : If I just pass an empty list ([]) as the second parameter to BCFTOOLS_STATS then the following test passes:

nextflow run main.nf -profile test,tools_tumoronly,singularity --tools manta

Is that an okay solution for you guys?

@FriederikeHanssen mentioned that with the target-bed-file supplied for wes-samples, BCFTOOLS_STATS will produce a depth-plot:

#638 (comment)

Perhaps we don't need that depth-plot for now? How about we just use an empty list ([]) as the second parameter to BCFTOOLS_STATS in this PR? I think that would keep this PR a lot simpler :-) If necessary, we can then, in a later PR, try to have Sarek pass in the tbi-files to BCFTOOLS_STATS?

@maxulysse
Copy link
Member

@maxulysse and @SusiJo : If I just pass an empty list ([]) as the second parameter to BCFTOOLS_STATS then the following test passes:

nextflow run main.nf -profile test,tools_tumoronly,singularity --tools manta

Is that an okay solution for you guys?

Sounds ok enough.

@FriederikeHanssen mentioned that with the target-bed-file supplied for wes-samples, BCFTOOLS_STATS will produce a depth-plot:

#638 (comment)

Perhaps we don't need that depth-plot for now? How about we just use an empty list ([]) as the second parameter to BCFTOOLS_STATS in this PR? I think that would keep this PR a lot simpler :-) If necessary, we can then, in a later PR, try to have Sarek pass in the tbi-files to BCFTOOLS_STATS?

OK for doing that in a separate PR

@asp8200 asp8200 mentioned this pull request Jul 27, 2022
10 tasks
@asp8200
Copy link
Contributor Author

asp8200 commented Jul 27, 2022

We are going for the solution in PR.

@asp8200 asp8200 closed this Jul 27, 2022
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.

3 participants