-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
[Serve] Expose Serve app source in get_serve_instance_details
#45522
[Serve] Expose Serve app source in get_serve_instance_details
#45522
Conversation
Signed-off-by: Josh Karpel <[email protected]>
Signed-off-by: Josh Karpel <[email protected]>
@@ -148,6 +148,7 @@ def autoscaling_app(): | |||
"message": "", | |||
"last_deployed_time_s": deployment_timestamp, | |||
"deployed_app_config": None, | |||
"source": 1, |
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.
This feels awkward to me - can the enum be a https://docs.python.org/3/library/enum.html#enum.StrEnum instead? Or perhaps if they need to be int
s we'd use the existing enum field names to convert at the last moment, just for the API (not internally)?
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.
Yes, I think it would be better to change this to a string enum, now that it's exposed through the rest api.
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 contribution Josh! Could you add test(s) to check that the source is exposed correctly through the rest api? You can add it to https://github.com/ray-project/ray/blob/master/dashboard/modules/serve/tests/test_serve_dashboard.py
.
Aha! I knew these tests must be somewhere! Will add some tests there. |
Signed-off-by: Josh Karpel <[email protected]>
…urce Signed-off-by: Josh Karpel <[email protected]>
…urce Signed-off-by: Josh Karpel <[email protected]>
Signed-off-by: Josh Karpel <[email protected]>
…urce Signed-off-by: Josh Karpel <[email protected]>
Signed-off-by: Josh Karpel <[email protected]>
It looks like the setup for these tests is a little laborious, so I attached it to the existing |
…urce Signed-off-by: Josh Karpel <[email protected]>
Signed-off-by: Josh Karpel <[email protected]>
Head branch was pushed to by a user without write access
Signed-off-by: Josh Karpel <[email protected]>
…urce Signed-off-by: Josh Karpel <[email protected]>
@zcin I think I could use some help - I'm getting consistent timeout failures in
It looks like the Raylet crashes during teardown after the first test, then the second test blocks forever (note that I
If I remove the ray/python/ray/serve/tests/conftest.py Lines 149 to 167 in 42d9e05
|
Signed-off-by: Josh Karpel <[email protected]>
@JoshKarpel It is because adding |
…rce' into issue-44226-expose-serve-app-source Signed-off-by: Josh Karpel <[email protected]>
Signed-off-by: Josh Karpel <[email protected]>
…urce Signed-off-by: Josh Karpel <[email protected]>
Signed-off-by: Josh Karpel <[email protected]>
Sorry, didn't intend that to send out another review request - still having trouble with the tests. Moved back to draft. |
Signed-off-by: Josh Karpel <[email protected]>
Signed-off-by: Josh Karpel <[email protected]>
Signed-off-by: Josh Karpel <[email protected]>
Signed-off-by: Josh Karpel <[email protected]>
@@ -100,7 +100,7 @@ py_test( | |||
py_test( | |||
name = "test_serve_dashboard", | |||
size = "enormous", | |||
srcs = ["modules/serve/tests/test_serve_dashboard.py"], | |||
srcs = ["modules/serve/tests/test_serve_dashboard.py", "modules/serve/tests/deploy_imperative_serve_apps.py"], |
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'm not familiar with Bazel - is this the right way to declare that this test needs this file? Or should it go under eg. deps
?
@@ -418,6 +422,104 @@ def applications_running(): | |||
assert app_details[app].last_deployed_time_s > 0 | |||
assert app_details[app].route_prefix == expected_values[app]["route_prefix"] | |||
assert app_details[app].docs_path == expected_values[app]["docs_path"] | |||
assert app_details[app].source == expected_values[app]["source"] | |||
|
|||
# CHECK: all deployments are present |
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.
The diff is a bit confusing here because there's a lot of duplicated code between this existing test case and the new test_get_serve_instance_details_for_imperative_apps
below - the code in this test was already here!
"route_prefix": deployment_args.route_prefix | ||
if deployment_args.HasField("route_prefix") | ||
else None, | ||
"route_prefix": ( | ||
deployment_args.route_prefix | ||
if deployment_args.HasField("route_prefix") | ||
else None | ||
), | ||
"ingress": deployment_args.ingress, | ||
"docs_path": deployment_args.docs_path | ||
if deployment_args.HasField("docs_path") | ||
else None, | ||
"docs_path": ( | ||
deployment_args.docs_path | ||
if deployment_args.HasField("docs_path") | ||
else None | ||
), |
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.
Linter seemed to want these changes, not sure why they weren't already like this 🤔
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!
…project#45522) ## Why are these changes needed? This change exposes the new Serve app submission API type tracking introduced in ray-project#44476 in the dashboard API. My intent is to eventually introduce an *option* in KubeRay to *only* care about the status of `DECLARATIVE` Serve apps, so that it doesn't care about "dynamically deployed" `IMPERATIVE` apps. Per request, I've indicated that this is a developer API in the field docstring. ## Related issue number Follow-up for ray-project#44226 (comment) --------- Signed-off-by: Josh Karpel <[email protected]>
Why are these changes needed?
This change exposes the new Serve app submission API type tracking introduced in #44476 in the dashboard API.
My intent is to eventually introduce an option in KubeRay to only care about the status of
DECLARATIVE
Serve apps, so that it doesn't care about "dynamically deployed"IMPERATIVE
apps.Per request, I've indicated that this is a developer API in the field docstring.
Related issue number
Follow-up for #44226 (comment)
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.