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

Move code from main workflow to subworkflows #446

Closed
wants to merge 8 commits into from
Closed

Move code from main workflow to subworkflows #446

wants to merge 8 commits into from

Conversation

prototaxites
Copy link
Contributor

@prototaxites prototaxites commented May 18, 2023 •
edited
Loading

Creates a number of new subworkflows for separate 'stages' of the pipeline: short read pre-processing, long read pre-processing, short-read taxonomy, assembly, assembly QC (Quast), bin QC, bin taxonomy, and annotation.

Tidies the workflow particularly around the assembly input (as discussed here: #439) to avoid nested if-elses where possible and use empty channel skipping.

I moved the various DB channel resolving components to their respective subworkflows - this simplifies the architecture a bit by not having to pass DBs as arguments to subworkflows, but perhaps reduces their re-usability elsewhere. Not sure what the preferred style is here but this can be easily changed.

Subworkflow names suggestions only 😅

PR checklist

  • 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 pipeline conventions in the contribution docs- [ ] If necessary, also make a PR on the nf-core/mag branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@github-actions
Copy link

github-actions bot commented May 18, 2023 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit d1db73b

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

❗ Test warnings:

  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your prefered methods description, e.g. add publication citation for this pipeline

❔ Tests ignored:

  • files_unchanged - File ignored due to lint config: lib/NfcoreTemplate.groovy

✅ Tests passed:

Run details

  • nf-core/tools version 2.8
  • Run at 2023-06-13 10:24:52

@jfy133 jfy133 added this to the 2.4.0 milestone May 26, 2023
@jfy133
Copy link
Member

jfy133 commented Jun 7, 2023

FYI This is still on my radar @prototaxites - I just need to find more a space of more than 1h to be able to sit and focus on this as it's sort of a large refactoring 😬 (but very needed and welcome!)

@jfy133
Copy link
Member

jfy133 commented Jun 13, 2023

@prototaxites I just realised this will need a merge-conflict resolve since run-merging went in, sorry 🤦

@prototaxites
Copy link
Contributor Author

Was on my radar - should be fixed now!

@jfy133 jfy133 modified the milestones: 2.4.0, 2.5.0 Jun 23, 2023
@jfy133 jfy133 marked this pull request as draft June 23, 2023 11:45
@prototaxites
Copy link
Contributor Author

Had a thought while running umpteen gels in the lab today - taxprofiler and mag both take the same type of input data, and have very similar pre-processing steps: fastqc -> fastp -> (taxprofiler only: complexity filtering) -> host removal -> (mag only: phix removal) -> fastqc.

Would it make sense as part of a larger refactoring to consider spinning (some of) these parts into installable subworkflows, so that the exact same code could be shared between the pipelines?

@jfy133
Copy link
Member

jfy133 commented Jul 14, 2023

Yes very much so!

@prototaxites
Copy link
Contributor Author

Going to suggest we close this PR and revisit down the line. The pipeline has changed a lot with 2.4.0, and while I think it's definitely a good plan to break the pipeline in to subworkflows, I suspect it would be better to visit each independently and perhaps a bit of a roadmap.

@jfy133
Copy link
Member

jfy133 commented Sep 28, 2023

Yes sounds good 👍

@jfy133 jfy133 closed this Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@prototaxites @jfy133