Adding serve support for Api server #1728

blublinsky · 2023-12-11T12:16:21Z

Why are these changes needed?

Extending API server to support serve

Related issue number

Checks

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

blublinsky · 2023-12-11T12:17:37Z

@tedhtchang, @z103cb please take a look

cc: @kevin85421

tedhtchang

Very Nice! I was able to verify the PR following the instruction. There are some typos otherwise LGTM. Thanks

apiserver/ServeSubmission.md

blublinsky · 2023-12-12T07:59:20Z

@kevin85421 its ready for you

kevin85421 · 2023-12-12T21:01:46Z

The CI fails consistently. Would you mind taking a look? Thanks!

tedhtchang · 2023-12-13T08:04:48Z

This test sometimes PASSED locally.

pytest --log-level=info -v -s tests/test_sample_rayservice_yamls.py::TestRayService::test_zero_downtime_rollout

Sometimes failed with

Failed to connect to rayservice-sample-serve-svc.default.svc.cluster.local port 8000: Connection refused
command terminated with exit code 7
....
INFO     framework.utils:utils.py:375 Execute command (check_output): kubectl exec curl -n default -- curl -X POST -H 'Content-Type: application/json' rayservice-sample-serve-svc.default.svc.cluster.local:8000/fruit/ -d '["MANGO", 2]'
INFO     framework.utils:utils.py:382 Exception: b''
====================================================

or

....
INFO     test_sample_rayservice_yamls:test_sample_rayservice_yamls.py:149 Ray service transitioned to status Running.
INFO     test_sample_rayservice_yamls:test_sample_rayservice_yamls.py:154 Ray service has moved to cluster "rayservice-sample-raycluster-xtvwr"
INFO     framework.utils:utils.py:375 Execute command (check_output): kubectl exec curl -n default -- curl -X POST -H 'Content-Type: application/json' rayservice-sample-serve-svc.default.svc.cluster.local:8000/fruit/ -d '["MANGO", 2]'
INFO     framework.utils:utils.py:379 Output: b'6'
============================================================================== short test summary info ==============================================================================
FAILED tests/test_sample_rayservice_yamls.py::TestRayService::test_zero_downtime_rollout - AssertionError

blublinsky · 2023-12-13T08:15:32Z

ANd I do not think this PR changes anything there

kevin85421 · 2023-12-14T01:13:48Z

Hmm, I'm not referring to the test-rayservice-sample-yamls-latest-release. More than one CI check has failed. You may need to rebase the branch or open a new PR instead.

blublinsky · 2023-12-14T14:28:52Z

@kevin85421 we are all set

kevin85421

Perhaps the KubeRay API server should construct its own HTTP client instead of using the existing one in KubeRay.

The dashboard HTTP client in KubeRay is not a user-facing API, so no one promises its backward compatibility. Constructing a new one in KubeRay API server can improve the stability.
Reducing the dependencies between KubeRay API server and KubeRay can avoid a lot of issues for users and developers. For example, KubeRay API server may want to support multiple versions of KubeRay and Ray. It is hard to achieve if there are a lot of dependencies between them.
The fact that these functions are not used by KubeRay means they are not tested by end-to-end tests and not utilized by users. Therefore, it is difficult to ascertain whether they work or not.
Using the dashboard HTTP client in KubeRay for communication with Ray doesn't seem to make much sense to me. The KubeRay API server can directly talk to Ray.

blublinsky · 2023-12-14T18:52:16Z

Sorry @kevin85421 , I completely disagree with your statements, see point-by-point replies below

Perhaps the KubeRay API server should construct its own HTTP client instead of using the existing one in KubeRay.
I do not think it makes sense to maintain 2 copies of identical code
The dashboard HTTP client in KubeRay is not a user-facing API, so no one promises its backward compatibility. Constructing a new one in the KubeRay API server can improve the stability.
It is not user-facing in the API server either. API Server uses these APIs to communicate with the Ray cluster and then convert them to the internal representation defined by its own APIs
Reducing the dependencies between KubeRay API server and KubeRay can avoid a lot of issues for users and developers. For example, the KubeRay API server may want to support multiple versions of KubeRay and Ray. It is hard to achieve if there are a lot of dependencies between them.
Theoretically, it might be right, but practically will lead to a huge duplication of the code. The API server does depend on the KubeRay operator
The fact that these functions are not used by KubeRay means they are not tested by end-to-end tests and not utilized by users. Therefore, it is difficult to ascertain whether they work or not.
That's why I was testing them. Besides, they are used by KubeRay operator
Using the dashboard HTTP client in KubeRay for communication with Ray doesn't seem to make much sense to me. The KubeRay API server can directly talk to Ray.
It does talk to Ray directly using the dashboard HTTP client as a code to do this

blublinsky · 2023-12-15T19:46:41Z

@kevin85421, so where do we go from here. We disagree, should we ask for the third opinion?

blublinsky · 2023-12-20T08:31:17Z

@kevin85421 we need to come to some kind of resolution here. Leaving it hanging is not a solution @architkulkarni ?
Also, Ray job support is using the same class

apiserver/pkg/server/ray_serve_submission_service_server.go

blublinsky · 2024-01-26T20:09:25Z

@kevin85421 it has been over a month. Any chance you can get to this?

kevin85421 · 2024-02-01T06:41:12Z

I will remove all functions in dashboard_httpclient.go that are not used by the KubeRay operator itself before the release of KubeRay v1.1.0. I don't want to have any unnecessary dependencies between KubeRay and KubeRay API server. In addition, I may also consider to move the API server to a separate repository.

We have 3 possible options to clean up dashboard_httpclient.go

I revert Added Job submission support to the API server #1639.
I open a PR for a dashboard client for API server.
You can open a PR for a dashboard client for API server.

blublinsky · 2024-02-01T09:28:34Z

I will remove all functions in dashboard_httpclient.go that are not used by the KubeRay operator itself before the release of KubeRay v1.1.0. I don't want to have any unnecessary dependencies between KubeRay and KubeRay API server. In addition, I may also consider to move the API server to a separate repository.

There are 2 methods added there:

DeployApplication(context.Context, string) error which was copied from driver, thus making it simpler and cleaner trivial 10 lines of code
DeleteServeApplications(context.Context) error, which is 15 trivial lines of code

The biggest change is updating the return definition to align it with the definition of the serve implementation

Do whatever you have to do. This is taking way too long

rebased to master

blublinsky · 2024-03-01T11:28:50Z

@kevin85421 moved dashboard_httpclient.go API server specific version and supporting files to the API server. Can we, please, finally merge it

blublinsky · 2024-03-01T12:07:18Z

@kevin85421 the only change to the ray operator is in pod.go to capitalize initLivenessAndReadinessProbe to make it visible

kevin85421 · 2024-04-02T20:15:47Z

ray-operator/controllers/ray/common/pod.go

@@ -239,7 +239,7 @@ func DefaultWorkerPodTemplate(ctx context.Context, instance rayv1.RayCluster, wo
 	return podTemplate
 }

-func initLivenessAndReadinessProbe(rayContainer *corev1.Container, rayNodeType rayv1.RayNodeType, creatorCRDType utils.CRDType) {


It is better to avoid having the KubeRay API server call functions within KubeRay. I will only take care of the CRD compatibility, not the internal functions.

Oh, here we go again. There is plenty of dependencies already. Do you really assume reproducing all of the code in the API server? This makes no sense to me, sorry. Can we, please, get a second opinion from one of your colleges?
We have been sitting on this PR for several month right now and its getting counter productive. I can do requested enhancements anymore, because we are stuck here for several month

tedhtchang approved these changes Dec 12, 2023

View reviewed changes

kevin85421 self-assigned this Dec 12, 2023

blublinsky force-pushed the servesupport branch from 1279161 to e4c5db8 Compare December 14, 2023 08:14

kevin85421 requested changes Dec 14, 2023

View reviewed changes

blublinsky force-pushed the servesupport branch 3 times, most recently from 6b4e022 to 900f6c2 Compare January 8, 2024 09:41

z103cb reviewed Jan 8, 2024

View reviewed changes

apiserver/pkg/server/ray_serve_submission_service_server.go Outdated Show resolved Hide resolved

blublinsky force-pushed the servesupport branch from d716d2f to c0553dd Compare January 10, 2024 09:06

Yicheng-Lu-llll mentioned this pull request Jan 17, 2024

[RayService][Hotfix] Hotfix for Flaky Zero Downtime Rollout Test #1837

Merged

4 tasks

blublinsky force-pushed the servesupport branch 3 times, most recently from 12e7f45 to daf060d Compare January 18, 2024 19:52

blublinsky force-pushed the servesupport branch from a06d2b2 to c4bdff5 Compare January 25, 2024 20:14

anyscalesam added the enhancement New feature or request label Jan 29, 2024

blublinsky added 2 commits March 1, 2024 09:46

Adding serve support for Api server

360023b

rebased to master

Adding serve support for Api server

a3dbe74

blublinsky added 9 commits March 1, 2024 09:48

cleaned up documentation

9efff1f

updated after rebasing

98eb373

moved dashboard httpclient and supporting code to the API server

c597fdd

moved dashboard httpclient and supporting code to the API server

d787ad9

addressed comments

bf31cc3

merge to master

30bd623

Fixing Python client

5bdb174

documentation updates

26fa2eb

moved dashboard httpclient and supporting code to the API server

4f46423

blublinsky force-pushed the servesupport branch from 46642c4 to 4f46423 Compare March 1, 2024 11:02

moved dashboard httpclient and supporting code to the API server

47b8fd8

blublinsky added 2 commits March 1, 2024 11:48

additional linting

e2b9815

additional linting

65e65df

switch to quay

e816ed4

kevin85421 reviewed Apr 2, 2024

View reviewed changes

blublinsky mentioned this pull request Apr 9, 2024

[Feature] Support ImagePullPolicy in Worker and Head NodeSpec #2047

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding serve support for Api server #1728

Adding serve support for Api server #1728

blublinsky commented Dec 11, 2023

blublinsky commented Dec 11, 2023

tedhtchang left a comment •

edited

Loading

blublinsky commented Dec 12, 2023

kevin85421 commented Dec 12, 2023

tedhtchang commented Dec 13, 2023 •

edited

Loading

blublinsky commented Dec 13, 2023

kevin85421 commented Dec 14, 2023

blublinsky commented Dec 14, 2023

kevin85421 left a comment

blublinsky commented Dec 14, 2023 •

edited

Loading

blublinsky commented Dec 15, 2023

blublinsky commented Dec 20, 2023

blublinsky commented Jan 26, 2024

kevin85421 commented Feb 1, 2024

blublinsky commented Feb 1, 2024 •

edited

Loading

blublinsky commented Mar 1, 2024

blublinsky commented Mar 1, 2024

kevin85421 Apr 2, 2024

blublinsky Apr 2, 2024

Adding serve support for Api server #1728

Are you sure you want to change the base?

Adding serve support for Api server #1728

Conversation

blublinsky commented Dec 11, 2023

Why are these changes needed?

Related issue number

Checks

blublinsky commented Dec 11, 2023

tedhtchang left a comment • edited Loading

Choose a reason for hiding this comment

blublinsky commented Dec 12, 2023

kevin85421 commented Dec 12, 2023

tedhtchang commented Dec 13, 2023 • edited Loading

blublinsky commented Dec 13, 2023

kevin85421 commented Dec 14, 2023

blublinsky commented Dec 14, 2023

kevin85421 left a comment

Choose a reason for hiding this comment

blublinsky commented Dec 14, 2023 • edited Loading

blublinsky commented Dec 15, 2023

blublinsky commented Dec 20, 2023

blublinsky commented Jan 26, 2024

kevin85421 commented Feb 1, 2024

blublinsky commented Feb 1, 2024 • edited Loading

blublinsky commented Mar 1, 2024

blublinsky commented Mar 1, 2024

kevin85421 Apr 2, 2024

Choose a reason for hiding this comment

blublinsky Apr 2, 2024

Choose a reason for hiding this comment

tedhtchang left a comment •

edited

Loading

tedhtchang commented Dec 13, 2023 •

edited

Loading

blublinsky commented Dec 14, 2023 •

edited

Loading

blublinsky commented Feb 1, 2024 •

edited

Loading