scverse / rapids_singlecell

Rapids_singlecell: A GPU-accelerated tool for scRNA analysis. Offers seamless scverse compatibility for efficient single-cell data processing and analysis.
https://rapids-singlecell.readthedocs.io/
MIT License
140 stars 20 forks source link

added dockerfile #177

Closed thawn closed 4 months ago

thawn commented 5 months ago

build a docker container that ships the latest pip-installable version

thawn commented 5 months ago

The docker rapids-singlecell-deps and rapids-singlecell docker images work for me (you can get them from the docker hub and test if they work for you) They also work via singularity and all tests succeeded on our cluster. I also tried apptainer v1.2.3 but that did not work on our cluster (not sure if that is a problem of apptainer itself or specific to the installation on our cluster)

This is as far as I can get with the CI. The github actions seem to do what they should, however they fail to push to the github container registry with a permission error (I guess because I am not allowed to push to this repository's container registry)

I guess the next steps would be:

In principle, publishing to docker hub is optional, since the github container registry should work as well (just need to add ghcr.io/ when pulling the docker container). However, people will look at docker hub first. Also, the docker hub account is free. So I would recommend also publishing to docker hub.

thawn commented 4 months ago

o.k. I got the docker pipeline to work for pull requests.

I changed the workflow so that the built images are not uploaded to the container registry for pull requests. I think this is sensible and it avoids permission problems.

For releases, the images should be uploaded to the github container registry (ghcr.io). This should not cause permission problems, since releases are triggered by someone with write permission to the repo. However, I have no way of testing this (but I will be happy to help with debugging in case of problems).

No Idea why the GPU-CI fails, but that should be unrelated to this PR.

Intron7 commented 4 months ago

The CI is failing because its not running. I need to set a label for it to run.

flying-sheep commented 4 months ago

Now we’re no longer testing if pushing works right?

thawn commented 4 months ago

Yes, you are right. This is as far as I can get with the permissions I have.

Pushing to the github container registry will work using the GITHUB_TOKEN, if the token permissions are set to permissive and if the action was not triggered by a public fork. Since this is a pull request from a public fork, we will not be able to test pushing to the github container registry from this pull request. This is the reason, why I don't think it makes a lot of sense to push to the container registry from pull requests. Testing pushing to the registry is only really necessary when you want to test/debug the github action (like we are at the moment).

I recommend the following approach in order to test pushing to the container registry:

  1. merge this pull request
  2. create a new branch test-container-push from within this repo, not from a public fork
  3. temporarily activate pushing to the registry form pull requests by changing docker.yml as follows:
    • comment out all if: github.event_name == 'release' lines
    • replace all occurrences of push: ${{ github.event_name == 'release' }} with push: true
  4. make sure that the GITHUB_TOKEN permissions are set to permissive
  5. create a pull request from the test-container-push branch - since this is an internal branch, the GITHUB_TOKEN should have write access to the github container registry
  6. pushing to the github container registry should work. If it doesn't, fix the problems within the pull request.
  7. reverse the changes made in 3.
  8. create a release which should trigger building and pushing a container to the github registry

An alternative would be to set up a token specifically for access to the container registry. But in that case, I would recommend to push to the docker registry instead, since the needed steps are very similar and then you have the image on docker hub, where more people will be able to find it.

flying-sheep commented 4 months ago

I see now! Yeah, that makes sense, what do you think @Intron7?

Intron7 commented 4 months ago

I think this is a great idea. I can merge now if you are ok with it @flying-sheep