[ci] First release test on GKE by khluu · Pull Request #53390 · ray-project/ray

khluu · 2025-05-29T00:26:19Z

No description provided.

Signed-off-by: kevin <kevin@anyscale.com>

aslonnie · 2025-05-29T20:38:13Z

release/ray_release/configs/global_config.py

    os.environ[
        "GOOGLE_APPLICATION_CREDENTIALS"
-    ] = f"/workdir/{config['aws2gce_credentials']}"
+    ] = f"/workdir/{config['aws2gke_credentials']}"


this should not be hard coded here?

fixed to use GCE credentials instead so no changes needed here

aslonnie · 2025-05-30T09:20:08Z

release/ray_release/glue.py

    # non critical for some tests. So separate it from the general one.
    fetch_result_exception = None
+
+    if test.get_name().endswith(".kuberay"):


why kuberay tests need to be handled differently?

I thought when the right methods are implemented, we should be able to reuse most of the code in this function unchanged?

I remember we talked about this before.. I intended to do that at the beginning but running release tests with Kuberay doesn't require all the steps that we have for anyscale (like submitting a compute config beforehand) so I don't feel it's necessary to implement all the steps. This one does reuse a lot of existing functions though

hmm.. ok, then could you implement another function with clearer input args, and intercept/branch out earlier?

there a set of vars that are initialized before this, and they are mostly not used.

essentially:

def _run_release_test_anyscale(...): # this is renamed from the existing function ... def _run_release_test_kuberay(...): # this is your new function ... def run_release_test(...): if test.get_name().endswith(".kuberay"): return _run_release_test_kuberay(...) # only send in required args return run_release_test_anyscale(...)

and also, this should use test.get_env() == "kuberay", right? rather than checking based on name's suffix?

aslonnie · 2025-05-30T09:23:18Z

release/ray_release/job_manager/kuberay_job_manager.py

+            for job in response_json["jobs"]:
+                print(job)
+                print()
+            raise Exception(f"Multiple jobs found for {self.job_name}")


this can happen when the job launching gets error in some way and got retried, right?

I think the service only returns one job.. i can check more

release/ray_release/job_manager/kuberay_job_manager.py

aslonnie · 2025-05-30T09:27:38Z

release/ray_release/util.py

+    import tempfile
+    import time


import on file top?

Signed-off-by: kevin <kevin@anyscale.com>

aslonnie · 2025-05-31T04:26:51Z

release/ray_release/glue.py

    # non critical for some tests. So separate it from the general one.
    fetch_result_exception = None
+
+    if test.get_name().endswith(".kuberay"):


hmm.. ok, then could you implement another function with clearer input args, and intercept/branch out earlier?

there a set of vars that are initialized before this, and they are mostly not used.

essentially:

def _run_release_test_anyscale(...): # this is renamed from the existing function ... def _run_release_test_kuberay(...): # this is your new function ... def run_release_test(...): if test.get_name().endswith(".kuberay"): return _run_release_test_kuberay(...) # only send in required args return run_release_test_anyscale(...)

aslonnie · 2025-05-31T04:39:23Z

release/ray_release/glue.py

+    # if no_terminate and run_type == "anyscale_job":
+    #     logger.warning(
+    #         "anyscale_job run type does not support --no-terminate. "
+    #         "Switching to job (Ray Job) run type."
+    #     )
+    #     run_type = "job"


just remove these code? maybe even in a separate PR?

aslonnie · 2025-05-31T04:40:09Z

release/ray_release/job_manager/kuberay_job_manager.py

+            response = requests.post(url, json=request, headers=headers)
+            print(f"Response: {response}")
+            response.raise_for_status()


have a helper class / function or something for sending request?

do we need a helper if it's just 2 lines?

aslonnie · 2025-05-31T04:40:30Z

release/ray_release/job_manager/kuberay_job_manager.py

+            response = requests.post(url, json=request, headers=headers)
+            print(f"Response: {response}")
+            response.raise_for_status()
+        except Exception as e:


catch a more specific exception type, rather than generic Exception ?

(kind reminder)

aslonnie · 2025-05-31T04:41:19Z

release/ray_release/job_manager/kuberay_job_manager.py

+
+    def _wait_job(self, timeout: int = 7200) -> Tuple[int, float]:
+        start_time = time.time()
+        next_status = start_time + 10


rename to next_status_timestamp? next_status feels like a status.

aslonnie · 2025-05-31T04:48:14Z

release/ray_release/scripts/run_release_test.py

+    #     )

-    maybe_fetch_api_token()
+    # maybe_fetch_api_token()


why is this commented out?

this will break existing release test, right?

aslonnie · 2025-05-31T04:49:15Z

release/ray_release/glue.py

    # non critical for some tests. So separate it from the general one.
    fetch_result_exception = None
+
+    if test.get_name().endswith(".kuberay"):


and also, this should use test.get_env() == "kuberay", right? rather than checking based on name's suffix?

aslonnie · 2025-05-31T04:51:00Z

release/ray_release/util.py

    return joined_path
+
+
+def convert_cluster_compute_to_kuberay_compute_config(compute_config: dict) -> dict:


could you separate this function into another PR, and add some unit tests?

also, could you move this function into a new file? maybe kuberay_util.py or something? rather than generic util.py?

done here #53681

so this function here can be removed now? since the other PR is merged?

aslonnie · 2025-05-31T04:52:26Z

release/requirements_buildkite.txt

+    --hash=sha256:11405fed67b68f406b3f3c7c5ae5104a79d2d309666d10d61b152e91d28fb968 \
+    --hash=sha256:843934ef8c09e3e858952887467f8256aac3910c55f077a359a65b2b3cde3e68
+    # via google-api-core
+h11==0.14.0 \


there is already an h11==0.16.0 above, how is this getting here? seems to be an incorrect merge?

aslonnie · 2025-05-31T04:52:56Z

release/ray_release/util.py

+    return config
+
+
+def upload_working_dir(working_dir: str) -> str:


we should probably consider also using this for anyscale, so that the working dir is unified.

Signed-off-by: kevin <kevin@anyscale.com>

aslonnie · 2025-06-10T23:09:00Z

release/ray_release/environments/kuberay.env

@@ -0,0 +1 @@
+GOOGLE_CLOUD_PROJECT="dhyey-dev"


can we remove the mentioning of @dayshah 's name here?

aslonnie · 2025-06-10T23:09:35Z

release/ray_release/glue.py

+            smoke_test=smoke_test,
+            test_definition_root=test_definition_root,
+        )
+    else:


nit: drop the else, and save an indentation?

aslonnie · 2025-06-10T23:09:51Z

release/ray_release/glue.py

+        )
+
+
+def run_release_test_kuberay(


as discussed offline, please add some unit test for this new function.

aslonnie · 2025-06-11T01:38:31Z

release/ray_release/job_manager/kuberay_job_manager.py

+            response = requests.post(url, json=request, headers=headers)
+            print(f"Response: {response}")
+            response.raise_for_status()
+        except Exception as e:


(kind reminder)

aslonnie · 2025-06-11T01:39:24Z

release/ray_release/job_manager/kuberay_job_manager.py

+        while True:
+            now = time.time()
+            if now >= deadline_timestamp:
+                self._terminate_job()
+                if not job_running:
+                    raise JobStartupTimeout(
+                        "Cluster did not start within "
+                        f"{self.cluster_startup_timeout} seconds."
+                    )
+                raise CommandTimeout(f"Job timed out after {timeout_sec} seconds")
+
+            if now >= next_status_timestamp:
+                if job_running:
+                    logger.info(
+                        f"... job still running ... ({int(now - start_timestamp)} seconds, {int(deadline_timestamp - now)} seconds to timeout)"
+                    )
+                else:
+                    logger.info(
+                        f"... job not yet running ... ({int(now - start_timestamp)} seconds, {int(deadline_timestamp - now)} seconds to timeout)"
+                    )
+                next_status_timestamp += JOB_STATUS_CHECK_INTERVAL


are these waiting code copied from the other job manager?

yes pretty much

aslonnie · 2025-06-11T01:40:26Z

release/ray_release/scripts/run_release_test.py

+    # if not anyscale_project:
+    #     raise ReleaseTestCLIError(
+    #         "You have to set the ANYSCALE_PROJECT environment variable!"
+    #     )


condition this check on if it is running an anyscale job?

aslonnie · 2025-06-11T01:41:22Z

release/ray_release/glue.py

    test_definition_root: Optional[str] = None,
    log_streaming_limit: int = LAST_LOGS_LENGTH,
+) -> Result:
+    if test.get_env() == "kuberay":


this should be using is_kuberay()?

aslonnie · 2025-06-11T01:41:59Z

release/ray_release/test.py

+    def get_env(self) -> str:
+        """
+        Returns the environment of the test.
+        """
+        return self.get("env")


other than checking is_kuberay, this function does not seem to be used anywhere?

maybe just remove this?

aslonnie · 2025-06-11T01:42:39Z

release/ray_release/util.py

    return joined_path
+
+
+def convert_cluster_compute_to_kuberay_compute_config(compute_config: dict) -> dict:


so this function here can be removed now? since the other PR is merged?

Signed-off-by: kevin <kevin@anyscale.com>

khluu · 2025-06-13T19:50:29Z

Tune test is passing: https://buildkite.com/ray-project/release/builds/45415

Signed-off-by: kevin <kevin@anyscale.com>

aslonnie · 2025-06-13T21:05:51Z

release/ray_release/glue.py

    # Workaround while Anyscale Jobs don't support leaving cluster alive
    # after the job has finished.


could you remove these comments too?

Signed-off-by: kevin <kevin@anyscale.com>

Signed-off-by: Lonnie Liu <95255098+aslonnie@users.noreply.github.com> Signed-off-by: Lonnie Liu <lonnie@anyscale.com>

aslonnie · 2025-06-14T11:51:46Z

@khluu helped you resolved the merge conflict and merged.

please remember to follow up with unit tests

Release tests seem to be failing because of a merge conflict on master after #53390 was merged where a param is missing Signed-off-by: kevin <kevin@anyscale.com>

starts running gke tests nightly Signed-off-by: kevin <kevin@anyscale.com> Signed-off-by: Lonnie Liu <95255098+aslonnie@users.noreply.github.com> Signed-off-by: Lonnie Liu <lonnie@anyscale.com> Co-authored-by: Lonnie Liu <95255098+aslonnie@users.noreply.github.com> Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>

Release tests seem to be failing because of a merge conflict on master after #53390 was merged where a param is missing Signed-off-by: kevin <kevin@anyscale.com> Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>

Release tests seem to be failing because of a merge conflict on master after ray-project#53390 was merged where a param is missing Signed-off-by: kevin <kevin@anyscale.com>

starts running gke tests nightly Signed-off-by: kevin <kevin@anyscale.com> Signed-off-by: Lonnie Liu <95255098+aslonnie@users.noreply.github.com> Signed-off-by: Lonnie Liu <lonnie@anyscale.com> Co-authored-by: Lonnie Liu <95255098+aslonnie@users.noreply.github.com> Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>

Release tests seem to be failing because of a merge conflict on master after #53390 was merged where a param is missing Signed-off-by: kevin <kevin@anyscale.com> Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>

p

6fc0a29

Signed-off-by: kevin <kevin@anyscale.com>

khluu requested a review from a team as a code owner May 29, 2025 00:26

khluu requested a review from aslonnie May 29, 2025 00:28

aslonnie reviewed May 30, 2025

View reviewed changes

khluu added 4 commits May 30, 2025 22:31

p

78b42a6

Signed-off-by: kevin <kevin@anyscale.com>

test

7fb81b1

Signed-off-by: kevin <kevin@anyscale.com>

use same credentials as gce

544c75b

Signed-off-by: kevin <kevin@anyscale.com>

p

c76b672

Signed-off-by: kevin <kevin@anyscale.com>

khluu requested a review from aslonnie May 30, 2025 23:51

p

07d0564

Signed-off-by: kevin <kevin@anyscale.com>

aslonnie reviewed May 31, 2025

View reviewed changes

khluu added 5 commits June 5, 2025 07:29

Merge branch 'master' into khluu/gke_1test

7507950

fix dep merge

1036296

Signed-off-by: kevin <kevin@anyscale.com>

fixes

81d77ad

Signed-off-by: kevin <kevin@anyscale.com>

lint

d16a48c

Signed-off-by: kevin <kevin@anyscale.com>

p

52f7ec5

Signed-off-by: kevin <kevin@anyscale.com>

khluu requested a review from aslonnie June 9, 2025 22:55

khluu added 2 commits June 10, 2025 09:14

p

cec235d

Signed-off-by: kevin <kevin@anyscale.com>

lint

cb8d402

Signed-off-by: kevin <kevin@anyscale.com>

khluu changed the title ~~[ci] Release test on GKE~~ [ci] First release test on GKE Jun 10, 2025

lint

82c8902

Signed-off-by: kevin <kevin@anyscale.com>

aslonnie reviewed Jun 11, 2025

View reviewed changes

khluu added 5 commits June 11, 2025 21:03

Merge branch 'master' into khluu/gke_1test

a87fdaf

p

13ed910

Signed-off-by: kevin <kevin@anyscale.com>

p

6ff187d

Signed-off-by: kevin <kevin@anyscale.com>

p

cbc1b7d

Signed-off-by: kevin <kevin@anyscale.com>

lint

8cd8c8d

Signed-off-by: kevin <kevin@anyscale.com>

aslonnie self-requested a review June 11, 2025 23:47

aslonnie added the go add ONLY when ready to merge, run all tests label Jun 13, 2025

p

0aa21d1

Signed-off-by: kevin <kevin@anyscale.com>

aslonnie reviewed Jun 13, 2025

View reviewed changes

p

764fe34

Signed-off-by: kevin <kevin@anyscale.com>

aslonnie self-requested a review June 14, 2025 03:27

Merge branch 'master' into khluu/gke_1test

aff9290

Signed-off-by: Lonnie Liu <95255098+aslonnie@users.noreply.github.com> Signed-off-by: Lonnie Liu <lonnie@anyscale.com>

aslonnie force-pushed the khluu/gke_1test branch from cbd7f11 to aff9290 Compare June 14, 2025 10:59

aslonnie approved these changes Jun 14, 2025

View reviewed changes

aslonnie merged commit af6b469 into master Jun 14, 2025
5 checks passed

aslonnie deleted the khluu/gke_1test branch June 14, 2025 11:51

khluu mentioned this pull request Jun 16, 2025

[release] Fix release tests #53855

Merged

aslonnie pushed a commit that referenced this pull request Jun 16, 2025

[release] Fix release tests (#53855)

28e3a72

Release tests seem to be failing because of a merge conflict on master after #53390 was merged where a param is missing Signed-off-by: kevin <kevin@anyscale.com>

		return joined_path


		def convert_cluster_compute_to_kuberay_compute_config(compute_config: dict) -> dict:

		return config


		def upload_working_dir(working_dir: str) -> str:

		# Workaround while Anyscale Jobs don't support leaving cluster alive
		# after the job has finished.

		@@ -0,0 +1 @@
		GOOGLE_CLOUD_PROJECT="dhyey-dev"

Conversation

khluu commented May 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!