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.

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

Start september/october maintainer minutes #2819

Merged
merged 23 commits into from
Oct 29, 2024
Merged
Changes from 6 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
e2717ab
Start october minutes
jfy133 Oct 27, 2024
ed0df90
Add template sections for Maxime
jfy133 Oct 27, 2024
ff79378
Update sites/main-site/src/content/blog/2024/maintainers-minutes-2024…
mashehu Oct 27, 2024
be218b8
Apply suggestions from code review
mashehu Oct 27, 2024
8da9f52
Finish Oct. sections
jfy133 Oct 27, 2024
1806650
[automated] Fix code linting
nf-core-bot Oct 27, 2024
cb78626
Apply suggestions from code review
sateeshperi Oct 27, 2024
3c13d82
Apply suggestions from code review
sateeshperi Oct 27, 2024
7a6f1aa
[automated] Fix code linting
nf-core-bot Oct 27, 2024
f8114d5
add author and formatting tidy
sateeshperi Oct 27, 2024
265968d
Merge branch 'main' into maintainers-minutes-oct24
jfy133 Oct 27, 2024
4201a5e
update minutes
sateeshperi Oct 28, 2024
89c0ba1
Merge branch 'main' into maintainers-minutes-oct24
sateeshperi Oct 28, 2024
5bc6946
Apply suggestions from code review
sateeshperi Oct 28, 2024
967b05a
Merge branch 'main' into maintainers-minutes-oct24
sateeshperi Oct 28, 2024
de252ec
[automated] Fix code linting
nf-core-bot Oct 28, 2024
f98ef0c
Update maintainers-minutes-2024-10-25.mdx
maxulysse Oct 28, 2024
bc0e181
[automated] Fix code linting
nf-core-bot Oct 28, 2024
80805be
Update sites/main-site/src/content/blog/2024/maintainers-minutes-2024…
maxulysse Oct 28, 2024
ae4c268
Apply suggestions from code review
jfy133 Oct 28, 2024
09d1736
Apply suggestions from code review
jfy133 Oct 28, 2024
c64df6e
Update sites/main-site/src/content/blog/2024/maintainers-minutes-2024…
mahesh-panchal Oct 29, 2024
77481b1
Merge branch 'main' into maintainers-minutes-oct24
jfy133 Oct 29, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
---
title: 'Maintainers Minutes: September/October 2024'
subtitle: 'Keeping you informed of the latest maintainers discussions'
pubDate: 2024-10-28T12:00:00+01:00
headerImage: '/assets/images/blog/maintainers-minutes-2024-07-26/maintainers_duck.png'
headerImageAlt: 'Cartoon yellow rubber duck with nf-core logo badge on its body with the nf-core logo.'
headerImageDim: [250, 250]
embedHeaderImage: true
authors:
- 'jfy133'
- 'MaxUlysse'
maxulysse marked this conversation as resolved.
Show resolved Hide resolved
label:
- 'maintainers'
---

import Profile from '@components/GitHubProfilePictureExtended.svelte';
import { Image } from 'astro:assets';

The 'Maintainers Minutes' aims to further give insight into the workings of the [nf-core maintainers team](/governance#maintainers)
jfy133 marked this conversation as resolved.
Show resolved Hide resolved
by providing brief summaries of the monthly team meetings.

## Overview

In this double post, we discussed the following topics in September and October:
jfy133 marked this conversation as resolved.
Show resolved Hide resolved
jfy133 marked this conversation as resolved.
Show resolved Hide resolved

- XXX SEPT POINT
- XXX SEPT PONINT
- [New way of passing parameters to modules within nf-test scripts](new-way-of-passing-parameters-to-modules-within-nf-test-scripts)
- [Stalled PRs due to waiting for review](stalled-prs-due-to-waiting-for-review)
- [Replacing monolithic `conf/modules.conf` into per module-configs](replacing-monolithic-confmodulesconf-into-per-module-configs)
- [Modification of modules specifications regarding channels](modifications-of-modules-specifications-regarding-channels)

### XXX SEPT POINT

### XXX SEPT POINT

## New way of passing parameters to modules within nf-test scripts

Our nf-core/modules have greatly benefited from the new nf-test implementation.
However one pet peeve for many is the greatly increased number of files that now comes with each module when installing on a pipeline (>100 file pipeline PRs anyone 😱).

Fortunately <Profile username="mahesh-panchal">Mahesh Binzer-Panchal</Profile> has made a [proof of concept](https://github.com/nf-core/modules/pull/5706) to help reduce this number.
sateeshperi marked this conversation as resolved.
Show resolved Hide resolved
By moving parameters into `when` blocks within the main nf-test script file, we can drop the test-only `nextflow.config` file that many modules have 🎉.
sateeshperi marked this conversation as resolved.
Show resolved Hide resolved

The proposal received unanimous approval.
Mahesh will now begin to formalise this by updating the [nf-core/modules specifications](https://nf-co.re/docs/guidelines/components/modules), and we can start to begin adding additional linting tests to help transfer to this system.

:::info
This system only applies to modules nf-test, and should not be used at pipeline level!
:::

## Stalled PRs due to waiting for review
sateeshperi marked this conversation as resolved.
Show resolved Hide resolved

It was raised that regularly get almost-complete module PRs, but sometimes they stall because a reviewer leaves comments but does not come back to re-review.
Even if the module author receives a second approving review, but the author often (because we are nice here at nf-core) doesn't feel comfortable merging without getting the approval from the original reviewer.
sateeshperi marked this conversation as resolved.
Show resolved Hide resolved

After a short discussion we felt that we can add a new reviewing guideline that if all (reasonable) comments have been addressed, and an approval has been given by another reviewer, a PR should be merged after 3 months if the original reviewer did not re-review.
sateeshperi marked this conversation as resolved.
Show resolved Hide resolved

If the original reviewer was temporarily unable to review during this period (which can happen!), the module can still be corrected in a follow up review.
sateeshperi marked this conversation as resolved.
Show resolved Hide resolved

## Replacing monolithic `conf/modules.conf` into per module-configs
sateeshperi marked this conversation as resolved.
Show resolved Hide resolved

A contentious point was brought up by the developers of [nf-core/rnaseq](https://nf-co.re/rnaseq) and [nf-core/methylseq](https://nf-co.re/methylseq).

Both pipelines recently have experimented with modifying the ways in which modules and processes can get customised by config files.
Currently, the nf-core template has a single file - `conf/modules.conf` - where all module-level configurations occur (specifying additional arguments, file publication specifications etc.).

The two pipelines have instead tried to split this single monolithic file into multiple, with one config file per module e.g. [in this nf-core/methylseq PR](https://github.com/nf-core/methylseq/commit/7b276c97589b65153ae23b34fcf4f6da86bb8d70).
sateeshperi marked this conversation as resolved.
Show resolved Hide resolved

One of the main aims of this is to make CI testing more efficient, by modifications to a config only triggering related tests, rather than the whole suite of tests (as all modules are reliant on the one config file).
Another reason is to make modules and configurations even more modular and thus portable.
sateeshperi marked this conversation as resolved.
Show resolved Hide resolved

This proposal initially brought up a lot of consternation with most of the other maintainers present.
The primary reason for this is the fear of findability by users, such as if they want to see which parameters of the tool is customised within the module.
Currently we can point them to a single file within a single directory, and they can search that.
By splitting into potentially hundreds of files, there was a fear it'll make it much harder for users to discover.
sateeshperi marked this conversation as resolved.
Show resolved Hide resolved
Furthermore, the system is not currently 'natively' supported by Nextflow (where modules can automatically detect and load `nextflow.config` files next to the `main.nf`), meaning a lot of manual work to manually `include` the config for each module.
sateeshperi marked this conversation as resolved.
Show resolved Hide resolved

A long debate occurred around this topic, ultimately between user readable vs developer efficiency and which should take more priority.
For example, maybe efficiency should be pushed to the nf-core/tools or GitHub CI configuration level where we can do some clever things to serve the same purpose.
Alternatively we can wait for Nextflow native support to make this less onerous to the developers.
sateeshperi marked this conversation as resolved.
Show resolved Hide resolved

sateeshperi marked this conversation as resolved.
Show resolved Hide resolved
A general agreement was that this new system is not yet palatable to enough maintainers to propose push to the template, however that nf-core/rnaseq and nf-core/methylseq can continue to refine the approach.

## Modification of modules specifications regarding channels

Finally, we debated pros- and cons- of different ways of structuring input channels in modules.

In the initial design of DSL2 nf-core modules, it was decided to require one input channel per file type, and not support 'mega-tuples', due to readability and portability.

For example, this was found to be less readable:

```nextflow
input:
tuple value(meta), path(bam), path(bai), path(fasta), path(fai), path(dict)
```

Compared to:

```nextflow
input:
tuple value(meta), path(bam)
tuple value(meta), path(bai)
tuple value(meta), path(fasta)
tuple value(meta), path(fai)
tuple value(meta), path(dict)
```

Where ensuring each file was associated with it's companion could be facilitated with (relatively) standardised `multiMap()` calls.
jfy133 marked this conversation as resolved.
Show resolved Hide resolved

By forcing a particular structure of a singular input tuple (former example), it restricted pipeline developers in how they can do their own file shuttling between processes, potentially requiring many custom `map` functions to restructure channels.

On the flipside, there are some instances (think `samtools`) where a particular file will _always_ be accompanied by a secondary file such as index files.
By combining these into a single tuple would make it easier to chain such modules with extremely uniform input file requirements.

Recently the module specifications had been relaxed to allow such cases of the former, but also in particular that different file format variants serving the same function (`.bai` vs `.csi` files for example), could be represented in the same element of a 'collapsed' input channel.

For example, an output channel could be like so:

```nextflow
output:
tuple value(meta), path("*bam"), path(".{bai,csi}"),
path(fasta)
path(fai)
path(dict)
```

Some maintainers did not like this, and rather wanted to go back to strict and always requiring each input file to be in it's own input channel.
Their reasoning was that it can be dangerous e.g. if a misconfiguration in a pipeline could result in the _wrong_ type of index file, and cause the pipeline to fail when a downstream process received the wrong type of index file.
sateeshperi marked this conversation as resolved.
Show resolved Hide resolved

We discussed wishes for 'typed' inputs (possibly coming natively to Nextflow in the future), versus code clarity (not constantly having to constantly `.join` and `.combine` after each module call).
Different philosophies came to the fore, whether trying to cover _all_ cases of errors of users is reasonable, versus pipelines only supporting specific configurations and parameters, and modifications away from this (e.g. via `ext.args`) being entirely at a users risk.
sateeshperi marked this conversation as resolved.
Show resolved Hide resolved

This discussion remains unresolved, and will likely rear it's head again in the future.

## The end

As always, if you want to get involved and give your input, join the discussion on relevant PRs and slack threads!

\- :heart: from your #maintainers team!
Loading