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

Use channel topic for tool versions #1109

Draft
wants to merge 3 commits into
base: dev
Choose a base branch
from
Draft

Use channel topic for tool versions #1109

wants to merge 3 commits into from

Conversation

bentsherman
Copy link

@bentsherman bentsherman commented Nov 9, 2023 •
edited
Loading

This PR refactors the pipeline to use an experimental topic channel (nextflow-io/nextflow#4459) to collect the tool versions.

The topic channel works, but multiqc is failing with this error:

@bentsherman bentsherman mentioned this pull request Nov 9, 2023
Merged
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 9, 2023 •
edited
Loading

nf-core lint overall result: Failed ❌

Posted for pipeline commit 8f7e5bd

+| ✅ 144 tests passed       |+
#| ❔   6 tests were ignored |#
!| ❗   4 tests had warnings |!
-| ❌   1 tests failed       |-

❌ Test failures:

  • actions_ci - Minimum pipeline NF version '23.04.0' is not tested in '.github/workflows/ci.yml'

❗ Test warnings:

  • files_exist - File not found: .github/workflows/awstest.yml
  • files_exist - File not found: .github/workflows/awsfulltest.yml
  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your preferred methods description, e.g. add publication citation for this pipeline
  • pipeline_todos - TODO string in WorkflowRnaseq.groovy: Optionally add in-text citation tools to this list.

❔ Tests ignored:

  • files_unchanged - File ignored due to lint config: assets/email_template.html
  • files_unchanged - File ignored due to lint config: assets/email_template.txt
  • files_unchanged - File ignored due to lint config: lib/NfcoreTemplate.groovy
  • files_unchanged - File ignored due to lint config: .gitignore or .prettierignore or pyproject.toml
  • actions_awstest - 'awstest.yml' workflow not found: /home/runner/work/rnaseq/rnaseq/.github/workflows/awstest.yml
  • multiqc_config - multiqc_config

✅ Tests passed:

Run details

  • nf-core/tools version 2.10
  • Run at 2023-12-12 20:14:54

@pditommaso
Copy link
Contributor

-225 lines!

@ewels
Copy link
Member

ewels commented Nov 12, 2023

I think you managed to hit the small window when the MultiQC bioconda build was broken. If you try again now it should be fine 🤞🏻

@ewels
Copy link
Member

ewels commented Nov 12, 2023

This is awesome! Do you think it'd be possible to go even further and use a topic to pick up all of the MultiQC inputs, not just the versions?

@drpatelh
Copy link
Member

This is indeed awesome!! 🤩

When this is out in a release, we will first need to bump the minimum version of NF on nf-core/modules as well as in pipeline repos. Might need to delay a little to enable users to bump their NF version installs.

Do you think it'd be possible to go even further and use a topic to pick up all of the MultiQC inputs, not just the versions?

Presumably, you would need to explicitly add topic: multiqc within all relevant modules for this to work?

output:
path "<SOME_MULTIQC_FILE", topic: multiqc

Is it possible to use emit and topic together in an output channel? There may be some instances where you want to use the output of a module in a workflow independent of a topic.

@bentsherman
Copy link
Author

bentsherman commented Nov 13, 2023 •
edited
Loading

I think you managed to hit the small window when the MultiQC bioconda build was broken

Okay I deleted the conda environment for multiqc but still got the error with a new build... does conda use a local cache that could be preventing a fresh download?

When this is out in a release, we will first need to bump the minimum version of NF on nf-core/modules as well as in pipeline repos.

This will be the challenge. It might be better to only patch the nf-core modules for the rnaseq pipeline and update the modules after another stable release (or two).

Is it possible to use emit and topic together in an output channel? There may be some instances where you want to use the output of a module in a workflow independent of a topic.

Currently it is not possible, not sure how difficult it would be to support. I can look at the multiqc inputs and see if a topic channel would be appropriate.

@bentsherman
Copy link
Author

In any case, I tested rnaseq on master and it fails with the same error, so that at least makes me confident that there's no issue with the topic channels

@bentsherman
Copy link
Author

As for the multiqc inputs, a topic might not be the best choice because it is not obvious that the corresponding process outputs are intended solely for multiqc.

For example, the FASTP process:

    output:
    tuple val(meta), path('*.fastp.fastq.gz') , optional:true, emit: reads
    tuple val(meta), path('*.json')           , emit: json
    tuple val(meta), path('*.html')           , emit: html
    tuple val(meta), path('*.log')            , emit: log
    path "versions.yml"                       , topic: versions
    tuple val(meta), path('*.fail.fastq.gz')  , optional:true, emit: reads_fail
    tuple val(meta), path('*.merged.fastq.gz'), optional:true, emit: reads_merged

Some of these outputs are sent to multiqc, but you couldn't tell from the process definition. I think forcing a channel topic here would be more trouble than it's worth.

@bentsherman
Use a topic channel for tool versions
18f6b75 
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
@bentsherman bentsherman changed the base branch from master to dev November 14, 2023 21:49
@nf-core nf-core deleted a comment from github-actions bot Nov 14, 2023
@bentsherman bentsherman mentioned this pull request Nov 15, 2023
Draft
7 tasks
@bentsherman
Copy link
Author

Shall we move this forward? Channel topics are available in 23.11.0-edge. Let me know how you want to handle it. Or maybe someone from nf-core can take it forward, don't really need me anymore 😄

Is it possible to use emit and topic together in an output channel? There may be some instances where you want to use the output of a module in a workflow independent of a topic.

I was wrong about this, you can use emit and topic together. So if you guys want you can add emit: ch_versions back to the process outputs.

Also would love to hear back about my comment regarding the use of env output versus the experimental cmd output. If you prefer the env approach then we can implement that right away, for the cmd approach we can implement separately if/when it is merged.

@bentsherman bentsherman changed the title Use topic channel for tool versions Use channel topic for tool versions Dec 11, 2023
@bentsherman bentsherman marked this pull request as ready for review December 11, 2023 14:57
@bentsherman
Update CI
ed5cf57 
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
@bentsherman
Merge remote-tracking branch 'upstream/dev' into topic-channel
8f7e5bd 
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
@bentsherman
Copy link
Author

I updated the CI and tests are passing now. The failing tests should work if they use 23.11.0-edge, but not sure how you wanted to manage that.

@maxulysse maxulysse marked this pull request as draft July 16, 2024 09:17
@edmundmiller edmundmiller mentioned this pull request Aug 1, 2024
Open
4 tasks
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
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bentsherman @pditommaso @ewels @drpatelh