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.

Sign up for GitHub

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

Jump to bottom

port parabricks/deepvariant to nf-test #6995

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

port parabricks/deepvariant to nf-test #6995

wants to merge 24 commits into from

Conversation

famosab
Copy link
Contributor

@famosab famosab commented Nov 14, 2024 •
edited
Loading

PR checklist

Closes #XXX

  • 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

@famosab famosab requested a review from a team as a code owner November 14, 2024 14:31
@famosab famosab requested a review from mirpedrol November 14, 2024 14:31
@famosab
Copy link
Contributor Author

famosab commented Nov 14, 2024

Currently assessing the created g.vcf always fails with

╭────────────────────────────────────────────────────────────────────────────────────────────────────────────── nf-test error ───────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ groovy.lang.MissingMethodException: No signature of method: static com.askimed.nf.test.lang.extensions.GlobalMethods.file() is applicable for argument types: (ArrayList) values: [[/home/paifb01/modules/.nf-test/tests/2608642d653b0463a │
│ Possible solutions: file(java.lang.String), find(), find(groovy.lang.Closure), use([Ljava.lang.Object;), with(groovy.lang.Closure), is(java.lang.Object)                                                                                   │
│                                                                                                                                                                                                                                            │
╰────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯

@famosab famosab added gpu module uses GPU nf-test help wanted Extra attention is needed labels Nov 18, 2024
@famosab famosab added Ready for Review and removed help wanted Extra attention is needed labels Dec 16, 2024
SPPearce
SPPearce reviewed Jan 8, 2025
View reviewed changes
modules/nf-core/parabricks/deepvariant/main.nf Outdated
@@ -1,40 +1,41 @@
process PARABRICKS_DEEPVARIANT {
tag "$meta.id"
label 'process_high'
label 'process_gpu'
stageInMode 'copy' // needed by the module to work properly can be removed when fixed upstream
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an issue that could be linked here, to make it easy to check if it has been fixed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#7226

modules/nf-core/parabricks/deepvariant/main.nf

input:
tuple val(meta), path(input), path(input_index), path(interval_file)
tuple val(ref_meta), path(fasta)

output:
tuple val(meta), path("*.vcf"), emit: vcf
path "versions.yml", emit: versions
tuple val(meta), path("*.vcf"), optional: true, emit: vcf
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this one not zipped?

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'll update it

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 think deepvariant gives either a vcf or a g.vcf.gz file. Otherwise the container needs to be changed and I am unsure how to do that with parabricks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but we can just gzip it afterwards (with whatever the magic way of doing that is for vcf files), presumably these vcf files will be large.

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 think we need to install bgzip to the container then.

modules/nf-core/parabricks/deepvariant/meta.yml
@@ -53,12 +53,24 @@ output:
e.g. [ id:'test' ]
- "*.vcf":
type: file
description: Variant file.
description: vcf file created with deepvariant (does not support .gz for normal vcf), optional
Copy link
Contributor

Choose a reason for hiding this comment

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

But could be gzipped afterwards, other modules do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SPPearce SPPearce SPPearce left review comments

@mirpedrol mirpedrol Awaiting requested review from mirpedrol mirpedrol is a code owner automatically assigned from nf-core/a-team

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
gpu module uses GPU nf-test Ready for Review
Projects
nf-test Migration
Status: Todo
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@famosab @SPPearce