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

New module Ilastik/multicut #2894

Merged
merged 15 commits into from
Mar 27, 2023
Merged

Conversation

FloWuenne
Copy link
Contributor

PR checklist

Closes #2865
Uses the same tool (ilastik) as in #2875

  • 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 module conventions in the contribution docs
  • If necessary, include test data in your PR.
  • Remove all TODO statements.
  • Emit the versions.yml file.
  • Follow the naming conventions.
  • Follow the parameters requirements.
  • Follow the input/output options guidelines.
  • Add a resource label
  • Use BioConda and BioContainers if possible to fulfil software requirements.
  • Ensure that the test works with either Docker / Singularity. Conda CI tests can be quite flaky:
    • PROFILE=docker pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware
    • PROFILE=singularity pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware
    • PROFILE=conda pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware

Description of changes

This PR adds ilastik Multicut as a DSL2 module. The module takes as input an image in .h5 format, a multicut ilastik project and a probability map from a previously trained ilastik pixel classification. It outputs a multicut segmentation mask in .tiff format.

Things to do before merging

@FloWuenne FloWuenne changed the title Ilastik/multicut New module Ilastik/multicut Feb 19, 2023
modules/nf-core/ilastik/multicut/main.nf Outdated Show resolved Hide resolved
modules/nf-core/ilastik/multicut/main.nf Outdated Show resolved Hide resolved
modules/nf-core/ilastik/multicut/main.nf Show resolved Hide resolved
modules/nf-core/ilastik/multicut/main.nf Show resolved Hide resolved
modules/nf-core/ilastik/multicut/main.nf Show resolved Hide resolved
tests/modules/nf-core/ilastik/multicut/main.nf Outdated Show resolved Hide resolved
tag "$meta.id"
label 'process_low'

container "labsyspharm/mcmicro-ilastik:1.6.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be added to conda?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it's a bit tricky with ilastik. It is already on conda, but on it's own channel called ilastik-forge. There is also a biocontainer, but I don't see from where it is built : https://hub.docker.com/r/biocontainers/ilastik/tags. Additionally, the biocontainer is not the most recent release candidate. This is problematic, since MacOS users can't use the latest stable release to train models but can use the release candidate. That's why for now I put our mcmicro container, which we can update and uses the latest release.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow I've never seen a tool have it's own specific conda channel. Is it impossible to use this channel in Nextflow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually have no idea. @drpatelh is a custom conda channel like ilastik-forge useable in the conda definition of modules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am gonna check with the ilastik developers, whether they plan to have a maintained container on dockerhub that tracks the newest version. In the meantime, I think having the ilastik modules run off the latest mcmicro-ilastik container is the best way to go. That way we can update the container whenever we need to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Allright, can you also ask whether or not they would want to add their tool to biocontainers instead of that separate channel? I think it would be best for everyone (although I don't know what their reason is so we'll have to see)

@FloWuenne
Copy link
Contributor Author

Any idea why the singularity build still fails? Is it due to this part of the nextflow.config in test?

profiles {
    standard {
        docker {
            enabled = true
            userEmulation = false
        }
    }
}

I had to add userEmulation = false to the config, because otherwise the workflow gives errors about puid tracking within the container.

@FloWuenne
Copy link
Contributor Author

Any idea why the singularity build still fails? Is it due to this part of the nextflow.config in test?

profiles {
    standard {
        docker {
            enabled = true
            userEmulation = false
        }
    }
}

I had to add userEmulation = false to the config, because otherwise the workflow gives errors about puid tracking within the container.

It seems that having enabled = true for docker as the standard profile was the issue. I removed it and now the singularity container also builds correctly!

Copy link
Contributor

@nvnieuwk nvnieuwk left a comment

Choose a reason for hiding this comment

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

Can you also add a check for pipelines running with conda like this?

    // Exit if running this module with -profile conda / -profile mamba
    if (workflow.profile.tokenize(',').intersect(['conda', 'mamba']).size() >= 1) {
        exit 1, "ILASTIK_MULTICUT module does not support Conda. Please use Docker / Singularity / Podman instead."
    }

Otherwise LGTM

@FloWuenne
Copy link
Contributor Author

Before merging, the ilastik authors have just released a "stable" release version 1.4.0. We might wanna update the mcmicro ilastik container and this here to that version. I will look into that now!

@FloWuenne
Copy link
Contributor Author

Should be good to merge now!

@FloWuenne FloWuenne requested a review from maxulysse March 8, 2023 09:00
@maxulysse
Copy link
Member

I'm hoping I fixed the merge conflicts properly

@maxulysse
Copy link
Member

@nf-core-bot fix linting pretty please 🙏

@FloWuenne FloWuenne added this pull request to the merge queue Mar 27, 2023
Merged via the queue into nf-core:master with commit 4640422 Mar 27, 2023
FynnFreyer pushed a commit to FynnFreyer/modules that referenced this pull request Mar 28, 2023
* Current version files due to getpwuid()

* Functioning version. All tests pass.

* Removed To-Do from test.yml.

* Updated multicut tests to use test-datasets.

* Added meta2 and meta3 for input files.

* Removed docker enabled from nextflow.config

* Added exception exit for conda.

* Bumping ilastik version to 1.4.0.

* Fixed module author field.

* Updated container to biocontainer version 1.4.0

* Added conda test exception.

* [automated] Fix linting with Prettier

---------

Co-authored-by: Nicolas Vannieuwkerke <101190534+nvnieuwk@users.noreply.github.com>
Co-authored-by: Maxime U Garcia <max.u.garcia@gmail.com>
Co-authored-by: nf-core-bot <core@nf-co.re>
jvfe added a commit to jvfe/modules that referenced this pull request Mar 28, 2023
* master:
  Rename spatialomics to imaging in test_data.config (nf-core#3164)
  Freyja subworkflow with required modules (nf-core#3003)
  Version update for sourmash modules (nf-core#3159)
  update UMI subworkflow with duplex and best practices (nf-core#3104)
  Zip prodigal outputs (closes nf-core#922) (nf-core#3136)
  new module: pyrodigal (nf-core#3165)
  Group all mus_musculus together (nf-core#3170)
  PEDDY: added prefix to the name of outputs (nf-core#2984)
  Fix WFMASH error - missing variable (nf-core#3155)
  Updating endorspy (nf-core#3134)
  Limma: mandatory non-file args update, add subset options for differential analysis (nf-core#3116)
  sambamba flagstat module (nf-core#3133)
  Wisecondorx/convert (nf-core#3131)
  Metaphlan/makedb (nf-core#3138)
  New module Ilastik/multicut (nf-core#2894)
@FloWuenne FloWuenne deleted the ilastik/multicut branch March 31, 2023 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

new module: ilastik/multicut
4 participants