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

feat: simplify unify containers #959

Open
wants to merge 147 commits into
base: master
Choose a base branch
from
Open

Conversation

daler
Copy link
Member

@daler daler commented Feb 27, 2024

This takes the ideas from the code review on #953 and simplifies by refactoring how everything is configured. See #953 (comment) for the specific proposed items, which are addressed here.

The only major difference from those proposed items is storing the conda/mamba versions, which are not clearly defined in requirements. For example, I think mamba comes in from the base image (quay.io/condaforge/linux-anvil-cos7-x86_64). So I kept the same strategy as before, where the create-env depends on the build-env and inspects the conda/mamba versions used there.

Edits 2024-08-10

Here's an overview of the changes...

This is strictly for the base images. They are used for creating images for packages, and once this is merged they will will be pushed to the same quay.io tags as existing base images. But this PR does not deal with creating package images.

(if the nomenclature of images is confusing, see dockerfile inventory for a guide)

The package building has been unified to use a single build.sh script which takes an image directory as its only argument. That image directory should have prepare.sh, Dockerfile, and Dockerfile.test. Each <IMAGE DIR>/prepare.sh sources versions.sh which is the common place to set versions and tags. It also provides some helper functions. You can read about all of this in images/README.md.

The new build-images.yml GitHub workflow handles the interdependencies of the image building and testing. It uses GitHub Container Registry as a temporary storage space for images. After testing, tagged images are pushed to quay.io.

The main advantage of this overhaul is to factor out all the commonalities across building images, which otherwise have grown in different directions over the years. This provides just one unified system that maintainers need to understand. The reason it is in this repo rather than bioconda-containers is that we have strong dependencies on the version of bioconda-utils, so ideally changes to bioconda-utils should also test that corresponding base images work correctly.

NOTE: this does NOT yet replace the bioconda bot image so bioconda-containers is still needed for that.

@daler daler mentioned this pull request Aug 10, 2024
@daler
Copy link
Member Author

daler commented Aug 11, 2024

Some notes on the current status of this:

I would like to be able to test the just-built images (in the case that they were rebuilt in a PR) but also test the existing images on quay.io (in the case that only bioconda-utils has been changed).

This requires a clean way of specifying which images to use, but this is currently complicated. So the next stage here is cleaning up how images are specified throughout the code base. To avoid needing to simultaneously update mulled-build source, the strategy here should be to match what mulled-build looks for and uses.

Specifically in these lines of mulled_build.py, we have:

  • DEFAULT_BASE_IMAGE which is the busybox base image
  • DEFAULT_EXTENDED_BASE_IMAGE which is the debian base image
  • DEST_BASE_IMAGE, also the busybox base image
  • CONDA_IMAGE which is the "create-env" image

@daler
Copy link
Member Author

daler commented Aug 11, 2024

I noticed that the build-env container is installing a version of conda that is not specified in bioconda-utils requirements. Can't quite figure out why. Despite requirements saying conda=24.5.*", it's installing 23.7`...and it's taking almost an hour. Going to revert f311612 which removed mamba...

need to be able to override common.sh so that bioconda-utils in host
matches that in just-built container
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.

1 participant