https://pantsbuild.org/ logo
Join Slack
Powered by
<@U03P5521VBJ> it seems like you wrote majority of...
# development
g
@broad-processor-92400 it seems like you wrote majority of the rust code related with grpc stuff aka tonic 
Request::new
-- I was curious if you can help me understand if there is a way to set the max_send_message_length/max_receive_message_length. I just need some hints 🙂
I'm essentially trying to set this https://docs.rs/tonic/latest/tonic/#max-message-size
I want to set it lower, to like 3MB.
b
Thanks for diving in. Unfortunately I've touched all the remote caching stuff except the grpc specific code. My appearance in git blame is likely because I moved a bunch of code around. But I can still help, gimme a second 🙂
❤️ 1
g
I’m in hacker mode just trying to poke at this to see if I can fix my own problem.
Unfortunately it seems like 4MB is the standard across gRPC and for some reason I’m still getting errors related to messages being larger than 4MB. I’m totally lost and don’t really know where to begin. I started by increasing the max message size on the server and that didn’t work. So I’m now trying to lower the client side.
👍 1
Oddly when I increased the server side I actually got a new error.
But still similar in that it was too large. I’m wondering if it’s actually resource related because the generic gRPC docs for the error have to do with resource exhaustion and it said something like disk space as an example.
🤔 1
b
I can still help
Hm, I'm not sure I can... I can't find if/where tonic manages these limits 😞 https://stackoverflow.com/q/77209989/1256624 references a 
max_encoding_message_size
function that doesn't seem to exist any more 🤷
Ah, no, it does exist in 0.10.2 that pants uses, I was just looking at too-old source
🫠 1
g
I just realized a can also take a packet capture to try to better understand what’s going on.
b
Ah yeah, that might provide insight, especially if you can point a protobuf decoder at it (I'm guessing you've found the definitions at 
src/rust/engine/protos/protos/bazelbuild_remote-apis/build/bazel/remote/execution/v2/remote_execution.proto
and similar). BTW, I haven't been able to find how one gets an instance of 
tonic::client::Grpc
from within Pants to be able to call 
max_encoding_message_size
👍 1
Oh, apparently every (code-generated) client like 
ActionCacheClient
gets that method. I found this by running 
./cargo doc --open
in the pants repo, and then searching for 
max_encoding_message_size
.
🙌 1
g
btw, this is what I was referencing for the resource exhasted error message
c
I have zero experience with that part of the code, but if you didn't see it before there are a few seemly related threads about the max size: https://pantsbuild.slack.com/archives/C046T6T9U/p1683573624046389
g
@curved-manchester-66006 I appreciate you highlighting that others are running into the exact same issue. This gives me more motivation to get to the root cause and hopefully document and/or fix as appropriate.
It would be helpful if someone can help me understand if this error message is communicating if the client or the server is sending a message larger than the limit.
Copy code
15:52:55.15 [WARN] Failed to write to remote cache (1 occurrences so far): ResourceExhausted: "grpc: trying to send message larger than max (5790600 vs. 4194304)"
The thing that is failing to get cached is 180,548 files under a node_modules directory, produced by yarn install.
b
Ah, I wonder if it's attempting to write a huge protobuf entry for a directory/tree, and the code is implicitly assuming that directories will always be under the single-request limits, somewhere? Can you hack up pants to set 
max_encoding_message_size
on every client and see we can localise the error better?
g
So I just hooked up a gRPC proxy and have the call that’s so big. Watching my son right now and away from computer. Will let you know the specific call when I get back. Also the proxy is complaining about a parsing error when loading the proto file in the pants repo.
b
Ah, cool, request interception works too. No rush 🙂
Bummer about the proto parse errors
g
@broad-processor-92400 this is the call: /build.bazel.remote.execution.v2.ContentAddressableStorage/FindMissingBlobs
I'm assuming it's a fat list of file hashes, but I don't know 🤷
b
haha! yeah, that'd be it. The Pants functions to do those calls is called 
list_missing_digests
g
Does that chunk up requests or just send it?
b
src/rust/engine/fs/store/src/lib.rs
and 
src/rust/engine/remote_provider/remote_provider_reapi/src/byte_store.rs
are the places of interest. The latter seems to just plop it all in one big request: 
blob_digests: digests.into_iter().map(|d| d.into()).collect::<Vec<_>>()
.
👍 1
g
parsing error for context
Copy code
Parameter grpc: Parsing error:    --> 114:32
    |
114 |     option (google.api.http) = { post: "/v2/{instance_name=**}/actions:execute" body: "*" };
    |                                ^---
    |
    = expected constant
I think it's this tool proxide
b
yep. As you hinted, a possible fix would be having the first one chunk up the requests and send multiple, as required.
g
I wonder if streaming would be better/easier? The implementation might be harder though.
Unfortunately I've never written rust and this is way over my head. Even with chatgpt, I'm lost 🤣
I've also never used protobuf.
b
Hehe, uphill learning experience. No worries. Can you file an issue with the learnings here?
(or update an existing one,if there is one)
g
@broad-processor-92400 based on you experience is this clear that it is a bug in pants not honoring the 4MB limit?
b
Yes, I think so. The protobuf subset in question is
Copy code
syntax = "proto3";
message Digest {
  string hash = 1;
  int64 size_bytes = 2;
}
message FindMissingBlobsRequest {
  string instance_name = 1;
  repeated Digest blob_digests = 2;
}
According to https://www.protobufpal.com, a message like 
{"blob_digests": [{"hash":"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", "size_bytes": 1000}]}
encodes to about 70 bytes (and similarly if the value is repeated), so 180548 potential digests * 70 bytes = 12.05 MiB is an upper limit on the size of the request there. There may be digests that are already available on your local system or files with identical contents (which would result in a repeated digest), so requesting fewer than 180548 to make ~5.5MiB seems plausible. Plus, there's no part of the code there that indicates a very long list would be broken up.
g
I’m going to see if I can pull the request in wireshark since this tool isn’t working to parse proto files.
@broad-processor-92400 how does this actually work? Does the server side need to know about these proto files? I’m wondering if setting up streaming would work
b
I believe the servers are operating off these (or compatible) proto files, yes. I don't know how streaming works with grpc/protobufs in general or with tonic/tower specifically; I haven't actually ever used it in anger (my only exposure is refactoring this part of the pants code)
g
I ended up contributing this to tonic to help identify the root of the problem.
⭐ 2
b
Cool! BTW, it'd still be good to have an issue against pants, if you have a moment for that (even if it turns out our current theory isn't correct, or it isn't actually an issue in pants, that's fine; issues are cheap 😄).
g
It is an issue in pants and I won't forget to file it. After running pants with the modified tonic I was able to identify that it is indeed an issue with a misconfigured use of the tonic library. I was struggling to clearly articulate the problem because I didn't fully know what was going on and couldn't explain discrepancies between what I was reading here and what I was running into -- I couldn't get the described solution to work at all. It was confusing. I was going back and forth between wondering why it worked for someone in the pants community about a year ago but not now. It turns out tonic introduced this as a feature and pants updated the tonic dependency.
👍 1
Oh, and to be clear: I modified tonic here and overrode the max message receive size default from 4MB to 25MB and validated that it fixes the problem. It also highlighted that it's very inefficient to cache 180K files 🤣 -- This all led to me pivoting away from the output of adhoc_tool with 180K raw files to use 
archive_file
instead. I'm still glad I got to the bottom of this; it feels good.
OK, I finally posted a bug here
🙏 1
👍 1
Testing this out now
Copy code
diff --git a/src/rust/engine/remote_provider/remote_provider_reapi/src/byte_store.rs b/src/rust/engine/remote_provider/remote_provider>
index 839ee07d26..e9f7bb6955 100644
--- a/src/rust/engine/remote_provider/remote_provider_reapi/src/byte_store.rs
+++ b/src/rust/engine/remote_provider/remote_provider_reapi/src/byte_store.rs
@@ -92,9 +92,10 @@ impl Provider {
             Some((options.timeout, Metric::RemoteStoreRequestTimeouts)),
         );
 
+        let limit = 25 * 1024 * 1024;
         let byte_stream_client = Arc::new(ByteStreamClient::new(channel.clone()));
 
-        let cas_client = Arc::new(ContentAddressableStorageClient::new(channel.clone()));
+        let cas_client = Arc::new(ContentAddressableStorageClient::new(channel.clone()).max_decoding_message_size(limit));
 
         let capabilities_client = Arc::new(CapabilitiesClient::new(channel));
4 Views
Open in Slack
PreviousNext
Powered by