-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
[Core] Add gRPC streaming support by moving to the async callback/reactor API. #15279
[Core] Add gRPC streaming support by moving to the async callback/reactor API. #15279
Conversation
0324b89
to
5e94229
Compare
bazel/ray_deps_setup.bzl
Outdated
url = "https://github.com/grpc/grpc/archive/4790ab6d97e634a1ede983be393f3bb3c132b2f7.tar.gz", | ||
sha256 = "df83bd8a08975870b8b254c34afbecc94c51a55198e6e3a5aab61d62f40b7274", | ||
# url = "https://github.com/grpc/grpc/archive/53ba4a101e80e1a67d4ec741b7e1aad6ea8d790f.tar.gz", | ||
# url = "https://github.com/grpc/grpc/archive/refs/tags/v1.36.4.tar.gz", |
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.
Will remove these alternatives before merging.
src/ray/rpc/client_call.h
Outdated
@@ -287,7 +287,7 @@ class ClientCallManager { | |||
std::atomic<unsigned int> rr_index_; | |||
|
|||
/// The gRPC `CompletionQueue` object used to poll events. | |||
std::vector<grpc::CompletionQueue> cqs_; | |||
std::vector<std::unique_ptr<grpc::CompletionQueue>> cqs_; |
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.
gRPC completion queues are no longer copyable.
c68c6d0
to
2566cfa
Compare
Hey @clarkzinzow Do we still need this PR -> #14598 after yours? |
@rkooo567 That PR will still be necessary until we've ported every gRPC service to use the new gRPC layer, so I think that PR should still be merged. |
Makes sense. Can you remove the tag once tests are in the passing state? |
// capturing `this` since the gRPC-level callback may outlive this | ||
// GrpcCallbackClient instance, while we're more confident that it will not | ||
// outlive the event loop, which should only be destroyed on process exit. | ||
[&io_context = io_context_, response, callback = std::move(callback), ctx, |
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.
The callback is not safe, we need to use shared_form_this to return a shared_ptr of this and capture it in callback to make sure async safe callback.
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.
Hmm I'd agree if we were going to reference a method or member on this client object in the callback, but the only member that we're going to reference is io_context_
, which we pass in to the callback by reference explicitly. The lifetime assumption that is made here is that the IO context will outlive the gRPC-internal nexting thread (where this callback is invoked), which I'm less sure about, but that wouldn't be solved by capturing a this
shared pointer.
const ClientCallback<Reply> &callback, | ||
std::shared_ptr<grpc::ClientContext> ctx = nullptr, | ||
const std::string call_name = "UNKNOWN_RPC") { | ||
auto reactor = new ClientWriterReactor<GrpcService, Request, Reply>( |
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.
Use shared_ptr maybe better, and no need to delete this after callback, more safer.
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.
I didn't go with a shared pointer given that (1) the gRPC stub method expects a raw pointer, (2) the gRPC library guarantees that the reactor will never be used after OnDone()
is called. If we were to use a shared pointer for this, (1) would still require us to give gRPC a raw pointer to the reactor which would result in a leaked pointer in the shared pointer's reference counting and would require us to guarantee that the application-level shared pointer to ClientWriterReactor
outlives the raw pointer held within the gRPC library (otherwise the gRPC library would do a use-after-free), which seems impossible to guarantee.
Given that (2) gives us clear semantics for when the reactor should be deleted, it should be easier to create the reactor as a raw pointer, and explicitly delete itself at the end of OnDone()
(we can document in the reactor API that it shouldn't be used after OnDone()
is called).
I know that this is still not ideal, so definitely lmk if you have any ideas on how to make this reactor lifecycle management better!
2566cfa
to
9a8eed2
Compare
FYI, the callback API is no longer considered to be experimental. |
Current CI failures:
|
216b355
to
bb81807
Compare
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.
It actually looks a lot simpler than I thought, and it definitely makes code smaller!! (which is great!). Here is a couple thoughts;
- Does this use any of existing grpc code? (e.g., client_call.h)? I am asking it because you seem to import the file, but I couldn't find the usage.
- Looks like it is not experimental anymore according to https://github.com/grpc/grpc/pull/25728/files#diff-79410abf9a4d8c0b0cfb8544485bc610c5eadffb1025d1206ab7f0c177e6a635R194, but we still seem to use them? Any reason why we don't use the non-experimental apis?
- Also, I think we can definitely break this down. One thing we can do is;
- only porting Unary RPC implementation first (and test it before merging -> revert) -> 2. Port all RPCs to the Unary RPCs in this layer first -> 3. implement streaming. Ideally, we should have some gap between 2~3 to make sure stability is guaranteed. Another possible approach is to modify the macro a little bit to feature flag them. At least for Unary RPCs, it seems to have the same macro definition.
src/ray/rpc/grpc_callback_client.h
Outdated
} | ||
auto response = std::make_shared<Reply>(); | ||
auto stats_handle = io_context_.RecordStart(call_name); | ||
(stub_->experimental_async()->*method)( |
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.
Looks like this experimental flag is gone now?
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.
Yep, once we update gRPC again we'll be able to use the callback/reactor API from the stable namespace.
#include "ray/common/grpc_util.h" | ||
#include "ray/common/ray_config.h" | ||
#include "ray/common/status.h" | ||
#include "ray/rpc/client_call.h" |
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.
What are we using from this file?
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.
We're only using the ClientCallback
type definition. If we want to remove that dependency, we can duplicate that type definition in this file.
thirdparty/patches/grpc-python.patch
Outdated
@@ -1,23 +1,63 @@ | |||
diff --git third_party/py/python_configure.bzl third_party/py/python_configure.bzl | |||
--- third_party/py/python_configure.bzl | |||
+++ third_party/py/python_configure.bzl | |||
@@ -163,1 +163,1 @@ | |||
@@ -177,7 +177,7 @@ def _get_bash_bin(repository_ctx): |
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.
Can you tell me why we need to change this file?
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.
We updated our gRPC dependency, so these patches were no longer applying to the correct lines, i.e. the correct code.
- name = shared_object_name, | ||
+ name = cc_kwargs.pop("name", shared_object_name), | ||
- srcs = [stem + ".cpp"], | ||
+ srcs = [stem + ".cpp"] + cc_kwargs.pop("srcs", []), |
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.
It makes sense why you added this, but do we need this change in this PR?
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.
We updated our gRPC dependency, so these patches were no longer applying to the correct lines, i.e. the correct code.
Agreed! 😁
It does not use any existing gRPC layer code except for the
The non-experimental APIs are the experimental APIs moved out of the experimental namespace; we can update our gRPC layer to use the non-experimental APIs after we update our gRPC dependency (the APIs were de-experimentalized while this PR was open).
We can definitely break this down, but I should clarify a few things
Since the old gRPC layer only supports unary RPCs, there are only unary RPCs to port! 😄
The macro could be shared, but then application code won't have access to the We could do something fancier for feature flagging, but it will probably require modifying the existing gRPC layer, which I was trying to avoid. Shouldn't be that difficult though. |
73b1a97
to
1c69ae6
Compare
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.
setup changes lgtm as codeowner
1678ec2
to
277ff23
Compare
Switching to gRPC streaming is really cool! But there are still some corner cases we should concern:
|
3d88da0
to
076aa52
Compare
Maybe we could replace the ugly macros with generic templates, i think coroutine will be the ultra improvement of async callbacks, the async code will be much more simple and clean. |
Agreed on both points! I didn't deviate from the macros or the asio event loop model since I'm hoping to keep this PR small, but we're hoping to do both soon. |
|
… proxy for the asio event loop's lifetime.
6372201
to
f70d662
Compare
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.
|
Hi again! The issue will be closed because there has been no more activity in the 14 days since the last message. Please feel free to reopen or open a new issue if you'd still like it to be addressed. Again, you can always ask for help on our discussion forum or Ray's public slack channel. Thanks again for opening the issue! |
1 similar comment
Hi again! The issue will be closed because there has been no more activity in the 14 days since the last message. Please feel free to reopen or open a new issue if you'd still like it to be addressed. Again, you can always ask for help on our discussion forum or Ray's public slack channel. Thanks again for opening the issue! |
Why are these changes needed?
Our current gRPC layer only supports unary RPCs, while some of our current communication patterns, such as object location subscriptions, actor method calls, and within-lease task submissions, and future communication patterns, such as a non-Redis GCS-based pub/sub, are best suited to streaming RPCs. Streaming RPCs take full advantage of HTTP/2, streaming requests and/or responses within an existing HTTP/2 request/response, ridding ourselves of the overhead of e.g. creating a new HTTP/2 request for each unary call. Moreover, we have ordering guarantees within a single RPC, which saves us from RPC reordering issues that we may encounter when using unary RPCs that would normally have to be handled in the application-level protocol.
The current async gRPC layer is implemented using the completion queue API, where both the client and the server implement RPC call abstractions that transition through a state machine representing stages of a unary RPC call. Although we could attempt to extend this completion queue state machine to support streaming RPCs, this PR adds support for gRPC streaming by creating a new gRPC layer that uses the experimental (but soon to be stable) async callback/reactor API. This provides a gRPC-native callback API for unary RPCs, and a reactor API for each of the streaming RPCs. Although there isn't a callback API for the server-side of unary RPCs, we wrap the unary reactor API with a callback API ourselves, so the common unary RPC case can be entirely implemented with callbacks at the application-level, and is defined via macros such as:
UNARY_CALLBACK_RPC_CLIENT_METHOD(NodeManagerService, RequestResourceReport, grpc_client_, )
on the client and
UNARY_CALLBACK_RPC_SERVICE_HANDLER(NodeManagerService, RequestResourceReport)
on the server, maintaining application-level API compatibility with the completion queue gRPC layer for unary RPCs. This allows us to gradually port each gRPC client and server to the new callback-based layer by only updating the gRPC client and server (i.e. macro) definitions, with no application-level changes necessary. Also note that this is entirely a layer on top of gRPC without any observable transport-level effects, meaning that the client and server for a given gRPC service can be ported independently. Beyond being simpler to implement than extending the completion queue state machine, this should eliminate a lot of technical debt in our current gRPC layer and allow us to take advantage of upstream work on this callback/reactor API (which is active!) instead of maintaining and extending our own completion queue --> callback layer.
Note that this PR (currently) ports the raylet gRPC client and server to the callback API. This is primarily to demonstrate the API and that there are no performance regressions. If we wish, we could merge this PR without porting any of the gRPC services to the callback API and wait until we think the layer is stable (either gating behind a feature flag or removing the port entirely for now).
Streaming Reactor API Design
See the gRFC for the async callback API design and see the de-experimentalization PR for examples. Both the client and server macros provide a thin layer that essentially exposes these APIs to the application layer. An end-to-end streaming RPC example will be implemented in a future PR.
Risks
num_cores
/ 2, clamped to the range [2, 16]) or how many completion queues to use (currently set to 1). Compared to the completion queue implementation, where we can control both the number of completion queues and the number of nexting threads, this puts us in quite an opinionated box that could result in performance regressions in some cases (e.g., taking valuable CPU resources from the core worker). However, preliminary benchmarking (with the raylet using the new gRPC layer) has actually shown performance improvements over our existing CQ-based gRPC layer. This is possibly due to thenum_cores
/ 2 nexting threads being a much more aggressive gRPC-level resource allocation than what is done when using the completion queue layer.RayletClient
orGcsClient
) that would handle the reactors themselves and expose transport-agnostic APIs.Other Niceties
TODOs
test_global_gc
on Linux, a few tests on MacOS).GrpcClient
andGrpcServer
interface (could explore this more).Related issue number
Closes #15219
Checks
scripts/format.sh
to lint the changes in this PR.