Skip to content

[serve] Add multi-app support to serve deploy - #33013

Merged
edoakes merged 8 commits into
ray-project:masterfrom
zcin:deploy-cli
Mar 10, 2023
Merged

edoakes merged 8 commits into
ray-project:masterfrom
zcin:deploy-cli

Conversation

@zcin

@zcin zcin commented Mar 3, 2023

Copy link
Copy Markdown
Contributor

Why are these changes needed?

Updated serve deploy to accept both

  • The original ServeApplicationSchema for deploying a single application
  • The new ServeDeploySchema for deploying multiple applications

Details:

  • ServeApplicationSchema and ServeDeploySchema have different required fields, so there is no ambiguity - one config cannot be successfully parsed as both.
  • If a config fails to parse as either of them, the validation error from trying to parse as ServeApplicationSchema will be surfaced.
  • ServeDeploySchema will go through /api/serve/applications and ServeApplicationSchema will go through /api/serve/deployments

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 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: Cindy Zhang <cindyzyx9@gmail.com>
@zcin
zcin marked this pull request as ready for review March 4, 2023 00:06
zcin added 3 commits March 7, 2023 08:29
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>

@edoakes edoakes left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@zcin can we add a test for multiple apps with different runtime_envs?

@sihanwang41

Copy link
Copy Markdown
Contributor

Hi Cindy, let's add some failure case in the unit tests:

  • Same app name.
  • Same route_prefix.
  • One application failed to deploy and the other one is good.

zcin added 2 commits March 8, 2023 13:37
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
@zcin

zcin commented Mar 8, 2023

Copy link
Copy Markdown
Contributor Author

@zcin can we add a test for multiple apps with different runtime_envs?

@edoakes Added! I added a test at the python API level instead of adding it as a cli test.

Hi Cindy, let's add some failure case in the unit tests:

  • Same app name.
  • Same route_prefix.
  • One application failed to deploy and the other one is good.

@sihanwang41 I added a unit test for one app that succeeds and one that fails (also a python API test instead of cli test). For the other two, I believe those should be Pydantic validation errors after this PR is merged.

@sihanwang41

sihanwang41 commented Mar 8, 2023

Copy link
Copy Markdown
Contributor

@zcin can we add a test for multiple apps with different runtime_envs?

@edoakes Added! I added a test at the python API level instead of adding it as a cli test.

Hi Cindy, let's add some failure case in the unit tests:

  • Same app name.
  • Same route_prefix.
  • One application failed to deploy and the other one is good.

@sihanwang41 I added a unit test for one app that succeeds and one that fails (also a python API test instead of cli test). For the other two, I believe those should be Pydantic validation errors after this PR is merged.

For the REST/CLI perspective, let's add those tests and assert validation errors always happening ? :) (make sure we show the error message clearly to the caller )

@zcin

zcin commented Mar 8, 2023

Copy link
Copy Markdown
Contributor Author

For the REST/CLI perspective, let's add those tests and assert validation errors always happening ? :) (make sure we show the error message clearly to the caller )

Sounds good, I'll add those after #32787 is merged.

zcin added 2 commits March 10, 2023 10:56
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
@zcin

zcin commented Mar 10, 2023

Copy link
Copy Markdown
Contributor Author

@sihanwang41 Added tests for conflicting app name and conflicting route prefix.

@zcin

zcin commented Mar 10, 2023

Copy link
Copy Markdown
Contributor Author

@edoakes ready for merge?

@edoakes
edoakes merged commit e1b7b4f into ray-project:master Mar 10, 2023
@zcin
zcin deleted the deploy-cli branch March 10, 2023 22:52
ProjectsByJackHe pushed a commit to ProjectsByJackHe/ray that referenced this pull request Mar 21, 2023
Updated `serve deploy` to accept both
- The original `ServeApplicationSchema` for deploying a single application
- The new `ServeDeploySchema` for deploying multiple applications

Details:
- `ServeApplicationSchema` and `ServeDeploySchema` have different required fields, so there is no ambiguity - one config cannot be successfully parsed as both.
- If a config fails to parse as either of them, the validation error from trying to parse as `ServeApplicationSchema` will be surfaced.
- `ServeDeploySchema` will go through `/api/serve/applications` and `ServeApplicationSchema` will go through `/api/serve/deployments`

Signed-off-by: Jack He <jackhe2345@gmail.com>
edoakes pushed a commit to edoakes/ray that referenced this pull request Mar 22, 2023
Updated `serve deploy` to accept both
- The original `ServeApplicationSchema` for deploying a single application
- The new `ServeDeploySchema` for deploying multiple applications

Details:
- `ServeApplicationSchema` and `ServeDeploySchema` have different required fields, so there is no ambiguity - one config cannot be successfully parsed as both.
- If a config fails to parse as either of them, the validation error from trying to parse as `ServeApplicationSchema` will be surfaced.
- `ServeDeploySchema` will go through `/api/serve/applications` and `ServeApplicationSchema` will go through `/api/serve/deployments`

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
peytondmurray pushed a commit to peytondmurray/ray that referenced this pull request Mar 22, 2023
Updated `serve deploy` to accept both
- The original `ServeApplicationSchema` for deploying a single application
- The new `ServeDeploySchema` for deploying multiple applications

Details:
- `ServeApplicationSchema` and `ServeDeploySchema` have different required fields, so there is no ambiguity - one config cannot be successfully parsed as both.
- If a config fails to parse as either of them, the validation error from trying to parse as `ServeApplicationSchema` will be surfaced.
- `ServeDeploySchema` will go through `/api/serve/applications` and `ServeApplicationSchema` will go through `/api/serve/deployments`
elliottower pushed a commit to elliottower/ray that referenced this pull request Apr 22, 2023
Updated `serve deploy` to accept both
- The original `ServeApplicationSchema` for deploying a single application
- The new `ServeDeploySchema` for deploying multiple applications

Details:
- `ServeApplicationSchema` and `ServeDeploySchema` have different required fields, so there is no ambiguity - one config cannot be successfully parsed as both.
- If a config fails to parse as either of them, the validation error from trying to parse as `ServeApplicationSchema` will be surfaced.
- `ServeDeploySchema` will go through `/api/serve/applications` and `ServeApplicationSchema` will go through `/api/serve/deployments`

Signed-off-by: elliottower <elliot@elliottower.com>
ProjectsByJackHe pushed a commit to ProjectsByJackHe/ray that referenced this pull request May 4, 2023
Updated `serve deploy` to accept both
- The original `ServeApplicationSchema` for deploying a single application
- The new `ServeDeploySchema` for deploying multiple applications

Details:
- `ServeApplicationSchema` and `ServeDeploySchema` have different required fields, so there is no ambiguity - one config cannot be successfully parsed as both.
- If a config fails to parse as either of them, the validation error from trying to parse as `ServeApplicationSchema` will be surfaced.
- `ServeDeploySchema` will go through `/api/serve/applications` and `ServeApplicationSchema` will go through `/api/serve/deployments`

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants