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

[ci] migrate remaining non-gpu ml tests to civ2 #40527

Merged
merged 1 commit into from
Oct 24, 2023
Merged

Conversation

can-anyscale
Copy link
Collaborator

@can-anyscale can-anyscale commented Oct 20, 2023

  • Install hdfs into the ml.build image as well, so we can reuse the same images for a lot of test settings
  • Use bash as a default shell for these test images

Test:

  • CI

@can-anyscale can-anyscale force-pushed the can-tail-04 branch 5 times, most recently from a75ab7f to 208522a Compare October 20, 2023 19:41
./ci/env/install-dependencies.sh
INSTALL_HDFS=1 ./ci/env/install-dependencies.sh
Copy link
Collaborator Author

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 line here

@@ -8,6 +8,8 @@ ARG BUILD_TYPE
ENV CC=clang
ENV CXX=clang++-12

SHELL ["/bin/bash", "-ice"]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is required to use oss-ci-base_build for testing

Copy link
Collaborator

Choose a reason for hiding this comment

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

why? what is the error?

rayci already sets shell to -elic for the docker plugin?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah -elic is only provided during docker run; this is docker build; the oss_ci_base_build is already have pip installed (through ci/env/install-dependencies.sh i assume), but without this the docker build process will say pip not found (assume pip is loaded through .bashrc)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

in the build, it has !/bin/bash shebang. I think the only thing you want is -i, which adds anaconda's pip into PATH, you do not want to enable -c and -e by default though. that would be quite surprising result for users.

it might be easier and cleaner to add $HOME/anaconda3/bin/ into PATH (export PATH="$HOME/anaconda3/bin:$PATH" at the beginning of the script.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Keeping only -i sounds good, I want to load the .bashrc instead of only getting pip, there are quite a few places setting things up in .bashrc (https://github.com/ray-project/ray/blob/master/.buildkite/pipeline.ml.yml#L367).

A few things at put in root/.bashrc, others are in ~/.bashrc, so -i will be a safer option to set all things up.

@can-anyscale can-anyscale changed the title [ci] migrate ray air hdfs tests [ci] migrate remaining non-gpu ml tests to civ2 Oct 20, 2023
@can-anyscale can-anyscale marked this pull request as ready for review October 20, 2023 23:12
- bazel run //ci/ray_ci:test_in_docker -- //python/ray/tune/... ml
--only-tags soft_imports
--build-name oss-ci-base_build
depends_on: oss-ci-base_build
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in this image, the default shell is sh not bash

@can-anyscale can-anyscale force-pushed the can-tail-04 branch 3 times, most recently from bc561c7 to cb59369 Compare October 23, 2023 19:43
@can-anyscale
Copy link
Collaborator Author

@aslonnie I still need to use at least bash -ic for the shell to loads .bash_rc; if there is a better way to do this let me know

@can-anyscale can-anyscale force-pushed the can-tail-04 branch 2 times, most recently from 25559c9 to fa7d9de Compare October 23, 2023 21:46
@can-anyscale can-anyscale merged commit 4e9a0f3 into master Oct 24, 2023
21 of 45 checks passed
@can-anyscale can-anyscale deleted the can-tail-04 branch October 24, 2023 02:50
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.

None yet

2 participants