Repository navigation
Feature/prs modules - #10
Conversation
Feature/modules test
… into feature/prs_modules
Feature/variant calling prs
topic versions
rernst
left a comment
rernst
left a comment
There was a problem hiding this comment.
Looks good, some minor remarks / discussion points and comments:
- Add version to (python) tools instead of nextflow module.
- Update diff comparisons to reflect update modules -> maybe evaluate whether we still need to 'copy' these nf-core modules in our own repo (out of scope for this PR)?
- I think it would be a good idea to create a separate 'prsutils' repo for the python tools in that module. Comparable to what we want to do with for example DIMS.
| # Required for nft-utils nfcoreInstall() to install nf-core modules | ||
| # during test setup (provides the `nf-core` CLI on PATH). | ||
| shell: bash | ||
| run: python -m pip install --upgrade --force-reinstall git+https://github.com/UMCUGenetics/nf-core-tools.git |
There was a problem hiding this comment.
Do we still need our own checkout -> if so, we should make sure to keep it up to date.
There was a problem hiding this comment.
It is worth to discus this. Technically, no we don't need it. But using the unforked nf-core tools would require us to add more metadata and a few other required fields in main.nf. Most notably a meta.yml file for each module/subworkflow. In my opinion it it very useful to have documentation alongside each entity, but that would involve some extra work on the development side as a tradeoff.
I've made an example PR that highlights the differences between the linting requirements of nf-core/tools and our fork: https://github.com/UMCUGenetics/NF-Modules/pull/13/changes
There was a problem hiding this comment.
Why do we need our own custom GATK module? Do we use a different version?
There was a problem hiding this comment.
For PRS we need the input tuple val(meta7), path(alleles), path(alleles_tbi) which is not present in the nf-core version of the module by default. In DxNextflowPRS I previously added this input and generated a path file with nf-core modules patch, but the module patch does not carry over when installing the subworkflow from this modules repository into a pipeline.
Hence I vendored the module and renamed it to GATK4_HAPLOTYPECALLERALLELES to disambiguate. A cleaner solution would be to fix this upstream and add the alleles input as an optional input in the nf-core/modules repo. However, that is a lot of work, as it would require me to also check all subworkflows and other modules in the nf-core repo that use GATK4_HAPLOTYPECALLER and update those as well
There was a problem hiding this comment.
Should we add a meta.yml for our modules as well? Is there any added value?
There was a problem hiding this comment.
This ties in with the thread over here: https://github.com/UMCUGenetics/NF-Modules/pull/10/changes#r3702790926
tldr: documentation has added value, but it involves more work on the development side. What do you think?
Jorisvansteenbrugge
left a comment
Jorisvansteenbrugge
left a comment
There was a problem hiding this comment.
Comments from @rernst have been addressed, a few discussion points remain.
| # Required for nft-utils nfcoreInstall() to install nf-core modules | ||
| # during test setup (provides the `nf-core` CLI on PATH). | ||
| shell: bash | ||
| run: python -m pip install --upgrade --force-reinstall git+https://github.com/UMCUGenetics/nf-core-tools.git |
There was a problem hiding this comment.
It is worth to discus this. Technically, no we don't need it. But using the unforked nf-core tools would require us to add more metadata and a few other required fields in main.nf. Most notably a meta.yml file for each module/subworkflow. In my opinion it it very useful to have documentation alongside each entity, but that would involve some extra work on the development side as a tradeoff.
I've made an example PR that highlights the differences between the linting requirements of nf-core/tools and our fork: https://github.com/UMCUGenetics/NF-Modules/pull/13/changes
There was a problem hiding this comment.
For PRS we need the input tuple val(meta7), path(alleles), path(alleles_tbi) which is not present in the nf-core version of the module by default. In DxNextflowPRS I previously added this input and generated a path file with nf-core modules patch, but the module patch does not carry over when installing the subworkflow from this modules repository into a pipeline.
Hence I vendored the module and renamed it to GATK4_HAPLOTYPECALLERALLELES to disambiguate. A cleaner solution would be to fix this upstream and add the alleles input as an optional input in the nf-core/modules repo. However, that is a lot of work, as it would require me to also check all subworkflows and other modules in the nf-core repo that use GATK4_HAPLOTYPECALLER and update those as well
There was a problem hiding this comment.
This ties in with the thread over here: https://github.com/UMCUGenetics/NF-Modules/pull/10/changes#r3702790926
tldr: documentation has added value, but it involves more work on the development side. What do you think?
2 participants
The modules and subworkflows from DxNextflowPRS have been moved to the central Modules repository. A few small refactors have been made to make the subworkflows more 'stand alone', but 90% of the content of this PR was previously reviewed and is already used in production (https://github.com/UMCUGenetics/DxNextflowPRS/tree/main)