methylseq icon indicating copy to clipboard operation
methylseq copied to clipboard

Published 20 hours ago • verified publisher icon nf-core

Template update, big DSL2 modules update

Open ewels opened this issue 3 years ago • 5 comments

My attempt at getting the DSL2 port of this pipeline over the line.

A lot has changed since @phue originally worked on this. For starters, the DSL2 template is now officially released. This meant that I merged in the TEMPLATE commits from the sync (which had a lot of merge conflicts as I think it was all copied in before without git history). There are also a lot of changes in modules syntax, and many previously local modules are now merged in to the central repo.

WIP - been manually trying to merge this all but untested so far, so still quite a way to go.

Note: This PR is to the dsl2 branch and not dev I hope that when this is merged we can then get on to merging #199 soon after and then release.

When this is merged, it will encompass #220 and so that will close automatically immediately.

  • [x] Fix merge conflicts from template sync
  • [x] Remove local modules and replace with nf-core/modules
  • [x] Strip out all modules and reinstall with new modules.json
  • [ ] Check that there aren't any now-dead files left hanging after the template update
  • [ ] Go through linting errors
  • [ ] Go through test pipeline errors
  • [ ] Final test for review

ewels avatar Nov 04 '21 16:11 ewels

nf-core lint overall result: Passed :white_check_mark: :warning:

Posted for pipeline commit 9d13eb0

+| ✅ 135 tests passed       |+
#| ❔   1 tests were ignored |#
!| ❗   7 tests had warnings |!

:heavy_exclamation_mark: Test warnings:

  • readme - README did not have a Nextflow minimum version mentioned in Quick Start section.
  • pipeline_todos - TODO string in awsfulltest.yml: You can customise AWS full pipeline tests as required
  • pipeline_todos - TODO string in check_samplesheet.py: Update the script to check the samplesheet
  • pipeline_todos - TODO string in WorkflowMain.groovy: Add Zenodo DOI for pipeline after first release
  • pipeline_todos - TODO string in methylseq.nf: Add all file path parameters for the pipeline to the list below
  • pipeline_todos - TODO string in base.config: Check the defaults for all processes
  • pipeline_todos - TODO string in base.config: Customise requirements for specific processes.

:grey_question: Tests ignored:

:white_check_mark: Tests passed:

Run details

  • nf-core/tools version 2.1
  • Run at 2021-11-08 08:25:21

github-actions[bot] avatar Nov 04 '21 21:11 github-actions[bot]

Hi @ewels,

Thanks for picking this up again, I hadn't looked at this for a (quite a) while.

Just as a heads up, there are few caveats regarding the removal of local modules:

There were 2 main reasons why I went with local modules for this: i) relates to the discussion here: https://github.com/nf-core/modules/pull/239#discussion_r594618090 To keep pipeline-specific code out of nf-core/modules, I went with just keeping those memory/cpu settings in the local modules as the logic is not only scary but also depends on library presets as they are defined in the pipeline code. But it is not optimal, because the generic module lacks multithreading completely because of that.

ii) relates to https://github.com/nf-core/methylseq/issues/181 (some discussion here: https://github.com/nf-core/modules/pull/129#issuecomment-774997513) The current implementation on the dsl2 branch relies on the meta map being attached to not only samples, but also the reference genome to enable mapping against multiple pseudogenomes (or assemblies) in one pipeline run. See here for example: https://github.com/ewels/nf-core-methylseq/blob/dsl2-template-merge/subworkflows/local/bismark.nf#L54-L61

phue avatar Nov 04 '21 23:11 phue

Hi @phue!

Great to hear from you 😀 Massive kudos for the work you did here with the DSL2 conversion, sorry it's taken me so long to get around to helping you with it.

i) Ok so I think I'm starting to understand this now. I get the reasoning for keeping the module as simple and generic as possible. But does the pipeline need to have a local module for this? Can it not just append to the $options.args like other flags and use the central module?

ii) Ahhh, you mean this subtle little difference?

-    tuple val(meta), path(bam), path(index)
+    tuple val(meta), path(bam)
+    path index

I totally missed that.. Right, and the objection for having it in nf-core/modules with that style is that it's not consistent with other modules / too complex?

Ok I see both standpoints. I kind of want to avoid having local versions of modules that are also available in the central nf-core/modules repo though. I think that it's inevitable that there will be feature drift between them and it kind of loses the advantages of having the DSL2 modules. I'll take it up on Slack and see if there's a way that we can resolve this.

ewels avatar Nov 05 '21 12:11 ewels

sorry it's taken me so long to get around to helping you with it.

No worries, great that you brought it up again so we can have this discussion and get this thing finalized :+1:

i) Can it not just append to the $options.args like other flags and use the central module?

Yes, that's probably the cleaner option. Having a groovy function to do these memory/core calculations would be even better. I just went with the local modules because they were anyway required for ii)

Right, and the objection for having it in nf-core/modules with that style is that it's not consistent with other modules / too complex?

I don't think it's complex or anything, consistency is what mattered in the discussion back then. It would have required to extend the meta map concept beyond samples but also to references/indices etc. Doing this change now across modules would be even more cumbersome as it would likely break existing dsl2 pipeline code.

phue avatar Nov 06 '21 12:11 phue

Reminder to myself: Need to implement the new versioning code. Docs: https://nf-co.re/developers/adding_modules#new-module-guidelines-and-pr-review-checklist + check rnaseq pipeline.

ewels avatar Nov 08 '21 10:11 ewels