-
Notifications
You must be signed in to change notification settings - Fork 334
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
[RayService] Add e2e tests #1167
Conversation
Signed-off-by: cindyz <cindyz@anyscale.com>
Signed-off-by: cindyz <cindyz@anyscale.com>
Signed-off-by: cindyz <cindyz@anyscale.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 addressing all my comments! The change looks good to me.
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.
Leave some comments; the rest looks good to me! Having these tests would be very helpful!
routePrefix: "/" | ||
rayActorOptions: | ||
numCpus: 0.1 | ||
serveConfigV2: | |
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.
How many CPUs will these applications utilize?
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.
0.5 per replica, it should scale to 14 replicas in the test so that would require 7 CPUs.
######################headGroupSpecs################################# | ||
headGroupSpec: | ||
# The `rayStartParams` are used to configure the `ray start` command. | ||
# See https://github.com/ray-project/kuberay/blob/master/docs/guidance/rayStartParams.md for the default settings of `rayStartParams` in KubeRay. | ||
# See https://docs.ray.io/en/latest/cluster/cli.html#ray-start for all available options in `rayStartParams`. | ||
rayStartParams: {"num-cpus": "0"} | ||
rayStartParams: |
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.
What is the reason for this change?
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 didn't make the changes from the original autoscaler file, instead I added autoscaling configurations to the original config/samples/ray_v1alpha1_rayservice.yaml
file.
Signed-off-by: cindyz <cindyz@anyscale.com>
Signed-off-by: cindyz <cindyz@anyscale.com>
|
||
scale_up_rule = AutoscaleRule( | ||
query={"path": "/", "json_args": {}}, | ||
num_repeat=20, |
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 mean sending 20 requests? How do we know it will scale up to 14 replicas (7 CPUs) instead of another number, such as 5 replicas?
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 do not understand when will the Serving autoscaling be triggered.
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 use the deployment from https://github.com/ray-project/serve_workloads/blob/main/autoscaling_test/blocked.py. If you send requests to this deployment, the request will block indefinitely until https://github.com/ray-project/serve_workloads/blob/main/autoscaling_test/signaling.py releases the "lock". The target number of ongoing requests per replica is 1, so serve will try to add enough replicas to serve all 20 requests (since all requests are blocked).
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.
Thank you for the explanation! We may need to add comments to both this test and the YAML file.
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.
Good point. I've added comments to both the test and yaml.
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.
LGTM
|
||
scale_up_rule = AutoscaleRule( | ||
query={"path": "/", "json_args": {}}, | ||
num_repeat=20, |
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.
Thank you for the explanation! We may need to add comments to both this test and the YAML file.
Signed-off-by: cindyz <cindyz@anyscale.com>
Add e2e tests
Why are these changes needed?
4 e2e tests for RayService:
The e2e tests should be run with the following command:
To run a specific test in
test_sample_rayservice_yamls.py
, specify the test name with-k
, e.g:Related issue number
Checks