Split remote providers into their own crates by huonw · Pull Request #19958 · pantsbuild/pants

huonw · 2023-10-01T06:35:07Z

This does a no-functionality-change refactoring of the remote store providers (backed by REAPI and OpenDAL). This splits the concept of a remote provider out into their own internal "surface" crate remote_provider, plus three crates that are (mostly) implementation-details of that one:

remote_provider/remote_provider_traits for the traits (and structs!) that the various providers have to implement
remote_provider/remote_provider_reapi for the gRPC/REAPI-backed provider
remote_provider/remote_provider_opendal for the OpenDAL-backed provider

They're arranged like this:

graph BT
   remote("process_execution/remote (existing)")
   store("fs/store (existing)")
   traits("remote_provider_traits (new)")
   grpc("remote_provider_reapi (new)")
   opendal("remote_provider_opendal (new)")
   selector("remote_provider (new)")
   grpc --> traits
   opendal --> traits
   selector --> grpc
   selector --> opendal
   remote -- for remote execution --> grpc
   remote --> selector
   store --> selector

Theoretically the new crates other than remote_provider are an implementation detail... except there's a helper in remote_provider_reapi that's used for the remote execution in process_execution/remote, in addition to the byte store and action cache, hence the dependency there.

This is one point of #19902, following up on #19827 (comment).

The commits are individually reviewable, although the overall PR view gives a slightly more useful view of the overall file renames for some files.

…rced by coherence)

huonw · 2023-10-03T06:17:15Z

+  async fn get_action_result(
+    &self,
+    action_digest: Digest,
+    build_id: &str,


This has changed from context: &process_execution::Context to just build_id: &str, because that's all that's used and it ensures that this code doesn't need to depend on process_execution.

huonw · 2023-10-03T06:19:00Z

+
+const REAPI_ADDRESS_SCHEMAS: [&str; 4] = ["grpc://", "grpcs://", "http://", "https://"];
+
+// TODO(#19902): a unified view of choosing a provider would be nice


These two functions are very similar, but I haven't tried to merge them as part of this work. There's two closely-related points in #19902 for follow-up ("tweaking the internal Rust details of how the remote providers are built to have less duplication", "improving how the options are fed through").

huonw · 2023-10-03T06:19:58Z


 pub(crate) const STORE_BATCH_API_SIZE_LIMIT: usize = 4 * 1024 * 1024;

-pub fn big_file_fingerprint() -> Fingerprint {


The functions here have moved to testutil so they can be used by the remote_provider_... tests without duplication.

stuhood

Thanks!

tgolsson

Looks overall good, one comment. A bit hard to follow each individual move as I'm not familiar with all the code involved, but it mostly seems sensible.

If I have to make one complaint; it's that the remote_provider/remote_provider_xxx scheme is a bit redundant -- I'd repeat the domain if it wasn't nested, but when nesting it just leads to paths that are overly long -- it's a lot easier to distinguish remote_provider/{src,opendal,reapi,traits}/. And if you're in an shell, tab completion works better.

tgolsson · 2023-10-05T19:18:00Z

 parking_lot = { workspace = true }
 opendal = { workspace = true }
+remote_provider = { path = "../../remote_provider" }
+remote_provider_reapi = { path = "../../remote_provider/remote_provider_reapi" }


This isn't in your graph in the PR description; and it seems maybe a bit leaky if we're just picking up this dependency for apply_headers?

seems maybe a bit leaky if we're just picking up this dependency for apply_headers?

Yeah, I guess theoretically this should be entirely an implementation detail of providers surfaced by remote_provider... except currently the remote execution isn't wrapped up into a "provider" interface, just hard-coded to Bazel REAPI (and I could imagine it will remain this way), so it felt okay to leak here.

This isn't in your graph in the PR description

Oops good catch, fixed and updated the text to call it out too.

huonw · 2023-10-05T21:51:36Z

A bit hard to follow each individual move as I'm not familiar with all the code involved, but it mostly seems sensible.

Yeah, sorry, it's a bit of a mess, especially how it's not just moving files around, but merging/splitting some too!

If I have to make one complaint; it's that the remote_provider/remote_provider_xxx scheme is a bit redundant -- I'd repeat the domain if it wasn't nested, but when nesting it just leads to paths that are overly long -- it's a lot easier to distinguish remote_provider/{src,opendal,reapi,traits}/. And if you're in an shell, tab completion works better.

Yeah, I was in two minds, I definitely am sympathetic to the repetition being annoying... but I thought overall it's better if the crates names are spelled out for the uses etc., so remote_provider_reapi not just reapi, which means name = "remote_provider_reapi" in the Cargo.toml... and, thus the directory names too, to match (this isn't strictly necessary, but felt better to be consistent there).

huonw · 2023-10-05T21:59:23Z

I'm going to merge because I think the broad strokes are good enough here (and it'll be good to unblock downstream work towards #19902), but can iterate as follow-up, depending on the discussion from @tgolsson's points.

Thanks both.

This replaces uses of `RemoteCacheProviderOptions` with `RemoteStoreOptions` (previously `RemoteOptions`): they're very similar, and only separate because they were previously in very different parts of the codebase, but this has changed with #19958. This makes a few additional changes to make the merging smoother: - removes the "capability cell" sharing optimisation, which enabled sharing of the `GetCapabilitiesRequest` gRPC request result between the CAS and the remote execution providers, if both were being used with the same address. I thought that optimising one-time (on start-up) requests from 2 to 1 wasn't worth the complexity of plumbing it through these more generic options, but can restore it if it is. - renames various fields This does meant that there's a few options that are seemingly irrelevant to the action cache providers, e.g. "batch API size limit" is meaningless. I think this is fine: I think it's fundamental to supporting multiple providers that there'll be a grab-bag of options, which some providers support and others do not. The commits are individually reviewable. (This is one point of #19902.)

There's a been a few new crates added recently (#18854, #19958), but we didn't update `[workspace].members` and `[workspace].default-members` in `src/rust/engine/Cargo.toml` to match. In addition, the older `protos` and `grpc_util` weren't listed. This syncs up the lists. The lists of `members` and `default-members` now exactly matches, except for `fs/brfs` as commented. They also match the `Cargo.toml`s that exist on disk.

huonw added the category:internal CI, fixes for not-yet-released features, etc. label Oct 1, 2023

huonw added 12 commits October 2, 2023 17:38

Prep: Factor out big_file and extra_big_file

146fd43

Prep: Simplify tests now that big files are available as TestData

61cab64

Prep: Move mk_tempfile to testfile::file

409f182

Prep: Express StubCAS-prefill more directly in reapi_tests

cc41f05

Sketch crate structure

b78d82a

Move ByteStoreProvider trait to remote_provider_traits

1118756

Move RemoteOptions struct to remote_provider_traits

3ec564a

Move REAPI byte store to remote_provider_reapi

7f97bfa

Move opendal byte store to remote_provider_opendal

5c8ceaa

Move byte store choose_provider to remote_provider

56fc7b4

Prep: move apply_headers to remote_provider_reapi

211b08b

Prep: remove use of process_execution::Context

1876d74

huonw force-pushed the huonw/remote-provider-crates branch from 0d32612 to 419288c Compare October 2, 2023 06:49

huonw added 6 commits October 3, 2023 11:58

Move ActionCacheProvider to remote_provider_traits & opendal impl (fo…

fe785b2

…rced by coherence)

Move RemoteCacheProviderOptions to remote_provider_traits

307c446

Move REAPI action cache to remote_provider_reapi

c9f5e20

Move action cache choose_provider to remote_provider

d453479

Clean-up: de-pub operator in opendal provider

1f926f9

Comments

db46815

huonw force-pushed the huonw/remote-provider-crates branch from 419288c to db46815 Compare October 3, 2023 01:00

huonw commented Oct 3, 2023

View reviewed changes

huonw marked this pull request as ready for review October 3, 2023 06:20

huonw requested review from stuhood, tgolsson and thejcannon October 3, 2023 06:20

huonw mentioned this pull request Oct 4, 2023

Refactoring of remote stores after initial addition of OpenDAL #19902

Open

8 tasks

stuhood approved these changes Oct 5, 2023

View reviewed changes

tgolsson approved these changes Oct 5, 2023

View reviewed changes

huonw merged commit 76f8103 into main Oct 5, 2023

huonw deleted the huonw/remote-provider-crates branch October 5, 2023 21:59

huonw mentioned this pull request Oct 19, 2023

Fix Store benchmarks #20057

Merged

This was referenced Oct 29, 2023

Merge "byte store" and "action cache" provider options #20115

Merged

Refine Rust workspace members for recently added crates #20117

Merged

Navigation Menu

Uh oh!

Uh oh!

Split remote providers into their own crates - #19958

Split remote providers into their own crates#19958
huonw merged 18 commits into
mainfrom
huonw/remote-provider-crates

huonw commented Oct 1, 2023 •

edited

Loading

Uh oh!

huonw Oct 3, 2023

Uh oh!

huonw Oct 3, 2023

Uh oh!

huonw Oct 3, 2023

Uh oh!

stuhood left a comment

Uh oh!

tgolsson left a comment

Uh oh!

tgolsson Oct 5, 2023

Uh oh!

huonw Oct 5, 2023

Uh oh!

huonw commented Oct 5, 2023

Uh oh!

huonw commented Oct 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants


		const REAPI_ADDRESS_SCHEMAS: [&str; 4] = ["grpc://", "grpcs://", "http://", "https://"];

		// TODO(#19902): a unified view of choosing a provider would be nice


		pub(crate) const STORE_BATCH_API_SIZE_LIMIT: usize = 4 * 1024 * 1024;

		pub fn big_file_fingerprint() -> Fingerprint {

Uh oh!

Uh oh!

Conversation

huonw commented Oct 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

huonw Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

huonw Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

huonw Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

stuhood left a comment

Choose a reason for hiding this comment

Uh oh!

tgolsson left a comment

Choose a reason for hiding this comment

Uh oh!

tgolsson Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

huonw Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

huonw commented Oct 5, 2023

Uh oh!

huonw commented Oct 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

huonw commented Oct 1, 2023 •

edited

Loading