https://pantsbuild.org/ logo
Join Slack
Powered by
I'm hoping to add a new flag to `process_executor`...
# development
a
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
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
…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
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
Depends on the remote implementation, I think? Some do it all via Execution, some only do the separate ActionCache interface?
w
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
Cool - I will have a closer look tomorrow and try to put something reasonable together 🙂
w
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
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
yep. i think that MultiPlatformProcess can be removed.
a
I will refresh myself on that doc and give it a slightly more involved look then 😄
Thanks!
w
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
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
Cool cool - thanks for that context! I will probably send some refactor PRs around!
👍 1
Open in Slack
PreviousNext
Powered by