[serve] Remove ability to specify `route_prefix` at deployment level #48223

edoakes · 2024-10-23T19:13:08Z

Why are these changes needed?

Removes the ability to specify route_prefix at the deployment level (which has been deprecated for a number of releases).

Also rips out some vestigial code which was unused:

Deployment._deploy
log_deployment_update_status
get_deployment

Related issue number

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
- I've added any new APIs to the API Reference. For example, if I added a
  method in Tune, I've added it in doc/source/tune/api/ under the
  corresponding .rst file.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

…ve-deprecations

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

zcin

nice!

zcin · 2024-10-25T16:14:45Z

python/ray/serve/deployment.py

@@ -479,7 +419,6 @@ def options(
            new_deployment_config,
            new_replica_config,
            version=version,


should we remove version too, or does that require more code changes

we should, but I'd rather do it in a separate PR at least

this already got bigger than I wanted 😓

zcin · 2024-10-25T16:18:09Z

python/ray/serve/scripts.py

        schema = ServeApplicationSchema(
            name=name,
-            route_prefix=ingress.route_prefix,
+            route_prefix="/",


Suggested change

route_prefix="/",

route_prefix=f"/{name}",

I think this will need to be a distinct generated route prefix for each application because the pydantic model will fail to parse if every route prefix is "/".

oh you're right. don't we default it to /{name} somewhere else already?

Ok I changed it to do /{name} if there are multiple apps, else just /

python/ray/serve/tests/test_deployment_graph_ingress.py

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

GeneDer

LGTM! Kinda feel this is something Alex would have picked up for onboarding XP

java/serve/src/main/java/io/ray/serve/api/Serve.java

python/ray/serve/_private/deployment_graph_build.py

python/ray/serve/_private/client.py

python/ray/serve/deployment.py

python/ray/serve/tests/test_http_routes.py

…ve-deprecations

zcin · 2024-10-26T02:33:04Z

python/ray/serve/api.py



 @PublicAPI(stability="stable")
 def run(
    target: Application,
    blocking: bool = False,
    name: str = SERVE_DEFAULT_APP_NAME,
-    route_prefix: Optional[str] = DEFAULT.VALUE,
+    route_prefix: Optional[str] = "/",


Is there a reason why route_prefix had to default to DEFAULT.VALUE instead of "/" before?

None means "no route prefix" and previously we wanted to pick up the route_prefix of the ingress deployment if nothing was specified here to override

…ve-deprecations

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

…t level" (#48292) Reverts #48223 Broke #48289 so revert to unblock the release.

Stacked on: #48223 Reimplements the `.bind()` codepath to avoid using the Ray DAG codepath which adds a lot of complexity for little benefit. The DAG traversal code is much simpler, around 100 lines of self-contained code in `build_app.py`. I've also added unit tests for it. There should be no behavior change here. --------- Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

…ay-project#48223) Removes the ability to specify `route_prefix` at the deployment level (which has been deprecated for a number of releases). Also rips out some vestigial code which was unused: - `Deployment._deploy` - `log_deployment_update_status` - `get_deployment` Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

…t level" (ray-project#48292) Reverts ray-project#48223 Broke ray-project#48289 so revert to unblock the release.

Stacked on: ray-project#48223 Reimplements the `.bind()` codepath to avoid using the Ray DAG codepath which adds a lot of complexity for little benefit. The DAG traversal code is much simpler, around 100 lines of self-contained code in `build_app.py`. I've also added unit tests for it. There should be no behavior change here. --------- Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

…ay-project#48223) Removes the ability to specify `route_prefix` at the deployment level (which has been deprecated for a number of releases). Also rips out some vestigial code which was unused: - `Deployment._deploy` - `log_deployment_update_status` - `get_deployment` Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

…t level" (ray-project#48292) Reverts ray-project#48223 Broke ray-project#48289 so revert to unblock the release. Signed-off-by: JP-sDEV <jon.pablo80@gmail.com>

Stacked on: ray-project#48223 Reimplements the `.bind()` codepath to avoid using the Ray DAG codepath which adds a lot of complexity for little benefit. The DAG traversal code is much simpler, around 100 lines of self-contained code in `build_app.py`. I've also added unit tests for it. There should be no behavior change here. --------- Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

…ay-project#48223) Removes the ability to specify `route_prefix` at the deployment level (which has been deprecated for a number of releases). Also rips out some vestigial code which was unused: - `Deployment._deploy` - `log_deployment_update_status` - `get_deployment` Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com> Signed-off-by: mohitjain2504 <mohit.jain@dream11.com>

…t level" (ray-project#48292) Reverts ray-project#48223 Broke ray-project#48289 so revert to unblock the release. Signed-off-by: mohitjain2504 <mohit.jain@dream11.com>

Stacked on: ray-project#48223 Reimplements the `.bind()` codepath to avoid using the Ray DAG codepath which adds a lot of complexity for little benefit. The DAG traversal code is much simpler, around 100 lines of self-contained code in `build_app.py`. I've also added unit tests for it. There should be no behavior change here. --------- Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com> Signed-off-by: mohitjain2504 <mohit.jain@dream11.com>

fix

7005040

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

edoakes added the go add ONLY when ready to merge, run all tests label Oct 23, 2024

edoakes added 7 commits October 25, 2024 08:22

fix

8503082

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

fix

52eace7

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

fix

da37b9a

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

more fixes

f206f83

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

more fixes

749c9f7

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

more fixes

ac79021

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

more fixes

fa66535

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

edoakes marked this pull request as ready for review October 25, 2024 14:02

edoakes changed the title ~~[WIP][serve] Remove route_prefix at deployment level~~ [serve] Remove ability to specify route_prefix at deployment level Oct 25, 2024

edoakes added 3 commits October 25, 2024 09:02

Merge branch 'master' of https://github.com/ray-project/ray into remo…

55abba0

…ve-deprecations

fix

11eaf12

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

fix

a2b3e9b

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

edoakes mentioned this pull request Oct 25, 2024

[serve] Simplify application build codepath #48218

Merged

8 tasks

edoakes added 4 commits October 25, 2024 09:19

fix

dadc536

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

fix

8d32f64

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

fix

f22e6c3

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

fix

3c8ac47

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

edoakes requested review from zcin and GeneDer October 25, 2024 15:49

edoakes added 3 commits October 25, 2024 10:58

fix

abcc572

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

fix

16f2c28

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

fix

05f6d29

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

zcin reviewed Oct 25, 2024

View reviewed changes

edoakes added 4 commits October 25, 2024 11:32

fix

b35cf7a

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

more fixes

2d97896

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

fix

b3c561b

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

fix

6173940

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

edoakes requested a review from shrekris-anyscale as a code owner October 25, 2024 18:14

edoakes added 6 commits October 25, 2024 13:30

more fixes

87453d2

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

more fixes

2171af5

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

more fixes

d7f892c

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

fix java

df3ecef

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

fix

d48441e

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

fix

93c181f

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

edoakes self-assigned this Oct 25, 2024

edoakes requested a review from zcin October 25, 2024 20:44

GeneDer approved these changes Oct 26, 2024

View reviewed changes

Merge branch 'master' of https://github.com/ray-project/ray into remo…

fbec568

…ve-deprecations

zcin approved these changes Oct 26, 2024

View reviewed changes

edoakes added 6 commits October 26, 2024 07:58

Merge branch 'master' of https://github.com/ray-project/ray into remo…

e1686d6

…ve-deprecations

fix

dabeca7

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

fix

c339024

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

fix

1fdc44b

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

more fixes

3487b1f

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

fix

02d9f63

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

edoakes merged commit 90e6e72 into ray-project:master Oct 26, 2024
5 checks passed

can-anyscale mentioned this pull request Oct 28, 2024

Revert "[serve] Remove ability to specify route_prefix at deployment level" #48292

Merged

can-anyscale added a commit that referenced this pull request Oct 28, 2024

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

703194a

…t level" (#48292) Reverts #48223 Broke #48289 so revert to unblock the release.

Jay-ju pushed a commit to Jay-ju/ray that referenced this pull request Nov 5, 2024

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

7ad3ecc

…t level" (ray-project#48292) Reverts ray-project#48223 Broke ray-project#48289 so revert to unblock the release.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[serve] Remove ability to specify `route_prefix` at deployment level #48223

[serve] Remove ability to specify `route_prefix` at deployment level #48223

edoakes commented Oct 23, 2024 •

edited

Loading

zcin left a comment

zcin Oct 25, 2024

edoakes Oct 25, 2024

edoakes Oct 25, 2024

zcin Oct 25, 2024

edoakes Oct 25, 2024

edoakes Oct 25, 2024

zcin Oct 26, 2024

GeneDer left a comment

zcin Oct 26, 2024

edoakes Oct 26, 2024

[serve] Remove ability to specify route_prefix at deployment level #48223

[serve] Remove ability to specify route_prefix at deployment level #48223

Conversation

edoakes commented Oct 23, 2024 • edited Loading

Why are these changes needed?

Related issue number

Checks

zcin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeneDer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[serve] Remove ability to specify `route_prefix` at deployment level #48223

[serve] Remove ability to specify `route_prefix` at deployment level #48223

edoakes commented Oct 23, 2024 •

edited

Loading