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

added dockerfile #177

Merged
merged 47 commits into from
May 10, 2024
Merged

added dockerfile #177

merged 47 commits into from
May 10, 2024

Conversation

thawn
Copy link
Contributor

@thawn thawn commented Apr 24, 2024

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

build a docker container that ships the latest pip-installable version
Copy link
Contributor Author

@thawn thawn left a comment

Choose a reason for hiding this comment

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

When we integrate this into the CI, we need to switch to installing rapids-singlecell from the tag created on github rather than installing the latest version from pypi.

This means we need to modify the environment yaml file accordingly within the CI job

pre-commit-ci bot and others added 19 commits April 24, 2024 16:03
that way, the final container will be much smaller and faster to build. Enabling rebuilding the container on every repository push. We could then also run the tests inside the container if we want.

The deps container only needs to be rebuilt if the dependencies changed. We should also rebuild the deps container for every release, so that the dependencies in the release container are up to date.
apt-get dist-upgrade -y fixes above mentioned errors during unit tests test_clustering.py and test_emdedding_density.py
cleaner build log
@thawn thawn marked this pull request as ready for review May 6, 2024 08:10
@thawn
Copy link
Contributor Author

thawn commented May 6, 2024

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:

  • merge this pull request and see if the CI works.
    * if it does not work, I guess someone with developer or higher status in the repo should open a new pull request. I am happy to contribute there.
  • create a scverse account on docker hub and add pushing to docker hub to the CI workflow as documented here by github and here by docker

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
Copy link
Contributor Author

thawn commented May 8, 2024

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
Copy link
Member

Intron7 commented May 8, 2024

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

@Intron7 Intron7 added the run-gpu-ci runs GPU CI label May 8, 2024
@flying-sheep
Copy link
Member

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

@thawn
Copy link
Contributor Author

thawn commented May 10, 2024

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
Copy link
Member

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

@Intron7
Copy link
Member

Intron7 commented May 10, 2024

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

@Intron7 Intron7 merged commit c3c8d9d into scverse:main May 10, 2024
7 checks passed
@thawn thawn deleted the docker branch May 10, 2024 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-gpu-ci runs GPU CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants