Re-enable RayDP tests since arrow version limits removed #31124

kira-lin · 2022-12-15T01:20:32Z

Why are these changes needed?

Now that MLDataset has been made optional dependency, and PyArrow version limit has been removed, RayDP tests in ray can be re-enabled now.

Related issue number

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

kira-lin · 2022-12-15T03:20:16Z

It seems like java is not installed in the instances where arrow datasets are run. What should I do? @clarkzinzow @jjyao

Signed-off-by: Zhi Lin <[email protected]>

kira-lin · 2023-01-03T08:40:04Z

I found that the ci images is updated here #28641, it seems that java is not installed in ml images.
I tried to modify the Dockerfile to install java, but it did not work. What should I do? @krfricke

Signed-off-by: Zhi Lin <[email protected]>

nightly has been merged into raydp Signed-off-by: Zhi Lin <[email protected]>

krfricke

Thanks! I think there is a slight misunderstanding about the RAY_INSTALL_JAVA env variable which will build the Ray java language bindings, which we don't want in the base images (it will bloat the images and increase build time).

Instead we should install the java JDK in the 5 buildkite jobs that require java.

krfricke · 2023-02-15T22:22:59Z

ci/docker/base.ml.Dockerfile

@@ -1,6 +1,11 @@
 ARG DOCKER_IMAGE_BASE_TEST
 FROM $DOCKER_IMAGE_BASE_TEST

+ENV RAY_INSTALL_JAVA=1


Suggested change

ENV RAY_INSTALL_JAVA=1

ENV RAY_INSTALL_JAVA=1

Do we actually need the Ray Java language bindings here?

This will always build the Ray jars for every build job, this is probably not what we want

krfricke · 2023-02-15T22:25:49Z

ci/docker/ml.Dockerfile

@@ -13,6 +13,6 @@ WORKDIR /ray
 COPY . .

 # Install Ray
-RUN SKIP_BAZEL_BUILD=1 RAY_INSTALL_JAVA=0 bash --login -i -c -- "python3 -m pip install -e /ray/python/"
+RUN SKIP_BAZEL_BUILD=1 RAY_INSTALL_JAVA=1 bash --login -i -c -- "python3 -m pip install -e /ray/python/"


Suggested change

RUN SKIP_BAZEL_BUILD=1 RAY_INSTALL_JAVA=1 bash --login -i -c -- "python3 -m pip install -e /ray/python/"

RUN SKIP_BAZEL_BUILD=1 RAY_INSTALL_JAVA=0 bash --login -i -c -- "python3 -m pip install -e /ray/python/"

Let's revert this, as explained above we don't want to build the Ray java jars. This will drastically increase build time.

RayDP needs java binding to work though. It creates java actors. Where should we specify this option then?

krfricke · 2023-02-15T22:28:13Z

ci/docker/base.ml.Dockerfile

+RUN apt-get install -y -qq \
+    maven openjdk-8-jre openjdk-8-jdk


Instead of doing this in the base Dockerfile, can we just install the packages in the buildkite job that runs the RayDP test? I.e. move this to .buildkite/pipeline.ml.yml and then into the Dataset tests ... jobs.

To make this a bit nicer, maybe create a file ./ci/env/install-java.sh where we just run the apt install command and refer to this in the buildkite jobs

krfricke · 2023-02-15T22:28:49Z

ci/env/install-dependencies.sh

@@ -54,7 +54,7 @@ install_base() {
      curl -f -s -L -R https://bazel.build/bazel-release.pub.gpg | sudo apt-key add - || true
      sudo apt-get update -qq
      pkg_install_helper build-essential curl unzip libunwind-dev python3-pip python3-setuptools \
-        tmux gdb
+        tmux gdb maven openjdk-8-jre openjdk-8-jdk


Let's remove this once we install java in the buildkite jobs that require it

Signed-off-by: Zhi Lin <[email protected]>

kira-lin · 2023-02-17T01:07:56Z

@krfricke tests has passed, but when I try to add a ci/env/install_java.sh, tests failed due to permission denied. How to solve it?

krfricke · 2023-02-17T01:30:22Z

@krfricke tests has passed, but when I try to add a ci/env/install_java.sh, tests failed due to permission denied. How to solve it?

Did you enable the execution flag with chmod +x install_java.sh on your local machine?

kira-lin · 2023-02-17T05:11:47Z

Did you enable the execution flag with chmod +x install_java.sh on your local machine?
@krfricke thanks, the issue has been solved. Now the tests are ok, it's ready to merge

krfricke

Thank you very much! Almost there, I have just one last question about building the Ray java jars which I think are not needed. If they are, can you point me to where they are used for reference?

krfricke · 2023-02-17T05:57:45Z

ci/env/install-java.sh

+WORKSPACE_DIR="${SCRIPT_DIR}/../.."
+
+sudo apt-get install -y maven openjdk-8-jre openjdk-8-jdk
+"${WORKSPACE_DIR}"/java/build-jar-multiplatform.sh linux


Do we need to build the Ray java jars? We don't run any java code, right?

Raydp create java actors. Such as here

krfricke · 2023-02-17T05:59:47Z

Also, you are building on a pretty old base master branch. Could you please merge the latest upstream master into this PR?

krfricke

Thanks for the context! Will merge once CI passes

Signed-off-by: Zhi Lin <[email protected]>

…raydp-test

…ay-project#31124) Now that MLDataset has been made optional dependency, and PyArrow version limit has been removed, RayDP tests in ray can be re-enabled now. Signed-off-by: Zhi Lin <[email protected]> Signed-off-by: Edward Oakes <[email protected]>

…ay-project#31124) Now that MLDataset has been made optional dependency, and PyArrow version limit has been removed, RayDP tests in ray can be re-enabled now. Signed-off-by: Zhi Lin <[email protected]>

…ay-project#31124) Now that MLDataset has been made optional dependency, and PyArrow version limit has been removed, RayDP tests in ray can be re-enabled now. Signed-off-by: Zhi Lin <[email protected]> Signed-off-by: elliottower <[email protected]>

kira-lin requested review from ericl, scv119, clarkzinzow, jjyao, jianoaix and c21 as code owners December 15, 2022 01:20

uncomment raydp test

76ae690

Signed-off-by: Zhi Lin <[email protected]>

kira-lin added 2 commits January 3, 2023 14:33

install java in ml docker image

a8b18ac

Signed-off-by: Zhi Lin <[email protected]>

install java in script

47b6255

Signed-off-by: Zhi Lin <[email protected]>

kira-lin changed the title ~~Uncomment RayDP tests since arrow version limits removed~~ Re-enable RayDP tests since arrow version limits removed Feb 9, 2023

install raydp nightly

ce25890

nightly has been merged into raydp Signed-off-by: Zhi Lin <[email protected]>

krfricke requested changes Feb 15, 2023

View reviewed changes

krfricke self-assigned this Feb 15, 2023

krfricke added the @external-author-action-required Alternate tag for PRs where the author doesn't have labeling permission. label Feb 16, 2023

kira-lin added 3 commits February 16, 2023 16:02

address comments

b1e0685

Signed-off-by: Zhi Lin <[email protected]>

test build jar

4ba2d2a

Signed-off-by: Zhi Lin <[email protected]>

fix test

79cb22b

Signed-off-by: Zhi Lin <[email protected]>

krfricke reviewed Feb 17, 2023

View reviewed changes

krfricke approved these changes Feb 17, 2023

View reviewed changes

krfricke removed the @external-author-action-required Alternate tag for PRs where the author doesn't have labeling permission. label Feb 17, 2023

kira-lin added 4 commits February 17, 2023 08:42

fix

322eb0d

Signed-off-by: Zhi Lin <[email protected]>

use install-java.sh

e28cfd6

Signed-off-by: Zhi Lin <[email protected]>

revert change

31ace16

Signed-off-by: Zhi Lin <[email protected]>

Merge branch 'master' of https://github.com/ray-project/ray into fix-…

1f8832c

…raydp-test

c21 approved these changes Feb 17, 2023

View reviewed changes

clarkzinzow approved these changes Feb 17, 2023

View reviewed changes

clarkzinzow merged commit c7a50b4 into ray-project:master Feb 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-enable RayDP tests since arrow version limits removed #31124

Re-enable RayDP tests since arrow version limits removed #31124

kira-lin commented Dec 15, 2022

kira-lin commented Dec 15, 2022

kira-lin commented Jan 3, 2023

krfricke left a comment

krfricke Feb 15, 2023

krfricke Feb 15, 2023

kira-lin Feb 16, 2023

krfricke Feb 15, 2023

krfricke Feb 15, 2023

kira-lin commented Feb 17, 2023

krfricke commented Feb 17, 2023

kira-lin commented Feb 17, 2023

krfricke left a comment

krfricke Feb 17, 2023

kira-lin Feb 17, 2023

krfricke commented Feb 17, 2023

krfricke left a comment

	RUN SKIP_BAZEL_BUILD=1 RAY_INSTALL_JAVA=1 bash --login -i -c -- "python3 -m pip install -e /ray/python/"
	RUN SKIP_BAZEL_BUILD=1 RAY_INSTALL_JAVA=0 bash --login -i -c -- "python3 -m pip install -e /ray/python/"

		RUN apt-get install -y -qq \
		maven openjdk-8-jre openjdk-8-jdk

Re-enable RayDP tests since arrow version limits removed #31124

Re-enable RayDP tests since arrow version limits removed #31124

Conversation

kira-lin commented Dec 15, 2022

Why are these changes needed?

Related issue number

Checks

kira-lin commented Dec 15, 2022

kira-lin commented Jan 3, 2023

krfricke left a comment

Choose a reason for hiding this comment

krfricke Feb 15, 2023

Choose a reason for hiding this comment

krfricke Feb 15, 2023

Choose a reason for hiding this comment

kira-lin Feb 16, 2023

Choose a reason for hiding this comment

krfricke Feb 15, 2023

Choose a reason for hiding this comment

krfricke Feb 15, 2023

Choose a reason for hiding this comment

kira-lin commented Feb 17, 2023

krfricke commented Feb 17, 2023

kira-lin commented Feb 17, 2023

krfricke left a comment

Choose a reason for hiding this comment

krfricke Feb 17, 2023

Choose a reason for hiding this comment

kira-lin Feb 17, 2023

Choose a reason for hiding this comment

krfricke commented Feb 17, 2023

krfricke left a comment

Choose a reason for hiding this comment