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] Use Async Handle for DAG Execution #27411

Merged
merged 22 commits into from Aug 7, 2022
Merged

[Serve] Use Async Handle for DAG Execution #27411

merged 22 commits into from Aug 7, 2022

Conversation

simon-mo
Copy link
Contributor

@simon-mo simon-mo commented Aug 3, 2022 •
edited

Signed-off-by: simon-mo simon.mo@hey.com

Why are these changes needed?

This PR replaces the sync handle usage in deployment graph with async handle. It uses the following method

  • Change async handle's .remote return value as a asyncio.Task instead of a coroutine. This doesn't change the UX ref = await handle.remote() but it allows the result to be awaited twice by DAG code.
  • DAG still synchronously call .execute and get the asyncio.Task back.
  • The tasks are propagated directly to downstream's .remote call.
  • In the router's execution path, we use PyObjScanner to find all occurrence of asyncio.Task and resolve them in batch.
  • Generalized PyObjScanner to handle arbitrary source type for replacement.
  • Improved performance of PyObjScanner so it will not serialize input objects.

This PR is code complete and ready for review. Before merge, I'll update the documentation and example code. Add more tests.

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

@simon-mo
wip async handle for dag execution
f307797 
Signed-off-by: simon-mo <simon.mo@hey.com>
@simon-mo
allow gather
89772db 
Signed-off-by: simon-mo <simon.mo@hey.com>
@simon-mo
wip
0ad1f66 
Signed-off-by: simon-mo <simon.mo@hey.com>
@simon-mo
handle multiple await
1518f20 
Signed-off-by: simon-mo <simon.mo@hey.com>
@simon-mo
unused import
e67cc13 
Signed-off-by: simon-mo <simon.mo@hey.com>
@simon-mo
Merge branch 'master' of github.com:ray-project/ray into serve/async-dag
fa0bb37 
Signed-off-by: simon-mo <simon.mo@hey.com>
@simon-mo
comments
4429d54 
Signed-off-by: simon-mo <simon.mo@hey.com>
@simon-mo simon-mo changed the title wip async handle for dag execution [Serve] Use Async Handle for DAG Execution Aug 4, 2022
@simon-mo simon-mo marked this pull request as ready for review August 4, 2022 03:22
@simon-mo
lint
ed219a6 
Signed-off-by: simon-mo <simon.mo@hey.com>
@simon-mo
new line
53748f0 
Signed-off-by: simon-mo <simon.mo@hey.com>
sihanwang41
sihanwang41 reviewed Aug 4, 2022
View reviewed changes
python/ray/dag/py_obj_scanner.py Outdated
to internal data structures, preventing actually writing them to
the buffer.
"""
if obj is _get_object or obj is _get_object:
Copy link
Contributor

Choose a reason for hiding this comment

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

duplicate

@simon-mo
fix
94c5c5f 
Signed-off-by: simon-mo <simon.mo@hey.com>
@simon-mo
Copy link
Contributor Author

simon-mo commented Aug 4, 2022

cc @iycheng @jiaodong @suquark for PyObjScanner changes.

sihanwang41
sihanwang41 reviewed Aug 4, 2022
View reviewed changes
python/ray/dag/py_obj_scanner.py
index = len(self._found)
self._found.append(obj)
return _get_node, (id(self), index)
else:
index = len(self._objects)
self._objects.append(obj)
Copy link
Contributor

Choose a reason for hiding this comment

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

how do we do the dict object to do append?

@simon-mo
change objects to list again
644dbaa 
Signed-off-by: simon-mo <simon.mo@hey.com>
@simon-mo
fix list
2850bb9 
Signed-off-by: simon-mo <simon.mo@hey.com>
edoakes
edoakes approved these changes Aug 4, 2022
View reviewed changes
Copy link
Contributor

@edoakes edoakes left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for adding unit tests to the PyObjScanner, that class makes me a bit nervous in general.

Only concern is exposing this RayServeLazyAsyncHandle as a user-facing class... if we can fix that for the release, great, but not a complete dealbreaker.

python/ray/dag/py_obj_scanner.py Outdated
self._found = None
# List of other objects found during the serializatoin pass.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# List of other objects found during the serializatoin pass.
# List of other objects found during the serialization pass.

@simon-mo
Copy link
Contributor Author

simon-mo commented Aug 4, 2022

microbenchmark seems to be normal range.
before
after
percent change:

0.13
-2.24
-1.68
0.52
2.43
1.61
1.24
0.36
2.51
-4.45
-4.46
-0.00
3.67
0.93
3.99
-3.16
1.95
-4.82
-0.45
-1.26
2.91
-1.53
0.60
-4.42

@simon-mo
minor grammar
bf7b589 
Signed-off-by: simon-mo <simon.mo@hey.com>
@simon-mo
wip
f9e5441 
Signed-off-by: simon-mo <simon.mo@hey.com>
@fishbone fishbone self-assigned this Aug 5, 2022
@simon-mo
wip
9514826 
Signed-off-by: simon-mo <simon.mo@hey.com>
@simon-mo
pass tests!
3c74dde 
Signed-off-by: simon-mo <simon.mo@hey.com>
@simon-mo
rename RayServeLazyAsyncHandle -> RayServeDeploymentHandle
564aeb9 
Signed-off-by: simon-mo <simon.mo@hey.com>
@simon-mo
feature flag old behavior as escape hatch
862ac79 
Signed-off-by: simon-mo <simon.mo@hey.com>
@simon-mo
lint
370166f 
Signed-off-by: simon-mo <simon.mo@hey.com>
@simon-mo simon-mo requested a review from jiaodong as a code owner August 6, 2022 20:44
@simon-mo simon-mo requested review from shrekris-anyscale and a team as code owners August 6, 2022 20:44
@simon-mo
split the doc change
aaf7957 
Signed-off-by: simon-mo <simon.mo@hey.com>
@simon-mo simon-mo removed the request for review from a team August 6, 2022 20:47
@simon-mo
Merge branch 'master' of github.com:ray-project/ray into serve/async-dag
1636da9 
Signed-off-by: simon-mo <simon.mo@hey.com>
@simon-mo
fix tests
c401823 
Signed-off-by: simon-mo <simon.mo@hey.com>
@simon-mo simon-mo added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Aug 7, 2022
@simon-mo simon-mo merged commit efee158 into ray-project:master Aug 7, 2022
simon-mo added a commit to simon-mo/ray that referenced this pull request Aug 7, 2022
@simon-mo
[Serve] Use Async Handle for DAG Execution (ray-project#27411)
edab36a 
Signed-off-by: simon-mo <simon.mo@hey.com>
@fishbone fishbone mentioned this pull request Aug 9, 2022
Closed
@sihanwang41 sihanwang41 mentioned this pull request Aug 12, 2022
Merged
7 tasks
Stefan-1313 pushed a commit to Stefan-1313/ray_mod that referenced this pull request Aug 18, 2022
@simon-mo
[Serve] Use Async Handle for DAG Execution (ray-project#27411)
f795667 
Signed-off-by: Stefan van der Kleij <s.vanderkleij@viroteq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sihanwang41 sihanwang41 sihanwang41 left review comments

@edoakes edoakes edoakes approved these changes

@jiaodong jiaodong Awaiting requested review from jiaodong

@shrekris-anyscale shrekris-anyscale Awaiting requested review from shrekris-anyscale shrekris-anyscale is a code owner

Assignees

@sihanwang41 sihanwang41

@edoakes edoakes

@fishbone fishbone

Labels
tests-ok The tagger certifies test failures are unrelated and assumes personal liability.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@simon-mo @sihanwang41 @edoakes @fishbone