-
Notifications
You must be signed in to change notification settings - Fork 402
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
[CI] Refactor pipeline and test RayCluster sample yamls #1321
[CI] Refactor pipeline and test RayCluster sample yamls #1321
Conversation
Signed-off-by: Archit Kulkarni <[email protected]>
Signed-off-by: Archit Kulkarni <[email protected]>
Signed-off-by: Archit Kulkarni <[email protected]>
Signed-off-by: Archit Kulkarni <[email protected]>
Signed-off-by: Archit Kulkarni <[email protected]>
Signed-off-by: Archit Kulkarni <[email protected]>
# Install requirements | ||
pip install -r tests/framework/config/requirements.txt | ||
|
||
# Bypass Git's ownership check due to unconventional user IDs in Docker containers |
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 the only new part of the env setup that was needed for the RayCluster test to work. It's required because the RayCluster test uses some git
commands, but the owner of workdir
was "2000
" which git deems suspicious and throws an error:
stderr: 'fatal: detected dubious ownership in repository at '/workdir'
I assume this is somehow related to docker and that it's safe to ignore this.
.buildkite/setup-env.sh
Outdated
install -o root -g root -m 0755 kubectl /usr/local/bin/kubectl | ||
|
||
# Create the cluster | ||
time kind create cluster --wait 120s --config tests/framework/config/kind-config-buildkite.yml |
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.
Why do we need to create a Kind cluster (L24 - L56) here without running any tests?
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.
We don't need it anymore, this is just from the initial test. I have a followup PR which will remove it
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.
I just removed it in this PR.
.buildkite/setup-env.sh
Outdated
|
||
# Install python 3.10 and pip | ||
apt-get update | ||
apt-get install -y python3.10 python3-pip |
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.
We may update this based on #1325.
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 eye! If your PR gets merged first I'll fix the merge conflict in this 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.
#1325 has already been merged.
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.
O you're right
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.
Fixed
Signed-off-by: Archit Kulkarni <[email protected]>
…move-raycluster-sample-yaml-test Signed-off-by: Archit Kulkarni <[email protected]>
Signed-off-by: Archit Kulkarni <[email protected]>
…move-raycluster-sample-yaml-test
Signed-off-by: Archit Kulkarni <[email protected]>
Signed-off-by: Archit Kulkarni <[email protected]>
Signed-off-by: Archit Kulkarni <[email protected]>
Signed-off-by: Archit Kulkarni <[email protected]>
@kevin85421 all comments are addressed and I expect tests to pass, can you take another look? |
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.
Great! This is super helpful!
…1321) Refactor pipeline and test RayCluster sample yamls
…1321) Refactor pipeline and test RayCluster sample yamls
Why are these changes needed?
Moves the sample YAML tests from github actions to Buildkite.
This PR now tests 10 sample YAML files, instead of only three as before. We omit the TPU sample YAML because we don't have TPUs on buildkite.
Related issue number
Checks