Skip to content

[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

Merged
merged 3 commits into from
Jun 26, 2025

Conversation

zcin
Copy link
Contributor

@zcin zcin commented Jun 25, 2025

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>
@Copilot Copilot AI review requested due to automatic review settings June 25, 2025 19:47
@zcin zcin requested a review from a team as a code owner June 25, 2025 19:47
Copy link
Contributor

@Copilot Copilot AI left a 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, removing asyncio.wrap_future usage around calls.
  • Added local_testing_mode flag to UserCallableWrapper and propagated it in the local testing path.
  • Removed redundant asyncio.wrap_future calls in various Replica 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 of False or updating all UserCallableWrapper(...) 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 in local_testing_mode it returns a concurrent.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>
@zcin zcin added the go add ONLY when ready to merge, run all tests label Jun 25, 2025
Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
@zcin
Copy link
Contributor Author

zcin commented Jun 25, 2025

ping @akyang-anyscale @abrarsheikh for review, this one is pretty straightforward.

@zcin zcin merged commit 9dbc555 into ray-project:master Jun 26, 2025
5 checks passed
@zcin zcin deleted the rewrite-run-user-code-wrapper branch June 26, 2025 15:19
minerharry pushed a commit to minerharry/ray that referenced this pull request Jun 27, 2025
## 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>
lk-chen pushed a commit to alexeykudinkin/ray that referenced this pull request Jun 29, 2025
## 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>
elliot-barn pushed a commit that referenced this pull request Jul 2, 2025
## 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants