https://pantsbuild.org/ logo
a

average-vr-56795

08/18/2021, 3:44 PM
I'm hoping to add a new flag to
process_executor
-
--remote-cache-mode={skip,prefer,speculate,require}
but at the moment
remote_cache::CommandRunner
is hard-coded to do speculation... How would folks feel about: 1. Implementing a
SpeculatingCommandRunner
which takes two `CommandRunner`s and just races them, leaving
remote_cache::CommandRunner
to only worry about interacting with the remote cache itself 2. Introducing this flag to
process_executor
? (I actually only care about the
skip
, and
require
flag values, but I figure completing the matrix is worthwhile while doing so)
w

witty-crayon-22786

08/18/2021, 4:07 PM
works for me. note though that to speculate on a cache, you might have to change the CommandRunner Error type to support non-fatal errors for misses
…oh. but how would you write to the cache with a SpeculatingCommandRunner? it needs the successful result afterwards.
also, not sure what
prefer
or
require
actually mean
a

average-vr-56795

08/18/2021, 4:29 PM
…oh. but how would you write to the cache with a SpeculatingCommandRunner? it needs the successful result afterwards.
Good point - maybe that needs its own write decorator, rather than to be in the same
CommandRunner
as the read
prefer
means "check cache before trying to run at all"
require
means "require there's a cache hit (error if not)" (which can be useful for testing)
w

witty-crayon-22786

08/18/2021, 4:32 PM
got it.
also, fwiw, i think that remote execution currently has its own embedded remote cache lookup, rather than using the remote cache CommandRunner as a wrapper.
a

average-vr-56795

08/18/2021, 5:22 PM
Depends on the remote implementation, I think? Some do it all via Execution, some only do the separate ActionCache interface?
w

witty-crayon-22786

08/18/2021, 5:23 PM
yea. i just meant that the code/cache-lookup is duplicated.
but i suspect that if remote-cache was wrapped around remote-execution, you’d want it to be in
prefer
mode
cc @fast-nail-55400 (whole thread)
a

average-vr-56795

08/18/2021, 5:24 PM
Cool - I will have a closer look tomorrow and try to put something reasonable together 🙂
w

witty-crayon-22786

08/18/2021, 5:27 PM
and … actually, @average-vr-56795: i just remembered that there is backstory on why SpeculatingCommandRunner was removed: my conclusion in https://docs.google.com/document/d/1hS0n6vp-hBxy4Xo-q9QA_elofzQtOubjfN4nSeIgp90/edit# was that we should move the choice to speculate local/remote higher in the stack in order to better support cross-platform builds.
essentially, you’d have a Platform/Host+Platform value for an entire subgraph, and so at CommandRunner time, you’d already have had your fate decided for you
a

average-vr-56795

08/18/2021, 5:28 PM
Aah yes... I think I still lean towards MultiPlatformEPR being a kind of sketchy abstraction to be handing a single CommandRunner, and that the splitting should happen higher in the stack than that...
w

witty-crayon-22786

08/18/2021, 5:29 PM
yep. i think that MultiPlatformProcess can be removed.
a

average-vr-56795

08/18/2021, 5:29 PM
I will refresh myself on that doc and give it a slightly more involved look then 😄
Thanks!
w

witty-crayon-22786

08/18/2021, 5:36 PM
sure thing! happy to help however we can.
i just took another pass over the doc, and i think that it is all still accurate… just queued for when we get the time
f

fast-nail-55400

08/18/2021, 5:56 PM
maybe that needs its own write decorator, rather than to be in the same 
CommandRunner
 as the read
or just a write-only
CommandRunner
— that sounds like a good refactor to do regardless of what else we do, i.e. separation of read and write concerns
👍 1
also, fwiw, i think that remote execution currently has its own embedded remote cache lookup, rather than using the remote cache CommandRunner as a wrapper.
correct, the remote cache and remote execution clients each implement the cache lookup (they call into the same underlying function
check_action_cache
)
no particular reason for both to do it instead of having a “cache check”
CommandRunner
impl, but for the fact that Past Tom probably didn’t want to start messing with the remote execution client too much when writing the remote cache client
👍 1
a

average-vr-56795

08/18/2021, 7:11 PM
Cool cool - thanks for that context! I will probably send some refactor PRs around!
👍 1