Skip to content

Conversation

mashehu
Copy link
Collaborator

@mashehu mashehu commented Sep 30, 2025

Do not merge! This is a PR of dev compared to the TEMPLATE branch for whole-pipeline reviewing purposes. Changes should be made to dev and this PR should not be merged! The actual release PR is at

Copy link

github-actions bot commented Sep 30, 2025

nf-core pipelines lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 3e39d97

+| ✅ 191 tests passed       |+
#| ❔  12 tests were ignored |#
!| ❗   7 tests had warnings |!

❗ Test warnings:

  • nextflow_config - Config manifest.version should end in dev: 1.0.0
  • readme - README contains the placeholder zenodo.XXXXXXX. This should be replaced with the zenodo doi (after the first release).
  • pipeline_todos - TODO string in README.md: If applicable, make list of people who have also contributed
  • pipeline_todos - TODO string in README.md: Add citation for pipeline after first release. Uncomment lines below and update Zenodo doi and badge at the top of this file.
  • pipeline_todos - TODO string in awsfulltest.yml: You can customise AWS full pipeline tests as required
  • local_component_structure - utils.nf in modules/local should be moved to a TOOL/SUBTOOL/main.nf structure
  • local_component_structure - spaceranger.nf in subworkflows/local should be moved to a SUBWORKFLOW_NAME/main.nf structure

❔ Tests ignored:

  • files_exist - File is ignored: conf/igenomes.config
  • files_exist - File is ignored: conf/igenomes_ignored.config
  • files_exist - File is ignored: assets/multiqc_config.yml
  • files_exist - File is ignored: conf/igenomes.config
  • files_exist - File is ignored: conf/igenomes_ignored.config
  • files_exist - File is ignored: assets/multiqc_config.yml
  • files_unchanged - File ignored due to lint config: .github/CONTRIBUTING.md
  • files_unchanged - File ignored due to lint config: assets/sendmail_template.txt
  • files_unchanged - File ignored due to lint config: assets/nf-core-sopa_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-sopa_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-sopa_logo_dark.png
  • multiqc_config - multiqc_config

✅ Tests passed:

Run details

  • nf-core/tools version 3.3.2
  • Run at 2025-10-02 15:46:45

Copy link
Collaborator Author

@mashehu mashehu left a comment

Choose a reason for hiding this comment

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

first part of my review

@quentinblampey
Copy link
Collaborator

Thanks @mashehu for this first review, I updated it accordingly

Just one question regarding your other comment on the original PR:

When doing the change you suggested about the containerOption, I now have this CI error again. It's because for cellpose (one specific segmentation tool, not always used) I need to have permission for its cache directory. I followed this nf-core docs to fix this (see cellpose conf here)

Do you have a solution for it?

Copy link
Collaborator Author

@mashehu mashehu left a comment

Choose a reason for hiding this comment

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

went through the rest.

  • modules need to be split
  • more documentation needs to be moved over to nf-core
  • nf-core pipelines lint error need to be fixed
  • nextflow_schema.json is not valid (please use nf-core schema build to edit it in the future)


## Sopa parameters

You'll also need to choose some Sopa parameters that you'll provide to Nextflow via the `-params-file` option. You can find existing Sopa parameter files [here](https://github.com/gustaveroussy/sopa/tree/main/workflow/config), and follow the [corresponding README instructions](https://github.com/gustaveroussy/sopa/blob/main/workflow/config/README.md) of to get your `-params-file` argument.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sorry, but we having parameter files linked in an external repo is not really the nf-core way. we prefer to have everything nicely bundled together to avoid version mismatches and keep things reusable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's possible to provide all the params directly in the command line, but since there are many parameters I just recommend some "presets" to get started, but it's not mandatory to use these
I think the main problem comes from the parameters nested hierarchy, which doesn't show up on the instruction section of sopa on the nf-core website, although they are documented on the nextflow schema
What's your suggestion?

Comment on lines +240 to +241
def TRANSCRIPT_BASED_METHODS = ['proseg', 'baysor', 'comseg']
def STAINING_BASED_METHODS = ['stardist', 'cellpose']
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if you put this as enums in your nextflow_schema.json you can skip this

Copy link
Collaborator

Choose a reason for hiding this comment

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

I already list the possible options in the nextflow_schema, but here I want to check more complex logic, like here
I don't know how to do such check with a schema, do you know if it's possible?

assert params.read instanceof Map && params.read.containsKey('technology') : "Provide a 'read.technology' key"
assert params.containsKey('segmentation') : "Provide a 'segmentation' section"

// backward compatibility
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

do you need this for a 1.0.0 release?

anyway you can also do deprecation with the nextflow_schema.json

Copy link
Collaborator

Choose a reason for hiding this comment

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

It can be useful for Sopa users who were using Snakemake and want to move to nf-core/sopa
They could keep the same parameters and it should work, even though their parameters are one year old

Concerning the schema: is it also possible to handle the deprecation logic as in here? Again, I don't know how to update other parameters of the config directly based on the schema

@quentinblampey
Copy link
Collaborator

Thanks @mashehu for the second round of review!

I split the modules as requested and started adding more docs, but I still have a few questions (see my answer to your review above)

Also, when running nf-core pipelines lint, I don’t see any error - I’m using nf-core version 3.3.2. Which error do you see?

Regarding the schema, what’s the right command line to check that it’s valid? I tried nf-core pipelines schema lint and also received a “schema looks valid” result...

@mashehu
Copy link
Collaborator Author

mashehu commented Oct 6, 2025

sorry, looks like I linted the wrong version, all good there now concerning linting errors 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants