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

reduce config duplication, update third-party actions #101

Merged
merged 3 commits into from
Sep 9, 2024

Conversation

jameslamb
Copy link
Member

@jameslamb jameslamb commented Aug 29, 2024

Closes #100

Follow-up to #97

Contributes to rapidsai/build-planning#40

Proposes the following changes:

Notes for Reviewers

How do I test this?

I can't figure out where these image builds actually happen. At https://github.com/rapidsai/dask-build-environment/actions, I only see the "Check for gpuCI Updates" job running.

Could someone please tell me where docker build based on these Dockerfiles is run and how I could test these changes?

@jameslamb jameslamb requested a review from a team as a code owner August 29, 2024 21:05
@jameslamb jameslamb mentioned this pull request Aug 29, 2024
@jakirkham
Copy link
Member

Thanks James! 🙏

Generally this seems reasonable to me. Asking Charles to take a closer look as he is the expert here 😉

@jameslamb jameslamb changed the title add Python 3.12, reduce config duplication, update third-party actions reduce config duplication, update third-party actions Aug 30, 2024
@jameslamb
Copy link
Member Author

I just reverted the Python 3.12 part of this. Realized that some of these images are actually installing RAPIDS packages, and we don't yet have packages for Python 3.12.

e.g.:

name: rapids
channels:
- rapidsai
- rapidsai-nightly
- conda-forge
- nvidia
dependencies:
- cudatoolkit=CUDA_VER
- cudf=RAPIDS_VER
- dask-cudf=RAPIDS_VER
- cupy
- pynvml
- ucx-proc=*=gpu
- ucx-py=UCX_PY_VER

The work to add Python 3.12 here is tracked in rapidsai/build-planning#40

@charlesbluca
Copy link
Member

charlesbluca commented Sep 9, 2024

Thanks @jameslamb! A lot of long-overdue improvements here.

Could someone please tell me where docker build based on these Dockerfiles is run and how I could test these changes?

Yeah this is definitely something that could use more visibility - this is echoed as part of the build process, but this is only visible in the Jenkins runs, which I think are only visible to @rapidsai/dask-build-environment-write. Looking at one of these logs, the build command is:

docker build --pull -t gpuci/dask:24.10-cuda11.8.0-devel-ubuntu20.04-py3.10 --squash --build-arg RAPIDS_VER=24.10 --build-arg UCX_PY_VER=0.40 --build-arg CUDA_VER=11.8.0 --build-arg LINUX_VER=ubuntu20.04 --build-arg PYTHON_VER=3.10 /jenkins/workspace/dask/dask-build-environment/branch/dask-build-env-main/BUILD_NAME/dask/CUDA_VER/11.8.0/LINUX_VER/ubuntu20.04/PYTHON_VER/3.10/RAPIDS_VER/24.10/dask

Modifying this for local testing on this work (picking a tag arbitrarily):

docker build \
    --pull \
    -t gpuci/$PROJECT_NAME:$USER-dev \
    --squash \
    --build-arg RAPIDS_VER=24.10 \
    --build-arg UCX_PY_VER=0.40 \
    --build-arg CUDA_VER=11.8.0 \
    --build-arg LINUX_VER=ubuntu20.04 \
    --build-arg PYTHON_VER=3.10 \
    ./$PROJECT_NAME

Where PROJECT_NAME can be filled in with any of the subdirectories of this repo containing Dockerfiles.

@jameslamb
Copy link
Member Author

Ah got it, thanks for that!

Yep, can confirm that I'm not able to view those Jenkins logs.

  1. Could you tell me if it looks like these changes are passing CI?
  2. If they are, could you approve and merge this?
  3. Would you support folks from RAPIDS build-infra team being added with write access here, so we can help with these sorts of things like modifying the set of supported CUDA / Python verisons?

@charlesbluca
Copy link
Member

charlesbluca commented Sep 9, 2024

Could you tell me if it looks like these changes are passing CI?

Unfortunately, there isn't a PR builder job, so checking that these changes pass CI can't be done until this is merged in 😕 and typically I've relied on local testing for more extensive changes.

Locally, I can verify that not specifying all of the expected build args causes a failure:

docker build \
    --pull \
    -t gpuci/dask:charlesb-dev \
    ./dask
...
ERROR: failed to solve: rapidsai/miniforge-cuda:cudaunset-base-unset-pyunset: failed to resolve source metadata for docker.io/rapidsai/miniforge-cuda:cudaunset-base-unset-pyunset: docker.io/rapidsai/miniforge-cuda:cudaunset-base-unset-pyunset: not found

Then confirmed for each project that including those build arguments allowed the builds to proceed and pass.

If they are, could you approve and merge this?

I noticed that we missed this explicit setting of RAPIDS_VER in the dask-image dockerfile, which I think we can actually remove altogether since we don't actually install RAPIDS in that image - this would look like:

  • dropping the setting of RAPIDS_VER from this line setting the dask-image docker build args
  • dropping the line I linked to above

Would you mind applying this change?

@charlesbluca
Copy link
Member

Would you support folks from RAPIDS build-infra team being added with write access here, so we can help with these sorts of things like modifying the set of supported CUDA / Python verisons?

Followed up with an internal request to do this for now, if that spins off into a relevant GitHub issue can follow up here

@jameslamb
Copy link
Member Author

Unfortunately, there isn't a PR builder job, so checking that these changes pass CI can't be done until this is merged in 😕 and typically I've relied on local testing

Ohhh ok, thanks for that. I thought that maybe I just couldn't see the PR builds because I didn't have sufficient permissions.

Thanks for testing this one locally.

think we can actually remove altogether since we don't actually install RAPIDS in that image

Oh cool yeah, looks like that would be safe to do. I dropped RAPIDS_VER from dask_image in ddfd1e7

Copy link
Member

@charlesbluca charlesbluca left a comment

Choose a reason for hiding this comment

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

Thanks @jameslamb!

.github/workflows/update-gpuci.yml Show resolved Hide resolved
@charlesbluca charlesbluca merged commit 65c2149 into rapidsai:main Sep 9, 2024
@jameslamb jameslamb deleted the dockerfile-args branch September 10, 2024 22:03
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.

Build args in dockerfiles should not use realistic values
3 participants