BioContainers / containers

Bioinformatics containers
http://biocontainers.pro
Apache License 2.0
674 stars 246 forks source link

Update Cellpose from 2.2.2 to 3.0.1 #559

Closed FloWuenne closed 6 months ago

FloWuenne commented 6 months ago

Submitting a Container

(If you're requesting for a new container, please check the procedure described here.

Check BioContainers' Dockerfile specifications

Checklist

  1. Misc

    • [ ] My tool doesn't exist in BioConda
    • [x] The image can be built
  2. Metadata

    • [x] LABEL base_image
    • [x] LABEL version
    • [x] LABEL software.version
    • [x] LABEL about.summary
    • [x] LABEL about.home
    • [x] LABEL about.license
    • [x] MAINTAINER
  3. Extra (optionals)

    • [ ] I have written tests in test-cmds.txt
    • [x] LABEL extra.identifier
    • [x] LABEL about.documentation
    • [x] LABEL about.license_file
    • [ ] LABEL about.tags

    Check BioContainers' Dockerfile metadata

biocontainers-bot commented 6 months ago

No test-cmds.txt (test file) present, skipping tests

FloWuenne commented 6 months ago

This PR adds the newest version of Cellpose, some of the changes:

Currently, there is a chmod 777 command for permissions to folder ./cellpose. If possible, would like to remove this, but I don't know how to specify this differently so that permissions for that folder for writing are still given.

mboudet commented 6 months ago

Hello! Thanks for the PR.

Not sure what do you mean regarding writing permissions. At which step are the permissions required? It would be great to merge the pip install commands into one RUN if possible (either directly, or by using a requirement file).

FloWuenne commented 6 months ago

Thanks for the comments @mboudet!

I was referring to this line: https://github.com/FloWuenne/containers/blob/12a2df77f99ec7e77cd6edb8fa0399f0d209d93a/cellpose/2.3.2/Dockerfile#L28

We were having issues with permissions when running cellpose from within the container when not making the ./cellpose directory and giving it 777 permissions. Generally its quite bad practice to use chmod 777 in containers and so I was wondering whether there is another way we could try to have this folder have the right permissions without explicitly calling chmod 777.

FloWuenne commented 6 months ago

@mboudet I have merged the pip commands into one Layer and have changed the permission by adding a user, giving that user permissions and running the container as that user. This was suggested by ChatGPT but I have seen this approach suggested in some forums too. Is this an acceptable approach?

biocontainers-bot commented 6 months ago

No test-cmds.txt (test file) present, skipping tests

biocontainers-bot commented 6 months ago

No test-cmds.txt (test file) present, skipping tests

mboudet commented 6 months ago

Thanks for the changes regarding pip!

I'm guessing the permission issue is for Singularity? (Given that the docker is run as root, it should not matter for a docker run) (Does it even works in Singularity? I though images where non-writable..)

FloWuenne commented 6 months ago

@mboudet Actually the permission issue is not for singularity but is affecting docker when using the container with Nextflow.

I was unable to make the container run in Nextflow with any setting beside forcing the user to be root in Nextflow. I don't quite understand why the permissions within nextflow need to be root for this specific container and the folders it creates when downloading models...

I will check out the new release 3.0.0 and if that fixes these issues we might just skip this one and go directly for 3.0.0!

mboudet commented 6 months ago

Oh, interesting. I don't usually run nextflow with docker containers (only singularity), so I'm not sure what is happening permission-wise. I see that cellpose needs to download some model files, so I guess if nextflow use another user than root it will not have permissions. You might try to mount the /.cellpose folder from outside the docker maybe? (Might be for the best anyway, that would avoid redownloading the files on each run).

In any case, I don't think the chmod 777 is an issue. You have it set to 755 currently though, so it will only work if it needs read permission (and not write).

Alright for the release 3.0.0, you can juste update this PR with the correct release when it's ready.

FloWuenne commented 6 months ago

@mboudet I have updated to version 3.0.1 after testing that it works with old trained models and within our nextflow setup.

I have also figured out the issue with permission. We needed to add a new variable: ENV CELLPOSE_LOCAL_MODELS_PATH=/tmp/cellpose_models

This moves the models that are downloaded out of $HOME and thus removes the permissions issues. This image should now be good for version 3.0.1!

biocontainers-bot commented 6 months ago

No test-cmds.txt (test file) present, skipping tests

mboudet commented 6 months ago

Perfect, thank you!