-
Notifications
You must be signed in to change notification settings - Fork 635
Get details of only declarative serve apps #4084
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
Get details of only declarative serve apps #4084
Conversation
Signed-off-by: Jugal Shah <j.shah@workday.com>
Signed-off-by: Jugal Shah <j.shah@workday.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.
Hi, @jugalshah291
is this backward compatible?
what will happen when
- new kuberay + old ray image
- old kuberay + new ray image
will all of them work as expected?
update:
after traced the code from ray, I think it's backward compatible
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.
You can fix CI error here
https://github.com/ray-project/kuberay/blob/master/docs/development/development.md#pre-commit-hooks
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 thinking that we should add e2e test for this PR or not.
cc @andrewsykim @rueian
Yeah I had tested in my local and also went through ray
|
Signed-off-by: Jugal Shah <j.shah@workday.com>
I am not sure how the e2e test are setup can you share the file where existing tests are present, I can add to it |
Since this is adding a new query parameter, we need e2e tests to make sure it doesn't break on old Ray versions. And I think the existing e2e tests are already enough to cover that. |
@Future-Outlier can we merge this |
cc @rueian for the merge, thank you! |
Hi @rueian can you please get help with getting this merge |
Why are these changes needed?
As part of this PR I am trying to address Problem 2 raised in issue ray-project/ray#44226.
The main aim is to enable KubeRay to exclusively check the status of only DECLARATIVE Serve apps.
The solution would be build on top of this ray-project/ray#45522
Based on my current understanding, it seems KubeRay should only operate on the DECLARATIVE Serve apps
Thus my solution will involve two key steps:
Update the
/api/serve/applications/
endpoint to read the APIType from the request body and pass it on to the controller controller.get_serve_instance_details - Ray ray-project/ray#56458(This PR) Modify KubeRay to explicitly pass Declarative as the APIType when calling the
/api/serve/applications/
Related issue number
Checks