Added Job submission support to the API server #1639

blublinsky · 2023-11-09T13:46:53Z

Why are these changes needed?

API server already provides support for Ray Job, but its usage is not very convenient for Python developer. Its not easy to get execution log, stop running job, etc. This functionality is provided by Ray job submission API, but requires knowledge about endpoint and corresponding security when used outside the cluster. This implementation provides all the functionality through the API server (with the well known URL) and corresponding Python client

Related issue number

Checks

[x ] I've made sure the tests are passing.
Testing Strategy
- [x ] Unit tests
- Manual tests
- This PR is not tested :(

blublinsky · 2023-11-09T13:49:12Z

@z103cb @tedhtchang @astefanutti, pls take a look
cc @kevin85421

tedhtchang

Looks good. I was able to follow the README to test the PR. There are some typos.

tedhtchang · 2023-11-10T04:38:03Z

apiserver/JobSubmission.md

@@ -0,0 +1,211 @@
+# Job Submission using API server
+
+Ray provides very convinient and powerful [Job Submission APIs](https://docs.ray.io/en/latest/cluster/running-applications/job-submission/rest.html). The issue is that it needs to access cluster's dashboard Ingress, which currently does not have any security implementations. Alternatively you can use Ray cluster head service for this, but in this case your Ray job management code has to run in the same kubernetes cluster as Ray. Job submission implementation in the API server leverages already exposed URL of the API server to locate cluster and use its head service to implement Job management functionality. Because you can access API server running on the remote kubernetes cluster you can use these APIs for managing remote Ray clusters without exposing them via Ingress.


Suggested change

Ray provides very convinient and powerful [Job Submission APIs](https://docs.ray.io/en/latest/cluster/running-applications/job-submission/rest.html). The issue is that it needs to access cluster's dashboard Ingress, which currently does not have any security implementations. Alternatively you can use Ray cluster head service for this, but in this case your Ray job management code has to run in the same kubernetes cluster as Ray. Job submission implementation in the API server leverages already exposed URL of the API server to locate cluster and use its head service to implement Job management functionality. Because you can access API server running on the remote kubernetes cluster you can use these APIs for managing remote Ray clusters without exposing them via Ingress.

Ray provides very convenient and powerful [Job Submission APIs](https://docs.ray.io/en/latest/cluster/running-applications/job-submission/rest.html). The issue is that it needs to access cluster's dashboard Ingress, which currently does not have any security implementations. Alternatively you can use Ray cluster head service for this, but in this case your Ray job management code has to run in the same kubernetes cluster as Ray. Job submission implementation in the API server leverages already exposed URL of the API server to locate cluster and use its head service to implement Job management functionality. Because you can access API server running on the remote kubernetes cluster you can use these APIs for managing remote Ray clusters without exposing them via Ingress.

@blublinsky these doc changes don't show up as fixed on Github, can you double check on your end?

@blublinsky These fixes aren't showing up as fixed on Github, can you double check on your end? Are they being overwritten somehow?

tedhtchang · 2023-11-10T04:40:18Z

apiserver/JobSubmission.md

+
+### Deploy KubeRay operator and API server
+
+Reffer to [readme](README.md) for setting up KubRay operator and API server.


Suggested change

Reffer to [readme](README.md) for setting up KubRay operator and API server.

Refer to [readme](README.md) for setting up KubRay operator and API server.

Seems not fixed still? Also KubRay -> KubeRay

apiserver/JobSubmission.md

blublinsky · 2023-11-10T13:29:12Z

Thank you @tedhtchang. Can you, please, approve it

apiserver/JobSubmission.md

blublinsky · 2023-11-14T10:47:51Z

@kevin85421, I think its ready for commit

blublinsky · 2023-11-16T19:19:56Z

@kevin85421 just a reminder that this one is ready for commit

)

Signed-off-by: Anish Asthana <anishasthana1@gmail.com>

ray-project#1588)

…y-project#1589)

This PR adds e2e test following up ray-project#1539 and leverages ray-project#1575. It also factorises configuration across the RayJob e2e tests.

Signed-off-by: Boris Lublinsky <blublinsky@hotmail.com>

kevin85421 · 2023-11-16T21:00:17Z

@architkulkarni can review this PR. I will review it next week due to being overloaded this week.

architkulkarni

Looks good overall, just a few minor questions.

@blublinsky From what I can see it seems like all of Ted's doc comments haven't been fixed actually (maybe forgot to push or something got messed up when merging?)

apiserver/JobSubmission.md

apiserver/pkg/server/ray_job_submission_service_server.go

clients/python-apiserver-client/python_apiserver_client/kuberay_apis.py

clients/python-apiserver-client/python_apiserver_client/params/jobsubmission.py

ray-operator/controllers/ray/utils/dashboard_httpclient.go

clients/python-apiserver-client/python_apiserver_client/params/jobsubmission.py

kevin85421

There are some issues that we need to figure out before merging this PR. @architkulkarni is the expert on both Ray and KubeRay. @architkulkarni, could you collaborate with @blublinsky to solve these? Thanks!

(1) Some fields are required in RayJobInfo and RayJobRequest in Ray's definition. However, all fields in the struct are optional.
(2) Double-check the types between KubeRay's dashboard_httpclient.go and Ray. For example, the type of start_time is uint64 in Ray, but it is int64 in this PR, which may cause data loss.
(3) Some fields are missing in the dashboard client.
(4) Is there any change in compatibility between Ray and KubeRay?

ray-operator/controllers/ray/utils/dashboard_httpclient.go

…iserver-jobsubmission

blublinsky · 2023-11-20T20:43:33Z

@kevin85421, @architkulkarni addressed your comments

kevin85421 · 2023-11-20T21:37:15Z

e2e / ray-operator fails. @blublinsky could you take a look?

astefanutti · 2023-11-21T14:36:33Z

ray-operator/controllers/ray/utils/dashboard_httpclient.go

@@ -352,7 +367,7 @@ func (r *RayDashboardClient) GetJobInfo(ctx context.Context, jobId string) (*Ray
 	defer resp.Body.Close()

 	if resp.StatusCode == http.StatusNotFound {
-		return nil, nil
+		return nil, errors.NewBadRequest("Job " + jobId + " does not exist on the cluster")


That change breaks the RayJob controller, and the Ray cluster dashboard check that's performed prior to submitting the job, and that's been added as part of #1512, with a work-around for #1381.

I think the easier option in the short term is to update the call from the RayJob controller to check the error (better be NotFound error).

@kevin85421 @architkulkarni what do you think, you guys probably have more context on what's the best strategy to handle this?

Thank you @astefanutti. For the time being, I rolled back the changes (preserving them as comments). Lets merge this one and move the issue of the RayJob controller to check the error to a separate PR

Yes, rolling back sounds good so we can merge this PR. There is another PR in flight which is dealing with this issue: #1623

architkulkarni

@blublinsky It looks like most of the doc feedback for JobSubmission.md has been commented as "fixed" but is not actually fixed in the file. I'm okay with merging this PR to unblock your other PRs, but please help address the doc feedback either in this PR or a future PR.

clients/python-apiserver-client/python_apiserver_client/params/jobsubmission.py

blublinsky · 2023-11-21T17:36:03Z

@kevin85421, can we merge, please

kevin85421 · 2023-11-21T18:15:47Z

@architkulkarni would you mind answering #1639 (review) before we merge the PR? Thanks!

architkulkarni

(1) Some fields are required in RayJobInfo and RayJobRequest in Ray's definition. However, all fields in the struct are optional.
(2) Double-check the types between KubeRay's dashboard_httpclient.go and Ray. For example, the type of start_time is uint64 in Ray, but it is int64 in this PR, which may cause data loss.
(3) Some fields are missing in the dashboard client.
(4) Is there any change in compatibility between Ray and KubeRay?

(1) I have double checked them and indicated the necessary changes in the below comments.
(2) This was fixed in 4c12da6 and then un-fixed in 5bb51cc, it's not clear why. @blublinsky can you check this? I verified that a millisecond unix timestamp cannot overflow int64, but we might as well be consistent and use uint64.
(3) @kevin85421 which fields did you notice were missing? Are you talking about things like driver_node_id and driver_exit_code in JobInfo? These were missing before this PR and it's easy to add them later if needed. I don't think it needs to be part of this PR, but let me know if I misinterpreted your comment.
(4) No, the changes to the dashboard API client are all for Ray Job API methods that have been supported since Ray 2.2.0 (DELETE was the most recent one). KubeRay 1.1.0 only supports Ray 2.8.0 and later, so this is fine.

proto/job_submission.proto

blublinsky · 2023-11-21T19:39:04Z

I have double checked them and indicated the necessary changes in the below comments.
(2) This was fixed in 4c12da6 and then un-fixed in 5bb51cc, it's not clear why. @blublinsky can you check this? I verified that a millisecond unix timestamp cannot overflow int64, but we might as well be consistent and use uint64

These fields were already defined as int64. I did not introduce them. We can change them, but it creates more changes outside this PR

architkulkarni · 2023-11-21T19:45:31Z

We defined as required only the data that user provides data, doing it for data coming from the server does not make sense to me

Ok, that makes sense. Since we're not sending this data to the server there's no downside to leaving it as optional.

These fields were already defined as int64. I did not introduce them. We can change them, but it creates more changes outside this PR

That's fine with me, we can update it in a separate PR.

@kevin85421 thoughts? Is it okay to merge?

architkulkarni · 2023-11-21T20:07:40Z

Discussed offline with @kevin85421 , it's okay to merge. The failing test Compatibility Test - Nightly (pull_request) is flaky and unrelated (FAIL: test_detached_actor (main.RayFTTestCase.test_detached_actor))

Added Job submission support to the API server

e5131ac

tedhtchang reviewed Nov 10, 2023

View reviewed changes

apiserver/JobSubmission.md Show resolved Hide resolved

fixed grammer in JobSubmission.md

505f305

tedhtchang approved these changes Nov 10, 2023

View reviewed changes

astefanutti approved these changes Nov 14, 2023

View reviewed changes

apiserver/JobSubmission.md Outdated Show resolved Hide resolved

apiserver/JobSubmission.md Show resolved Hide resolved

apiserver/JobSubmission.md Show resolved Hide resolved

apiserver/JobSubmission.md Outdated Show resolved Hide resolved

blublinsky added 2 commits November 14, 2023 10:23

fixed punctuation in JobSubmission.md

aa61eda

Add missing swagger.json

cc27795

tedhtchang and others added 10 commits November 16, 2023 19:31

Flip Min and max replicas for apiserver workerNodeSpec (ray-project#1638

173954e

)

Make KubeRay Operator Image FIPS compliant (ray-project#1633)

3d9d481

Signed-off-by: Anish Asthana <anishasthana1@gmail.com>

chore: remove all deprecated HeadGroupSpec.replicas in example YAMLs (

eab6b2a

ray-project#1588)

chore: remove HeadGroupSpec.Replicas from raycluster_types.go (ra…

d4fc840

…y-project#1589)

[RayJob]: Add RayJob with RayCluster spec e2e test (ray-project#1636)

afda7a3

This PR adds e2e test following up ray-project#1539 and leverages ray-project#1575. It also factorises configuration across the RayJob e2e tests.

Add end to end tests to apiserver (ray-project#1460)

9ec5ed5

Update CHANGELOG for v1.0.0 (ray-project#1650)

64fb87e

Added Job submission support to the API server

1d0117f

rebased to master

9b52d18

Merge branch 'master' into apiserver-jobsubmission

b0f3dc0

Signed-off-by: Boris Lublinsky <blublinsky@hotmail.com>

kevin85421 assigned architkulkarni and kevin85421 Nov 16, 2023

architkulkarni reviewed Nov 20, 2023

View reviewed changes

kevin85421 reviewed Nov 20, 2023

View reviewed changes

blublinsky added 3 commits November 20, 2023 19:41

resolve comments

2e75436

Merge remote-tracking branch 'origin/apiserver-jobsubmission' into ap…

283b4f1

…iserver-jobsubmission

resolve comments

4c12da6

resolve comments

5bb51cc

astefanutti reviewed Nov 21, 2023

View reviewed changes

resolve comments

15a5eec

architkulkarni approved these changes Nov 21, 2023

View reviewed changes

architkulkarni reviewed Nov 21, 2023

View reviewed changes

clients/python-apiserver-client/python_apiserver_client/params/jobsubmission.py Outdated Show resolved Hide resolved

resolve comments

b44c752

architkulkarni reviewed Nov 21, 2023

View reviewed changes

proto/job_submission.proto Show resolved Hide resolved

proto/job_submission.proto Show resolved Hide resolved

architkulkarni merged commit 5c0e2e9 into ray-project:master Nov 21, 2023
24 of 25 checks passed

blublinsky deleted the apiserver-jobsubmission branch November 22, 2023 08:30

kevin85421 mentioned this pull request Feb 1, 2024

Adding serve support for Api server #1728

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Job submission support to the API server #1639

Added Job submission support to the API server #1639

blublinsky commented Nov 9, 2023

blublinsky commented Nov 9, 2023

tedhtchang left a comment

tedhtchang Nov 10, 2023

blublinsky Nov 10, 2023

architkulkarni Nov 20, 2023

architkulkarni Nov 20, 2023

tedhtchang Nov 10, 2023

blublinsky Nov 10, 2023

architkulkarni Nov 20, 2023

blublinsky commented Nov 10, 2023

blublinsky commented Nov 14, 2023

blublinsky commented Nov 16, 2023

kevin85421 commented Nov 16, 2023

architkulkarni left a comment

kevin85421 left a comment

blublinsky commented Nov 20, 2023

kevin85421 commented Nov 20, 2023

astefanutti Nov 21, 2023

blublinsky Nov 21, 2023

architkulkarni Nov 21, 2023

architkulkarni left a comment

blublinsky commented Nov 21, 2023

kevin85421 commented Nov 21, 2023

architkulkarni left a comment

blublinsky commented Nov 21, 2023

architkulkarni commented Nov 21, 2023

architkulkarni commented Nov 21, 2023

		@@ -0,0 +1,211 @@
		# Job Submission using API server

		Ray provides very convinient and powerful [Job Submission APIs](https://docs.ray.io/en/latest/cluster/running-applications/job-submission/rest.html). The issue is that it needs to access cluster's dashboard Ingress, which currently does not have any security implementations. Alternatively you can use Ray cluster head service for this, but in this case your Ray job management code has to run in the same kubernetes cluster as Ray. Job submission implementation in the API server leverages already exposed URL of the API server to locate cluster and use its head service to implement Job management functionality. Because you can access API server running on the remote kubernetes cluster you can use these APIs for managing remote Ray clusters without exposing them via Ingress.


		### Deploy KubeRay operator and API server

		Reffer to [readme](README.md) for setting up KubRay operator and API server.

	Reffer to [readme](README.md) for setting up KubRay operator and API server.
	Refer to [readme](README.md) for setting up KubRay operator and API server.

Added Job submission support to the API server #1639

Added Job submission support to the API server #1639

Conversation

blublinsky commented Nov 9, 2023

Why are these changes needed?

Related issue number

Checks

blublinsky commented Nov 9, 2023

tedhtchang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blublinsky commented Nov 10, 2023

blublinsky commented Nov 14, 2023

blublinsky commented Nov 16, 2023

kevin85421 commented Nov 16, 2023

architkulkarni left a comment

Choose a reason for hiding this comment

kevin85421 left a comment

Choose a reason for hiding this comment

blublinsky commented Nov 20, 2023

kevin85421 commented Nov 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

architkulkarni left a comment

Choose a reason for hiding this comment

blublinsky commented Nov 21, 2023

kevin85421 commented Nov 21, 2023

architkulkarni left a comment

Choose a reason for hiding this comment

blublinsky commented Nov 21, 2023

architkulkarni commented Nov 21, 2023

architkulkarni commented Nov 21, 2023