-
Notifications
You must be signed in to change notification settings - Fork 549
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
[Docker image] Nightly build of docker image #2229
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome, thanks @PratikKumar125! Trying it out now. Left some comments.
.github/workflows/nightly-build.yaml
Outdated
run: | | ||
docker build -t $DOCKER_USERNAME/skypilot:latest . | ||
docker login -u $DOCKER_USERNAME -p $DOCKER_PASSWORD | ||
docker push $DOCKER_USERNAME/skypilot:latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: new line at the end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for other file
.github/workflows/nightly-build.yaml
Outdated
@@ -0,0 +1,24 @@ | |||
name: Nightly Build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we call this docker-nightly-build
so we don't confuse this with pypi/other builds?
.github/workflows/release-build.yaml
Outdated
@@ -0,0 +1,40 @@ | |||
name: Release Build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we call this docker-release-build
?
.github/workflows/release-build.yaml
Outdated
on: | ||
push: | ||
branches: | ||
- 'releases/*' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use 'releases/**' to also catch any nested branches
.github/workflows/nightly-build.yaml
Outdated
@@ -0,0 +1,24 @@ | |||
name: Nightly Build | |||
|
|||
on: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add workflow_dispatch
to allow manual override when required?
.github/workflows/nightly-build.yaml
Outdated
build: | ||
runs-on: ubuntu-latest | ||
|
||
steps: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a check to run the nightly build only if there have been changes in the last 24 hours? You may want to refer to this.
.github/workflows/nightly-build.yaml
Outdated
env: | ||
DOCKER_USERNAME: ${{ secrets.DOCKER_USERNAME }} | ||
DOCKER_PASSWORD: ${{ secrets.DOCKER_PASSWORD }} | ||
run: | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before building the nightly, can we also update the version str like we're planning to do in #1446? See reference code here:
skypilot/.github/workflows/pypi-nightly-build.yml
Lines 40 to 45 in ee1e576
- name: Set release version | |
run: | | |
RELEASE_VERSION=$(date +%Y%m%d) | |
sed -i "s/{{SKYPILOT_COMMIT_SHA}}/${{ github.sha }}/g" sky/__init__.py | |
sed -i "s/__version__ = '.*'/__version__ = '1.0.0-dev${RELEASE_VERSION}'/g" sky/__init__.py | |
sed -i "s/name='skypilot',/name='skypilot-nightly',/g" sky/setup_files/setup.py |
For discussion: How about having the docker release workflow in the same workflow file as the pypi release so as to make sure the commit hash are the same for both nightly release? https://github.com/skypilot-org/skypilot/blob/master/.github/workflows/pypi-nightly-build.yml |
+1 for having the same version. However, it might be better to have them as two different workflows for isolation (in case something breaks in either of the flows).. How about we pick the head commit at 12AM PT for both workflows to ensure version is same? |
Another thing to think about and probably can be deferred to a future PR - can we have different images for different dependencies? E.g., This would help reduce bloat and pull times for users who use only a single cloud. |
Made some suggested changes. Require review. |
…ch 'master' of github.com:skypilot-org/skypilot into ci-cd/2193
Changed this to use the latest nightly build on pypi as a single source of truth. Tested in a fork and it works: https://github.com/michaelvll/skypilot/actions/workflows/docker-nightly-build.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Michaelvll!
apt install rsync vim -y && \ | ||
rm -rf /var/lib/apt/lists/* | ||
|
||
RUN pip install "skypilot-nightly[aws,gcp,azure,kubernetes]==$LATEST_VERSION" --no-cache-dir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth a quick check to measure the size difference between an image with skypilot-nightly[aws,gcp,azure,kubernetes]
vs skypilot-nightly[all]
. If not too big, perhaps we can ship with skypilot-nightly[all]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call! I was using the older python version and the installation of [all] seems taking forever, due to some dependency issue with the kubernetes extra for python 3.9. I just switched the base image to use python 3.10, and it seems working nicely with [all]
push: true | ||
tags: "${{ secrets.DOCKER_USERNAME }}/skypilot-nightly:latest,${{ secrets.DOCKER_USERNAME }}/skypilot-nightly:$LATEST_VERSION" | ||
cache-from: type=gha | ||
cache-to: type=gha,mode=max |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: since running a linux/amd64 image on apple silicon can be a bit slow because of emulation (e.g., on my M2 Mac, sky -v
takes 4 seconds on this image compared to 1 second running natively), consider adding builds for apple silicon.
IIUC from this guide, it should involve:
- Adding the qemu action above
- name: Set up QEMU
uses: docker/setup-qemu-action@v3
- In
docker/build-push-action@v5
, add:
platforms: linux/amd64,linux/arm64
Feel free to defer for a future PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call! I just tried adding it, but it seems the conda install -c conda-forge google-cloud-sdk
fail to be installed in the linux/arm64 version. Do you have an idea how to resolve it?
753f9fa
to
39bf5b4
Compare
Added two workflow files.
NOTE:-You need to store dockerhub username and password as github secrets with the same keys as follows :-