-
Notifications
You must be signed in to change notification settings - Fork 6.5k
[serve] refactor _run_user_code #54103
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.
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
Conversation
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Refactors internal user code invocation to use asyncio.Future by default and introduces local_testing_mode
to preserve concurrent.Future behavior in local tests.
- Renamed and updated decorator to
_run_user_code
, removingasyncio.wrap_future
usage around calls. - Added
local_testing_mode
flag toUserCallableWrapper
and propagated it in the local testing path. - Removed redundant
asyncio.wrap_future
calls in variousReplica
methods.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
python/ray/serve/_private/replica.py | Replaced asyncio.wrap_future calls, updated decorator to _run_user_code , added local_testing_mode flag |
python/ray/serve/_private/local_testing_mode.py | Passed local_testing_mode=True when creating the replica wrapper in local tests |
Comments suppressed due to low confidence (3)
python/ray/serve/_private/replica.py:1220
- The new
local_testing_mode
parameter is required but has no default, yet not all instantiation sites were updated; consider providing a default ofFalse
or updating allUserCallableWrapper(...)
calls to pass this flag.
self._local_testing_mode = local_testing_mode
python/ray/serve/_private/replica.py:1250
- The docstring states the decorator always returns an
asyncio.Future
, but inlocal_testing_mode
it returns aconcurrent.futures.Future
. Update the documentation to reflect this conditional behavior.
"""
python/ray/serve/_private/local_testing_mode.py:73
- Consider adding or updating tests to verify both branches of the
local_testing_mode
flag (True vs. False) to ensure the wrapper returns the expected Future type in each mode.
local_testing_mode=True,
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
ping @akyang-anyscale @abrarsheikh for review, this one is pretty straightforward. |
## Why are these changes needed? Refactor `_run_user_code` so that it returns an asyncio.future instead of concurrent.futures.Future. This is exactly how we had it implemented originally. We changed `_run_user_code` to return a `concurrent.futures.Future` in order to support local testing mode because we need to access the `concurrent.futures.Future` for local testing mode (ref: ray-project#48449). However we can change it back and just add a condition for local testing mode. This will be needed for supporting running user code on the same event loop. --------- Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
## Why are these changes needed? Refactor `_run_user_code` so that it returns an asyncio.future instead of concurrent.futures.Future. This is exactly how we had it implemented originally. We changed `_run_user_code` to return a `concurrent.futures.Future` in order to support local testing mode because we need to access the `concurrent.futures.Future` for local testing mode (ref: ray-project#48449). However we can change it back and just add a condition for local testing mode. This will be needed for supporting running user code on the same event loop. --------- Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
## Why are these changes needed? Refactor `_run_user_code` so that it returns an asyncio.future instead of concurrent.futures.Future. This is exactly how we had it implemented originally. We changed `_run_user_code` to return a `concurrent.futures.Future` in order to support local testing mode because we need to access the `concurrent.futures.Future` for local testing mode (ref: #48449). However we can change it back and just add a condition for local testing mode. This will be needed for supporting running user code on the same event loop. --------- Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com> Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
Why are these changes needed?
Refactor
_run_user_code
so that it returns an asyncio.future instead of concurrent.futures.Future.This is exactly how we had it implemented originally. We changed
_run_user_code
to return aconcurrent.futures.Future
in order to support local testing mode because we need to access theconcurrent.futures.Future
for local testing mode (ref: #48449). However we can change it back and just add a condition for local testing mode.This will be needed for supporting running user code on the same event loop.