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

Template update, big DSL2 modules update #222

Closed
wants to merge 15 commits into from
Closed

Template update, big DSL2 modules update #222

wants to merge 15 commits into from

Conversation

ewels
Copy link
Member

@ewels ewels commented Nov 4, 2021 •
edited
Loading

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.

  • Fix merge conflicts from template sync
  • Remove local modules and replace with nf-core/modules
  • 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 ewels added the WIP Work in progress label Nov 4, 2021
@ewels ewels requested a review from phue November 4, 2021 16:00
@ewels ewels mentioned this pull request Nov 4, 2021
Merged
3 tasks
@github-actions
Copy link

github-actions bot commented Nov 4, 2021 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 9d13eb0

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

❗ 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.

❔ Tests ignored:

✅ Tests passed:

Run details

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

ewels
ewels commented Nov 4, 2021
View reviewed changes
Copy link
Member Author

@ewels ewels left a comment

Choose a reason for hiding this comment

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

Self-review

@phue
Copy link
Member

phue commented Nov 4, 2021

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: nf-core/modules#239 (comment)
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 #181 (some discussion here: nf-core/modules#129 (comment))
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

@ewels
Copy link
Member Author

ewels commented Nov 5, 2021

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.

@phue
Copy link
Member

phue commented Nov 6, 2021 •
edited
Loading

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 👍

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.

@ewels
Copy link
Member Author

ewels commented Nov 8, 2021

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 ewels mentioned this pull request Apr 1, 2022
Merged
@ewels ewels closed this Sep 14, 2022
@ewels ewels deleted the dsl2-template-merge branch September 14, 2022 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phue phue Awaiting requested review from phue

Assignees
No one assigned
Labels
WIP Work in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ewels @phue @KevinMenden @nf-core-bot