[serve] Warn about upcoming change to run sync code on executor thread #44403
Assignees
Labels
enhancement
Request for new feature and/or capability
P1
Issue that should be fixed within a few weeks
ray-2.11
serve
Ray Serve Related Issue
Comments
edoakes
added
enhancement
13 tasks
jecsand838
pushed a commit
to jecsand838/ray
that referenced
this issue
Dec 4, 2024
## Why are these changes needed? Adds a feature flag to run sync user-defined methods in a threadpool by default. This matches the existing behavior when using a FastAPI ingress. This should address a lot of user confusion and make it easier to write performant code by default. For example, just sticking a torch model call in a sync method will now provide reasonable performance out of the box. However, there may be some existing user code that is not thread safe, so we need to do a gentle migration. This PR introduces the behavior behind a feature flag and warns users about the upcoming change and how to opt into the new behavior or maintain existing behavior once it does (just adding `async def` will do it). I've opted to set the max thread pool size to `max_ongoing_requests`, which seems like a reasonable policy. If needed we can add a user-facing API for this in the future. TODO before merging: - [x] Get it working for sync generators. - [x] Add warning for default change (people can keep behavior by changing to async def). - [x] Add/update UserCallableWrapper tests. - [x] Add/update some integration tests (verify that request context is set correctly!). - [x] Set maximum thread pool size. ## Related issue number Closes ray-project#44354 Closes ray-project#44403 Closes ray-project#48903 --------- Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com> Signed-off-by: Connor Sanders <connor@elastiflow.com>
dentiny
pushed a commit
to dentiny/ray
that referenced
this issue
Dec 7, 2024
## Why are these changes needed? Adds a feature flag to run sync user-defined methods in a threadpool by default. This matches the existing behavior when using a FastAPI ingress. This should address a lot of user confusion and make it easier to write performant code by default. For example, just sticking a torch model call in a sync method will now provide reasonable performance out of the box. However, there may be some existing user code that is not thread safe, so we need to do a gentle migration. This PR introduces the behavior behind a feature flag and warns users about the upcoming change and how to opt into the new behavior or maintain existing behavior once it does (just adding `async def` will do it). I've opted to set the max thread pool size to `max_ongoing_requests`, which seems like a reasonable policy. If needed we can add a user-facing API for this in the future. TODO before merging: - [x] Get it working for sync generators. - [x] Add warning for default change (people can keep behavior by changing to async def). - [x] Add/update UserCallableWrapper tests. - [x] Add/update some integration tests (verify that request context is set correctly!). - [x] Set maximum thread pool size. ## Related issue number Closes ray-project#44354 Closes ray-project#44403 Closes ray-project#48903 --------- Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com> Signed-off-by: hjiang <dentinyhao@gmail.com>
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Follow fastapi semantic. This is a breaking behavior change so we will need to warn at least a release ahead.
The text was updated successfully, but these errors were encountered: