remote caching support #10960

tdyas · 2020-10-14T21:24:25Z

Problem

Pants is currently able to run Process requests remotely or to run and cache them locally. It cannot, however, write local results to a remote cache and also consume a remote cache without remote execution.

Solution

Teach Pants how to read and write from a remote cache without remote execution.

Result

Added tests of remote caching functionality.

tdyas · 2020-10-14T21:27:36Z

Caveats:

Setup in context.rs is now even more convoluted. Moreover, I left the cache checks in the remote execution client because of platform choices differing. (Platform::current versus Platform::Linux). Remote caching mode and remote execution mode should probably be made mutually exclusive.
Still working on getting remote_cache_tests::recover_from_missing_store_contents to work. May point at a bug or need to verify all digests are present for a cache result.

tdyas · 2020-10-14T21:35:01Z

src/rust/engine/process_execution/src/remote_cache_tests.rs

+
+#[tokio::test]
+async fn recover_from_missing_store_contents() {
+  env_logger::init();


This is here while testing WIP

tdyas · 2020-10-14T21:35:34Z

src/rust/engine/process_execution/src/remote_cache_tests.rs

+    .unwrap();
+
+  // And that the entire output directory can be loaded.
+  assert!(store


This is failing currently.

tdyas · 2020-10-19T20:14:18Z

Discussed with @gshuflin: Going to refactor context.rs first in a separate PR to clean up how we setup CommandRunner impl's, then will rebase this PR on top of that refactor.

Eric-Arellano · 2020-10-19T20:21:34Z

Discussed with @gshuflin: Going to refactor context.rs first in a separate PR to clean up how we setup CommandRunner impl's, then will rebase this PR on top of that refactor.

That'd be really helpful. Thanks!

### Problem Core::new is a very convoluted function and it is hard to read what is actually going on in it. ### Solution Refactor the store and command runner setup into separate functions. (Also refactors the load of some certificates.) This refactor will make the subsequent introduction of remote caching in #10960 easier to comprehend. ### Result Existing tests pass.

[ci skip-build-wheels]

tdyas · 2020-10-22T04:47:11Z

This is ready for review. I will be adding more tests, but the basic functionality is ready to get reviewed.

coveralls · 2020-10-22T05:39:38Z

Coverage remained the same at 0.0% when pulling 9a1a2e2 on tdyas:remote_caching_support into 8da1680 on pantsbuild:master.

[ci skip-build-wheels]

Eric-Arellano

Awesome. Has this been used in the wild?

src/rust/engine/process_execution/src/remote.rs

Eric-Arellano · 2020-10-22T20:03:22Z

src/rust/engine/process_execution/src/remote_cache.rs

+/// This runner expects to sit between the local cache CommandRunner and the CommandRunner
+/// that is actually executing the Process. Thus, the local cache will be checked first,
+/// then the remote cache, and then execution (local or remote) as necessary if neither cache
+/// has a hit. On the way back out of the stack, the result will be stored remotely and
+/// then locally.


Thanks for the excellent comment! I'm a bit unclear how the execution CommandRunner is different than the local cache CommandRunner? I'm not very familiar with this part of the codebase.

Do you mean the remote execution command runner (process_execution::remote::CommandRunner) or the remote cache command runner (process_execution::remote_cache::CommandRunner)?

I don't understand what this sentence means:

This runner expects to sit between the local cache CommandRunner and the CommandRunner that is actually executing the Process.

It means that the nested "stack" of command runners looks like this:

cache::ComandRunner -> remote_cache::CommandRunner -> local::ComandRunner

The actual execution of the Process is done by local::CommandRunner (the one on the right). But before it is invoked, we first check the local cache and then check the remote cache. On the way back up the stack, the remote_cache::CommandRunner will take the result of any actual invocation performed by local::ComandRunner and write it to the remote Action Cache.

src/rust/engine/process_execution/src/remote_cache.rs

src/rust/engine/process_execution/src/remote.rs

[ci skip-build-wheels]

tdyas requested review from illicitonion, Eric-Arellano and gshuflin October 14, 2020 21:24

tdyas changed the title ~~remote caching support~~ [WIP] remote caching support Oct 14, 2020

tdyas commented Oct 14, 2020

View reviewed changes

tdyas mentioned this pull request Oct 20, 2020

refactor Core::new including command runner setup #10993

Merged

Tom Dyas added 7 commits October 21, 2020 21:38

add remote caching support

fd7d675

[ci skip-build-wheels]

load remote caching command runner

62f7206

[ci skip-build-wheels]

fix test compile

845b173

[ci skip-build-wheels]

skip using cache on errors

414e780

[ci skip-build-wheels]

add test for extracting trees

b27268a

[ci skip-build-wheels]

add test for extracting output file

63d30f5

[ci skip-build-wheels]

remove env_logger

c9903bb

[ci skip-build-wheels]

tdyas force-pushed the remote_caching_support branch from 430f84a to c9903bb Compare October 22, 2020 04:43

tdyas changed the title ~~[WIP] remote caching support~~ remote caching support Oct 22, 2020

tdyas marked this pull request as ready for review October 22, 2020 04:45

fix bug in remote store setup

33c38fc

[ci skip-build-wheels]

Eric-Arellano approved these changes Oct 22, 2020

View reviewed changes

gshuflin reviewed Oct 22, 2020

View reviewed changes

src/rust/engine/process_execution/src/remote_cache.rs Show resolved Hide resolved

gshuflin reviewed Oct 22, 2020

View reviewed changes

src/rust/engine/process_execution/src/remote.rs Show resolved Hide resolved

gshuflin approved these changes Oct 22, 2020

View reviewed changes

capitalize error messages

9a1a2e2

[ci skip-build-wheels]

tdyas merged commit 336ae9a into pantsbuild:master Oct 23, 2020

tdyas deleted the remote_caching_support branch October 23, 2020 17:33

stuhood mentioned this pull request Nov 5, 2020

Allow for a remote-store-only configuration in v2 #9719

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remote caching support #10960

remote caching support #10960

tdyas commented Oct 14, 2020 •

edited

Loading

tdyas commented Oct 14, 2020 •

edited

Loading

tdyas Oct 14, 2020

tdyas Oct 14, 2020

tdyas commented Oct 19, 2020

Eric-Arellano commented Oct 19, 2020

tdyas commented Oct 22, 2020

coveralls commented Oct 22, 2020 •

edited

Loading

Eric-Arellano left a comment

Eric-Arellano Oct 22, 2020

tdyas Oct 22, 2020

Eric-Arellano Oct 22, 2020

tdyas Oct 22, 2020 •

edited

Loading

remote caching support #10960

remote caching support #10960

Conversation

tdyas commented Oct 14, 2020 • edited Loading

Problem

Solution

Result

tdyas commented Oct 14, 2020 • edited Loading

tdyas Oct 14, 2020

Choose a reason for hiding this comment

tdyas Oct 14, 2020

Choose a reason for hiding this comment

tdyas commented Oct 19, 2020

Eric-Arellano commented Oct 19, 2020

tdyas commented Oct 22, 2020

coveralls commented Oct 22, 2020 • edited Loading

Eric-Arellano left a comment

Choose a reason for hiding this comment

Eric-Arellano Oct 22, 2020

Choose a reason for hiding this comment

tdyas Oct 22, 2020

Choose a reason for hiding this comment

Eric-Arellano Oct 22, 2020

Choose a reason for hiding this comment

tdyas Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

tdyas commented Oct 14, 2020 •

edited

Loading

tdyas commented Oct 14, 2020 •

edited

Loading

coveralls commented Oct 22, 2020 •

edited

Loading

tdyas Oct 22, 2020 •

edited

Loading