Revert "Revert "[serve] Remove ability to specify route_prefix at d…

…eployment level" (ray-project#48292)" (ray-project#48294) This reverts commit 703194a. The original commit caused `test_deploy_2.py` to become flaky. It appears this was a timing issue due to the 10s timeout set on the test. I've removed this, as it looks like a very old test case that is no longer relevant. --------- Signed-off-by: Edward Oakes <[email protected]>
miqdigital · Oct 29, 2024 · 3303614 · 3303614
1 parent 62525c0
commit 3303614
Show file tree

Hide file tree

Showing 40 changed files with 197 additions and 1,047 deletions.
diff --git a/doc/source/serve/doc_code/image_classifier_example.py b/doc/source/serve/doc_code/image_classifier_example.py
@@ -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__
 
 
@@ -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})

diff --git a/doc/source/serve/doc_code/translator_example.py b/doc/source/serve/doc_code/translator_example.py
@@ -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!"

diff --git a/java/serve/src/main/java/io/ray/serve/api/Serve.java b/java/serve/src/main/java/io/ray/serve/api/Serve.java
@@ -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());
   }
 
   /**
@@ -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);
   }
 
@@ -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,
@@ -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);
 
     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(
@@ -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() {

diff --git a/java/serve/src/main/java/io/ray/serve/api/ServeControllerClient.java b/java/serve/src/main/java/io/ray/serve/api/ServeControllerClient.java
@@ -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()];
 
@@ -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();
     }
@@ -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());
       }
     }
@@ -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);
   }
 

diff --git a/java/serve/src/main/java/io/ray/serve/dag/Graph.java b/java/serve/src/main/java/io/ray/serve/dag/Graph.java
@@ -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;
@@ -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) {
@@ -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] =
@@ -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(
@@ -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;

diff --git a/java/serve/src/main/java/io/ray/serve/deployment/Deployment.java b/java/serve/src/main/java/io/ray/serve/deployment/Deployment.java
@@ -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;
 
@@ -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,
@@ -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;
   }
 
   /**
@@ -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())
@@ -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;
   }

diff --git a/java/serve/src/main/java/io/ray/serve/deployment/DeploymentCreator.java b/java/serve/src/main/java/io/ray/serve/deployment/DeploymentCreator.java
@@ -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;
 
@@ -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()
@@ -120,8 +106,7 @@ public Deployment create(boolean check) {
         StringUtils.isNotBlank(name) ? name : CommonUtil.getDeploymentName(deploymentDef),
         deploymentConfig,
         replicaConfig,
-        version,
-        routePrefix);
+        version);
   }
 
   public Deployment create() {
@@ -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;
   }