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

Update subworkflow for deepvariant to version 1.8.0 #7473

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

Conversation

fa2k
Copy link
Contributor

@fa2k fa2k commented Feb 12, 2025

Update the deepvariant variant calling subworkflow (nf-core/deepvariant) to version 1.8.0.

Other module subcommands were updated in #7301.

This work is based on @fellen31's original pull request to update the subworkflow, and adds some handling of the "small model". Summary of changes:

  • Container version increased
  • Enable --regions parameter to DEEPVARIANT_POSTPROCESSVARIANTS process
  • Pass the small model outputs from DEEPVARIANT_MAKEEXAMPLES to DEEPVARIANT_POSTPROCESSVARIANTS if small model is used
  • Update tests to use small model (which is default), but add a test to disable the small model
  • Add a test to ensure that the subworkflow output is equal to the integrated DEEPVARIANT_RUNDEEPVARIANT process output

The future of the subworkflow in nf-core is a bit uncertain. Recent updates of DeepVariant have required an excessive amount of changes to be made in the subworkflow. Due to the argument handling with ext.args, they would also require many changes by the pipelines using the subworkflow, to keep up with the default arguments used by the run_deepvariant script.

The purpose of this subworkflow is to optimize compute resource usage, allocating GPUs only when needed. With version 1.8.0, Google released a new "fast pipeline" (https://github.com/google/deepvariant/blob/r1.8/docs/deepvariant-fast-pipeline-case-study.md) that could solve the same problem in a different way. At the moment, the fast pipeline is about as complex as the subworkflow approach, so I'm suggesting we keep the subworkflow for now.

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

@fa2k fa2k requested a review from fellen31 February 12, 2025 12:56
@fellen31
Copy link
Contributor

Looks like some tests are failing?

@fa2k
Copy link
Contributor Author

fa2k commented Feb 14, 2025

Oh sorry I missed that. I will check it and let you know when it's fixed. It may be related to the nextflow version.

@fa2k
Copy link
Contributor Author

fa2k commented Feb 19, 2025

@fellen31 I've fixed the test for POSTPROCESSVARIANTS, adding the correct input tuple. Now all the tests pass.

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.

2 participants