Skip to content
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] Remove ability to specify route_prefix at deployment level #48223

Merged
merged 37 commits into from
Oct 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
7005040
fix
edoakes Oct 23, 2024
8503082
fix
edoakes Oct 25, 2024
52eace7
fix
edoakes Oct 25, 2024
da37b9a
fix
edoakes Oct 25, 2024
f206f83
more fixes
edoakes Oct 25, 2024
749c9f7
more fixes
edoakes Oct 25, 2024
ac79021
more fixes
edoakes Oct 25, 2024
fa66535
more fixes
edoakes Oct 25, 2024
55abba0
Merge branch 'master' of https://github.com/ray-project/ray into remo…
edoakes Oct 25, 2024
11eaf12
fix
edoakes Oct 25, 2024
a2b3e9b
fix
edoakes Oct 25, 2024
dadc536
fix
edoakes Oct 25, 2024
8d32f64
fix
edoakes Oct 25, 2024
f22e6c3
fix
edoakes Oct 25, 2024
3c8ac47
fix
edoakes Oct 25, 2024
abcc572
fix
edoakes Oct 25, 2024
16f2c28
fix
edoakes Oct 25, 2024
05f6d29
fix
edoakes Oct 25, 2024
b35cf7a
fix
edoakes Oct 25, 2024
2d97896
more fixes
edoakes Oct 25, 2024
b3c561b
fix
edoakes Oct 25, 2024
6173940
fix
edoakes Oct 25, 2024
4c69b04
fix
edoakes Oct 25, 2024
db48abd
fix
edoakes Oct 25, 2024
87453d2
more fixes
edoakes Oct 25, 2024
2171af5
more fixes
edoakes Oct 25, 2024
d7f892c
more fixes
edoakes Oct 25, 2024
df3ecef
fix java
edoakes Oct 25, 2024
d48441e
fix
edoakes Oct 25, 2024
93c181f
fix
edoakes Oct 25, 2024
fbec568
Merge branch 'master' of https://github.com/ray-project/ray into remo…
edoakes Oct 26, 2024
e1686d6
Merge branch 'master' of https://github.com/ray-project/ray into remo…
edoakes Oct 26, 2024
dabeca7
fix
edoakes Oct 26, 2024
c339024
fix
edoakes Oct 26, 2024
1fdc44b
fix
edoakes Oct 26, 2024
3487b1f
more fixes
edoakes Oct 26, 2024
02d9f63
fix
edoakes Oct 26, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions doc/source/serve/doc_code/image_classifier_example.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ async def __call__(self, req: starlette.requests.Request):
return await self.classify(req["image_url"])


app = ImageClassifier.options(route_prefix="/classify").bind(downloader.bind())
app = ImageClassifier.bind(downloader.bind())
# __serve_example_end__


Expand Down Expand Up @@ -65,7 +65,7 @@ async def __call__(self, req: starlette.requests.Request):
# __serve_example_modified_end__


serve.run(app, name="app1")
serve.run(app, name="app1", route_prefix="/classify")
# __request_begin__
bear_url = "https://cdn.britannica.com/41/156441-050-A4424AEC/Grizzly-bear-Jasper-National-Park-Canada-Alberta.jpg" # noqa
resp = requests.post("http://localhost:8000/classify", json={"image_url": bear_url})
Expand Down
4 changes: 2 additions & 2 deletions doc/source/serve/doc_code/translator_example.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@ async def __call__(self, req: starlette.requests.Request):
return self.translate(req["text"])


app = Translator.options(route_prefix="/translate").bind()
app = Translator.bind()
# __serve_example_end__


serve.run(app, name="app2")
serve.run(app, name="app2", route_prefix="/translate")

# __request_begin__
text = "Hello, the weather is quite fine today!"
Expand Down
34 changes: 14 additions & 20 deletions java/serve/src/main/java/io/ray/serve/api/Serve.java
Original file line number Diff line number Diff line change
Expand Up @@ -297,8 +297,7 @@ public static Deployment getDeployment(String name) {
name,
deploymentRoute.getDeploymentInfo().getDeploymentConfig(),
deploymentRoute.getDeploymentInfo().getReplicaConfig(),
deploymentRoute.getDeploymentInfo().getVersion(),
deploymentRoute.getRoute());
deploymentRoute.getDeploymentInfo().getVersion());
}

/**
Expand All @@ -307,7 +306,7 @@ public static Deployment getDeployment(String name) {
* @param target A Serve application returned by `Deployment.bind()`.
* @return A handle that can be used to call the application.
*/
public static Optional<DeploymentHandle> run(Application target) {
public static DeploymentHandle run(Application target) {
return run(target, true, Constants.SERVE_DEFAULT_APP_NAME, null, null);
}

Expand All @@ -318,13 +317,11 @@ public static Optional<DeploymentHandle> run(Application target) {
* @param blocking
* @param name Application name. If not provided, this will be the only application running on the
* cluster (it will delete all others).
* @param routePrefix Route prefix for HTTP requests. If not provided, it will use route_prefix of
* the ingress deployment. If specified neither as an argument nor in the ingress deployment,
* the route prefix will default to '/'.
* @param routePrefix Route prefix for HTTP requests. Defaults to '/'.
* @param config
* @return A handle that can be used to call the application.
*/
public static Optional<DeploymentHandle> run(
public static DeploymentHandle run(
Application target,
boolean blocking,
String name,
Expand All @@ -335,19 +332,20 @@ public static Optional<DeploymentHandle> run(
throw new RayServeException("Application name must a non-empty string.");
}

if (StringUtils.isNotBlank(routePrefix)) {
Preconditions.checkArgument(
routePrefix.startsWith("/"), "The route_prefix must start with a forward slash ('/')");
} else {
routePrefix = "/";
}

ServeControllerClient client = serveStart(config);

List<Deployment> deployments = Graph.build(target.getInternalDagNode(), name);
Deployment ingress = Graph.getAndValidateIngressDeployment(deployments);
Deployment ingressDeployment = deployments.get(deployments.size() - 1);
edoakes marked this conversation as resolved.
Show resolved Hide resolved

for (Deployment deployment : deployments) {
// Overwrite route prefix
if (StringUtils.isNotBlank(deployment.getRoutePrefix())
&& StringUtils.isNotBlank(routePrefix)) {
Preconditions.checkArgument(
routePrefix.startsWith("/"), "The route_prefix must start with a forward slash ('/')");
deployment.setRoutePrefix(routePrefix);
}
deployment
.getDeploymentConfig()
.setVersion(
Expand All @@ -356,12 +354,8 @@ public static Optional<DeploymentHandle> run(
: RandomStringUtils.randomAlphabetic(6));
}

client.deployApplication(name, deployments, blocking);

return Optional.ofNullable(ingress)
.map(
ingressDeployment ->
client.getDeploymentHandle(ingressDeployment.getName(), name, true));
client.deployApplication(name, routePrefix, deployments, ingressDeployment.getName(), blocking);
return client.getDeploymentHandle(ingressDeployment.getName(), name, true);
}

private static void init() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,11 +160,19 @@ public BaseActorHandle getController() {
/**
* Deployment an application with deployment list.
*
* @param name application name
* @param deployments deployment list
* @param name application name.
* @param routePrefix route prefix for the application.
* @param deployments deployment list.
* @param ingressDeploymentName name of the ingress deployment (the one that is exposed over
* HTTP).
* @param blocking Wait for the applications to be deployed or not.
*/
public void deployApplication(String name, List<Deployment> deployments, boolean blocking) {
public void deployApplication(
String name,
String routePrefix,
List<Deployment> deployments,
String ingressDeploymentName,
boolean blocking) {

Object[] deploymentArgsArray = new Object[deployments.size()];

Expand All @@ -178,8 +186,8 @@ public void deployApplication(String name, List<Deployment> deployments, boolean
ByteString.copyFrom(deployment.getDeploymentConfig().toProtoBytes()))
.setIngress(deployment.isIngress())
.setDeployerJobId(Ray.getRuntimeContext().getCurrentJobId().toString());
if (deployment.getRoutePrefix() != null) {
deploymentArgs.setRoutePrefix(deployment.getRoutePrefix());
if (deployment.getName() == ingressDeploymentName) {
deploymentArgs.setRoutePrefix(routePrefix);
}
deploymentArgsArray[i] = deploymentArgs.build().toByteArray();
}
Expand All @@ -195,7 +203,6 @@ public void deployApplication(String name, List<Deployment> deployments, boolean
logDeploymentReady(
deployment.getName(),
deployment.getVersion(),
deployment.getUrl(),
"component=serve deployment=" + deployment.getName());
}
}
Expand Down Expand Up @@ -238,13 +245,11 @@ private void waitForApplicationRunning(String name, Long timeoutS) {
"Application {} did not become RUNNING after {}s.", name, timeoutS));
}

private void logDeploymentReady(String name, String version, String url, String tag) {
String urlPart = url != null ? MessageFormatter.format(" at `{}`", url) : "";
private void logDeploymentReady(String name, String version, String tag) {
LOGGER.info(
"Deployment '{}{}' is ready {}. {}",
"Deployment '{}{}' is ready. {}",
name,
StringUtils.isNotBlank(version) ? "':'" + version : "",
urlPart,
tag);
}

Expand Down
63 changes: 1 addition & 62 deletions java/serve/src/main/java/io/ray/serve/dag/Graph.java
Original file line number Diff line number Diff line change
@@ -1,13 +1,9 @@
package io.ray.serve.dag;

import com.google.common.base.Preconditions;
import io.ray.serve.deployment.Deployment;
import io.ray.serve.handle.DeploymentHandle;
import io.ray.serve.util.CollectionUtil;
import io.ray.serve.util.CommonUtil;
import io.ray.serve.util.DAGUtil;
import io.ray.serve.util.MessageFormatter;
import java.util.ArrayList;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
Expand All @@ -19,37 +15,7 @@ public class Graph {
public static List<Deployment> build(DAGNode rayDagRootNode, String name) {
DAGNodeBase serveRootDag =
rayDagRootNode.applyRecursive(node -> transformRayDagToServeDag(node, name));
List<Deployment> deployments = extractDeployments(serveRootDag);
List<Deployment> deploymentsWithHttp = processIngressDeploymentInServeDag(deployments);
return deploymentsWithHttp;
}

private static List<Deployment> processIngressDeploymentInServeDag(List<Deployment> deployments) {
if (CollectionUtil.isEmpty(deployments)) {
return deployments;
}

Deployment ingressDeployment = deployments.get(deployments.size() - 1);
if (StringUtils.isBlank(ingressDeployment.getRoutePrefix())
|| StringUtils.equals(
ingressDeployment.getRoutePrefix(), "/" + ingressDeployment.getName())) {
ingressDeployment.setRoutePrefix("/");
}

for (int i = 0; i < deployments.size() - 1; i++) {
Deployment deployment = deployments.get(i);
Preconditions.checkArgument(
StringUtils.isBlank(deployment.getRoutePrefix())
|| StringUtils.equals(deployment.getRoutePrefix(), "/" + deployment.getName()),
MessageFormatter.format(
"Route prefix is only configurable on the ingress deployment. "
+ "Please do not set non-default route prefix: "
+ "{} on non-ingress deployment of the "
+ "serve DAG. ",
deployment.getRoutePrefix()));
deployment.setRoutePrefix(null);
}
return deployments;
return extractDeployments(serveRootDag);
}

public static DAGNodeBase transformRayDagToServeDag(DAGNodeBase dagNode, String appName) {
Expand All @@ -64,12 +30,6 @@ public static DAGNodeBase transformRayDagToServeDag(DAGNodeBase dagNode, String
deploymentName = deploymentShell.getName();
}

String routePrefix =
StringUtils.isBlank(deploymentShell.getRoutePrefix())
|| !StringUtils.equals(deploymentShell.getRoutePrefix(), "/" + deploymentName)
? deploymentShell.getRoutePrefix()
: "/" + deploymentName;

Object[] replacedDeploymentInitArgs = new Object[clsNode.getBoundArgs().length];
for (int i = 0; i < clsNode.getBoundArgs().length; i++) {
replacedDeploymentInitArgs[i] =
Expand All @@ -84,7 +44,6 @@ public static DAGNodeBase transformRayDagToServeDag(DAGNodeBase dagNode, String
.setDeploymentDef(clsNode.getClassName())
.setName(deploymentName)
.setInitArgs(replacedDeploymentInitArgs)
.setRoutePrefix(routePrefix)
.create(false);

return new DeploymentNode(
Expand All @@ -111,26 +70,6 @@ public static List<Deployment> extractDeployments(DAGNodeBase rootNode) {
return deployments.values().stream().collect(Collectors.toList());
}

public static Deployment getAndValidateIngressDeployment(List<Deployment> deployments) {

List<Deployment> ingressDeployments = new ArrayList<>();
for (Deployment deployment : deployments) {
if (StringUtils.isNotBlank(deployment.getRoutePrefix())) {
ingressDeployments.add(deployment);
}
}

Preconditions.checkArgument(
ingressDeployments.size() == 1,
MessageFormatter.format(
"Only one deployment in an Serve Application or DAG can have non-None route prefix. {} ingress deployments found: {}",
ingressDeployments.size(),
ingressDeployments));

ingressDeployments.get(0).setIngress(true);
return ingressDeployments.get(0);
}

public static DeploymentHandle replaceWithHandle(DAGNode node) {
if (node instanceof DeploymentNode) {
DeploymentNode deploymentNode = (DeploymentNode) node;
Expand Down
36 changes: 1 addition & 35 deletions java/serve/src/main/java/io/ray/serve/deployment/Deployment.java
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
import io.ray.serve.handle.DeploymentHandle;
import java.util.HashMap;
import java.util.Map;
import org.apache.commons.lang3.StringUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -32,30 +31,12 @@ public class Deployment {

private final String version;

private String routePrefix;

private final String url;

private boolean ingress;

// TODO support placement group.

public Deployment(
String name,
DeploymentConfig deploymentConfig,
ReplicaConfig replicaConfig,
String version,
String routePrefix) {

if (StringUtils.isNotBlank(routePrefix)) {
Preconditions.checkArgument(routePrefix.startsWith("/"), "route_prefix must start with '/'.");
Preconditions.checkArgument(
routePrefix.equals("/") || !routePrefix.endsWith("/"),
"route_prefix must not end with '/' unless it's the root.");
Preconditions.checkArgument(
!routePrefix.contains("{") && !routePrefix.contains("}"),
"route_prefix may not contain wildcards.");
}
String name, DeploymentConfig deploymentConfig, ReplicaConfig replicaConfig, String version) {

Preconditions.checkArgument(
version != null || deploymentConfig.getAutoscalingConfig() == null,
Expand All @@ -65,8 +46,6 @@ public Deployment(
this.version = version;
this.deploymentConfig = deploymentConfig;
this.replicaConfig = replicaConfig;
this.routePrefix = routePrefix;
this.url = routePrefix != null ? Serve.getGlobalClient().getRootUrl() + routePrefix : null;
}

/**
Expand Down Expand Up @@ -97,7 +76,6 @@ public DeploymentCreator options() {
.setVersion(this.version)
.setNumReplicas(this.deploymentConfig.getNumReplicas())
.setInitArgs(this.replicaConfig.getInitArgs())
.setRoutePrefix(this.routePrefix)
.setRayActorOptions(this.replicaConfig.getRayActorOptions())
.setUserConfig(this.deploymentConfig.getUserConfig())
.setMaxOngoingRequests(this.deploymentConfig.getMaxOngoingRequests())
Expand Down Expand Up @@ -147,18 +125,6 @@ public String getVersion() {
return version;
}

public String getRoutePrefix() {
return routePrefix;
}

public String getUrl() {
return url;
}

public void setRoutePrefix(String routePrefix) {
this.routePrefix = routePrefix;
}

public boolean isIngress() {
return ingress;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,16 +43,6 @@ public class DeploymentCreator {
*/
private Object[] initArgs;

/**
* Requests to paths under this HTTP path prefix will be routed to this deployment. Defaults to
* '/{name}'. When set to 'None', no HTTP endpoint will be created. Routing is done based on
* longest-prefix match, so if you have deployment A with a prefix of '/a' and deployment B with a
* prefix of '/a/b', requests to '/a', '/a/', and '/a/c' go to A and requests to '/a/b', '/a/b/',
* and '/a/b/c' go to B. Routes must not end with a '/' unless they're the root (just '/'), which
* acts as a catch-all.
*/
@Deprecated private String routePrefix;

/** Options to be passed to the Ray actor constructor such as resource requirements. */
private Map<String, Object> rayActorOptions;

Expand Down Expand Up @@ -97,10 +87,6 @@ public Deployment create(boolean check) {
LOGGER.warn(
"DeprecationWarning: `version` in `@serve.deployment` has been deprecated. Explicitly specifying version will raise an error in the future!");
}
if (routePrefix != null) {
LOGGER.warn(
"DeprecationWarning: `route_prefix` in `@serve.deployment` has been deprecated. To specify a route prefix for an application, pass it into `serve.run` instead.");
}

DeploymentConfig deploymentConfig =
new DeploymentConfig()
Expand All @@ -120,8 +106,7 @@ public Deployment create(boolean check) {
StringUtils.isNotBlank(name) ? name : CommonUtil.getDeploymentName(deploymentDef),
deploymentConfig,
replicaConfig,
version,
routePrefix);
version);
}

public Deployment create() {
Expand Down Expand Up @@ -177,15 +162,6 @@ public DeploymentCreator setInitArgs(Object[] initArgs) {
return this;
}

public String getRoutePrefix() {
return routePrefix;
}

public DeploymentCreator setRoutePrefix(String routePrefix) {
this.routePrefix = routePrefix;
return this;
}

public Map<String, Object> getRayActorOptions() {
return rayActorOptions;
}
Expand Down
Loading