-
Notifications
You must be signed in to change notification settings - Fork 364
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
Exposing Serve Service #1117
Exposing Serve Service #1117
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there's a fair amount of duplicated code and test code from #1040, is it possible to refactor it to avoid duplicating code as much as possible? For example, by pulling out common code into helper functions.
For the test code, I'm less sure, but I think it's possible to use "table-driven tests" to parametrize multiple tests that are very similar.
Will give this a detailed review as soon as I can.
@architkulkarni Added helper function to reduce the duplicated code both in the implementation and test files. Please take a look. |
Signed-off-by: Siddharth Kodwani <96103980+kodwanis@users.noreply.github.com>
Signed-off-by: Siddharth Kodwani <96103980+kodwanis@users.noreply.github.com>
Signed-off-by: Siddharth Kodwani <96103980+kodwanis@users.noreply.github.com>
Signed-off-by: Siddharth Kodwani <96103980+kodwanis@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the refactor, looks good! Just one nit about the warning but it shouldn't block the merge.
Before merging I would like to get @kevin85421's opinion on just one point, which is the handling of ports. @kevin85421 do you think it makes sense? I don't have any strong preferences here for how to handle them.
// Add DeafultServePort if it is already not added and ignore any custom ports | ||
// Keeping this consistentent with adding only serve port in serve service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Add DeafultServePort if it is already not added and ignore any custom ports | |
// Keeping this consistentent with adding only serve port in serve service | |
// Add DefaultServePort if it is already not added and ignore any custom ports | |
// Keeping this consistent with adding only serve port in serve service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to print or log a warning when we see a custom port and ignore it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add the warning.
@kodwanis Sorry, do you mind fixing the last merge conflict? |
Signed-off-by: Siddharth Kodwani <96103980+kodwanis@users.noreply.github.com>
Signed-off-by: Siddharth Kodwani <96103980+kodwanis@users.noreply.github.com>
@architkulkarni resolved the conflict and added log statement for ports |
Signed-off-by: Siddharth Kodwani <96103980+kodwanis@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind adding the expected result of ray-service.custom-serve-service.yaml in the PR description? I will clone this branch and manually test it. Thanks!
@kevin85421 Updated the PR description with expected output for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Others look good to me.
Hi, I suggest to hold this change and introduce to beta APIs. #1146 is added and we can grow new fields in latest API version and controller version. |
Hi @Jeffwan, we may not update the version of RayService for v0.6.0. Instead, we will update the versions for RayJob and RayCluster in v0.6.0. I believe it is safe to merge this PR as it doesn't introduce any backward compatibility issues or cause significant delays for users. This PR is similar to Archit's PR #1040. Is it OK for you? Thanks! |
Experiments: # Run test
RAY_IMAGE=rayproject/ray:nightly OPERATOR_IMAGE=controller:latest python3 tests/compatibility-test.py RayFTTestCase.test_ray_serve 2>&1 | tee log
# Check Ray commit (in head Pod)
python3 -c "import ray; print(ray.__commit__)"
|
Thanks for testing it out manually. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why does it consistently fails on GitHub Actions, but it can pass on my devbox. We need to prioritize #1058.
![Screen Shot 2023-06-07 at 5 10 25 PM](https://private-user-images.githubusercontent.com/20109646/244229800-37330437-8832-4fdb-995b-1cfccdd7e80b.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU0NjA2MDAsIm5iZiI6MTcyNTQ2MDMwMCwicGF0aCI6Ii8yMDEwOTY0Ni8yNDQyMjk4MDAtMzczMzA0MzctODgzMi00ZmRiLTk5NWItMWNmY2NkZDdlODBiLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MDQlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTA0VDE0MzE0MFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTVjZDE0ODZjZmMyNzgxOGNkNGFmOTRlZmE5ODRjODU3OGNhNDM1NjFhZDhhZTlmNTIzOTU3MWUxNDY3MDg0ZmUmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.jLKqHgQUPjf5jEl11gnoJWX4HZWVxV5HwLquix0RD_0)
Experiments:
# Run test
RAY_IMAGE=rayproject/ray:nightly OPERATOR_IMAGE=controller:latest python3 tests/compatibility-test.py RayFTTestCase.test_ray_serve 2>&1 | tee log
# Check Ray commit (in head Pod)
python3 -c "import ray; print(ray.__commit__)"
- KubeRay: This PR, Ray: 2081ebb5724785f60a77b4ec482fb7e8f9baab0d => 1 Pass
Merge this PR before passing the nightly compatibility test. The failing has no relationship with this PR. See #1117 (review) for more details. |
Exposing Serve Service
Why are these changes needed?
This PR exposes the serve service and all of its fields as part of the RayService CRD. This allow users to pass custom fields like labels, annotations and other fields to Serve Service.
This is similar to exposing head service to users addressed in #1040
Here's the definitions of the priorities:
Name: If it's specified in ServeService, override the default.
Namespace: ignore user-provided namespace in ServeService. Enforce it to be the same namespace as the RayCluster.
annotations field: user-provided fields are passed as is.
Selector field: Ignore user specified selector fields and keep the default fields
Labels field: default field values takes priority user specified fields. Other user specified fields passed are merged.
Ports field: Keeping the behavior consisted of adding only serve port. If serve port is defined in rayCluster, that takes priority and user specified ports are ignored. If serve port is not defined in rayCluster, user specified serve port is added. All ports other than serve port are ignored.
Type: If it's specified in ServeService, override the default.
Expected result
The
serve-svc
should have the service defined by the user. It should contain the user defined labels, annotations, Type, name (if passed) and "serve" port (if not defined in the cluster config).Expected output for ray-service.custom-serve-service.yaml:
Related issue number
Closes #1034
Checks