Skip to content

[serve] Improve single-app and multi-app differentiation#33490

Merged
edoakes merged 11 commits into
ray-project:masterfrom
zcin:deploy-mode-cli
Mar 23, 2023
Merged

[serve] Improve single-app and multi-app differentiation#33490
edoakes merged 11 commits into
ray-project:masterfrom
zcin:deploy-mode-cli

Conversation

@zcin
Copy link
Copy Markdown
Contributor

@zcin zcin commented Mar 21, 2023 •
edited
Loading

Why are these changes needed?

  • Disallow empty string application names in ServeDeploySchema
  • Disallow specifying an app name if user is directly deploying a single-app config ServeApplicationSchema
    • Otherwise, GET /api/serve/deployments/status and GET /api/serve/deployments/config would not get the correct status and config. We could try to fix it, but there's no reason to allow users to specify the name of their application, so it's better to disallow it.
  • Add a field deploy_mode in ServeInstanceDetails so that we can automatically detect what config(s) to show and REMOVE --multi-app flag in serve config
  • Remove trailing space for serve config and serve status

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 :(

zcin added 2 commits March 20, 2023 17:20
@zcin
raise warning if there is a deploy mode mismatch in serve config
a0dc7c9 
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
@zcin
add cli tests
c71fbfd 
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
zcin added 3 commits March 20, 2023 20:24
@zcin
fix
688adaa 
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
@zcin
Merge branch 'master' into deploy-mode-cli
04b6f0a 
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
@zcin
fix
bdaa6ee 
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
@zcin zcin changed the title [serve] Improve serve config [serve] Multi-app general improvements Mar 21, 2023
@zcin zcin changed the title [serve] Multi-app general improvements [serve] Improve single-app and multi-app differentiation Mar 21, 2023
edoakes
edoakes reviewed Mar 21, 2023
View reviewed changes
Comment thread dashboard/modules/serve/serve_agent.py
Comment on lines +169 to +170
if "name" in config.dict(exclude_unset=True):
return Response(
Copy link
Copy Markdown
Collaborator

@edoakes edoakes Mar 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a logger.warning message as well for both of these

Copy link
Copy Markdown
Contributor Author

@zcin zcin Mar 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added!

@zcin
remove multi-app flag from serve config
f279a9b 
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
@zcin
Copy link
Copy Markdown
Contributor Author

zcin commented Mar 21, 2023

Please see updated description + code.

If we are OK with adding a field deploy_mode in ServeInstanceDetails, we can track whether a single-app or multi-app config has been deployed in the backend, and thus automatically decide what to show for serve config. So, I've removed the --multi-app flag in serve config.

zcin added 3 commits March 21, 2023 15:07
@zcin
better comment
9af93e8 
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
@zcin
simply serve config
bcffb10 
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
@zcin
Merge branch 'master' into deploy-mode-cli
b630610 
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
@zcin zcin force-pushed the deploy-mode-cli branch from c537a9d to 233d73a Compare March 21, 2023 23:10
@zcin
remove trailing space for serve config and status
9614a2a 
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
@zcin zcin force-pushed the deploy-mode-cli branch from 233d73a to 9614a2a Compare March 21, 2023 23:19
sihanwang41
sihanwang41 approved these changes Mar 22, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@sihanwang41 sihanwang41 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add test cases for name set in ServeApplicationSchema. Make sure the exception can be raised in serve_agent and deploy_apps.

@zcin
add tests
5a851cf 
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
@zcin zcin force-pushed the deploy-mode-cli branch from baa9b3a to 5a851cf Compare March 22, 2023 21:06
@zcin
Copy link
Copy Markdown
Contributor Author

zcin commented Mar 23, 2023

please add test cases for name set in ServeApplicationSchema. Make sure the exception can be raised in serve_agent and deploy_apps.

Added! @edoakes ping for review/merge

@edoakes edoakes merged commit ffa71ce into ray-project:master Mar 23, 2023
@zcin zcin mentioned this pull request Mar 24, 2023
Merged
8 tasks
elliottower pushed a commit to elliottower/ray that referenced this pull request Apr 22, 2023
@zcin @elliottower
[serve] Improve single-app and multi-app differentiation (ray-project…
ed5bc82 
…#33490)

- Disallow empty string application names in `ServeDeploySchema`
- Disallow specifying an app name if user is directly deploying a single-app config `ServeApplicationSchema`
  - Otherwise, `GET /api/serve/deployments/status` and `GET /api/serve/deployments/config` would not get the correct status and config. We could try to fix it, but there's no reason to allow users to specify the name of their application, so it's better to disallow it.
- Add a field `deploy_mode` in `ServeInstanceDetails` so that we can automatically detect what config(s) to show and ***REMOVE `--multi-app` flag in `serve config`***
- Remove trailing space for `serve config` and `serve status`

Signed-off-by: elliottower <elliot@elliottower.com>
ProjectsByJackHe pushed a commit to ProjectsByJackHe/ray that referenced this pull request May 4, 2023
@zcin @ProjectsByJackHe
[serve] Improve single-app and multi-app differentiation (ray-project…
e5cc390 
…#33490)

- Disallow empty string application names in `ServeDeploySchema`
- Disallow specifying an app name if user is directly deploying a single-app config `ServeApplicationSchema`
  - Otherwise, `GET /api/serve/deployments/status` and `GET /api/serve/deployments/config` would not get the correct status and config. We could try to fix it, but there's no reason to allow users to specify the name of their application, so it's better to disallow it.
- Add a field `deploy_mode` in `ServeInstanceDetails` so that we can automatically detect what config(s) to show and ***REMOVE `--multi-app` flag in `serve config`***
- Remove trailing space for `serve config` and `serve status`

Signed-off-by: Jack He <jackhe2345@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sihanwang41 sihanwang41 sihanwang41 approved these changes

@edoakes edoakes edoakes approved these changes

Assignees

@sihanwang41 sihanwang41

@edoakes edoakes

@shrekris-anyscale shrekris-anyscale

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zcin @sihanwang41 @edoakes @shrekris-anyscale