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.

Sign up for GitHub

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

Jump to bottom

[serve] Simplify application build codepath #48218

Open
wants to merge 70 commits into
base: master
Choose a base branch
from
Open

[serve] Simplify application build codepath #48218

wants to merge 70 commits into from
+598 −609

Conversation

edoakes
Copy link
Contributor

@edoakes edoakes commented Oct 23, 2024 •
edited
Loading

Why are these changes needed?

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.

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

@edoakes edoakes added the go add ONLY when ready to merge, run all tests label Oct 23, 2024
@edoakes
fix
7005040 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
fix
8503082 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
fix
52eace7 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
fix
da37b9a 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
more fixes
f206f83 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
more fixes
749c9f7 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
more fixes
ac79021 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
more fixes
fa66535 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
Merge branch 'master' of https://github.com/ray-project/ray into remo…
55abba0 
…ve-deprecations
@edoakes
fix
11eaf12 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
fix
a2b3e9b 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
fix
dadc536 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
fix
8d32f64 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
fix
f22e6c3 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
fix
3c8ac47 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes edoakes force-pushed the reimplement-build branch 2 times, most recently from 182cdcd to 2bb5722 Compare October 25, 2024 15:58
@edoakes
fix
abcc572 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
fix
16f2c28 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
fix
05f6d29 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
fix
b35cf7a 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
more fixes
2d97896 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
fix
b3c561b 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
fix
6173940 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
fix
1f7b25e 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
fix
b8cff28 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
fix
3f346ac 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
Merge branch 'master' of https://github.com/ray-project/ray into reim…
32fb73c 
…plement-build
@edoakes
fix
1e6a6ef 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
more fixes
3c83866 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes edoakes requested review from zcin and GeneDer and removed request for zcin October 26, 2024 23:13
@edoakes edoakes marked this pull request as ready for review October 26, 2024 23:13
@edoakes edoakes requested a review from zcin October 26, 2024 23:15
@edoakes
fix
fcd61f8 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
fix
9cfb1a7 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
fix
ca921e8 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
Copy link
Contributor Author

edoakes commented Oct 26, 2024

NOTE: test_deploy_2.py::test_deployment_error_handling was failing with my changes because it was not actually checking the right thing previously. It was passing because a validation error was raised in the DAG building codepath, not in the controller. This meant that if bad actor options did make it to the controller, it was not handled gracefully (similar to #43888).

I've updated the logic in deployment_scheduler.py to handle the case where Ray complains during actor creation and verified that it's handled gracefully now.

@edoakes
fix
f6c64c2 
Signed-off-by: Edward Oakes <[email protected]>
@edoakes
fix
b741b5c 
Signed-off-by: Edward Oakes <[email protected]>
GeneDer
GeneDer approved these changes Oct 28, 2024
View reviewed changes
Copy link
Contributor

@GeneDer GeneDer left a comment

Choose a reason for hiding this comment

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

LGTM!

python/ray/serve/_private/build_app.py
"""Dictionary that uses id() for keys instead of hash().
This is necessary because Application objects aren't hashable and we want each
instance to map to a unique key.
Copy link
Contributor

Choose a reason for hiding this comment

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

Great comment! Kinda wondering if there will be collisions between those ids if used across nodes and will this ever be used across nodes? Maybe a safer implementation is just generate a random uuid in Application and implement __hash__ to return that id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this will never be used across nodes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but I agree that would probably be a safer solution overall

@edoakes
fix
c187cc5 
Signed-off-by: Edward Oakes <[email protected]>
edoakes
edoakes commented Oct 28, 2024
View reviewed changes
python/ray/serve/_private/api.py
def call_app_builder_with_args_if_necessary(
def call_user_app_builder_with_args_if_necessary(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed to clarify the difference between this and build_app

@edoakes
Copy link
Contributor Author

edoakes commented Oct 28, 2024

now blocked on: #48294

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GeneDer GeneDer GeneDer approved these changes

@zcin zcin Awaiting requested review from zcin

Assignees

@edoakes edoakes

Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@edoakes @GeneDer