create append-only caches in remote execution #17290
Conversation
| caches: &BTreeMap<CacheName, RelativePath>, | ||
| base_path: &str, | ||
| ) -> Result<String, String> { | ||
| let mut script = String::new(); |
There was a problem hiding this comment.
Probably over-optimization, but maybe pre-allocate based on the length of caches?
There was a problem hiding this comment.
Or just preallocate something like 512 bytes.
| for (cache_name, path) in caches { | ||
| writeln!( | ||
| &mut script, | ||
| "mkdir -p '{}/{}'", |
There was a problem hiding this comment.
Do you know how this will interact with Pants messing with the PATH sometimes?
There was a problem hiding this comment.
This should probably get the mkdir and /bin/sh paths from a BinaryPathRequest.
There was a problem hiding this comment.
Or at least hardcode to /bin/mkdir for an initial implementation. Or maybe let users define the paths in remote_environment targets (to accommodate Docker containers)? Using BinaryPathRequest may be hard to do with chicken and egg?
There was a problem hiding this comment.
Yes, definitely a "chicken and egg" problem. it is unclear to me how to trigger a BinaryPathRequest from this part of the Rust code.
There was a problem hiding this comment.
Another open question: Should failure to symlink the append-only cache fail the Process as well?
There was a problem hiding this comment.
Maybe a failure fails all of Pants, and we instruct the user to disable this mechanism by unsettling the global option? We could mention that they can open a GitHub issue if they need more flexibility like setting /bin/mkdir
| "TODO-DEV: Append-only caches and working directory cannot be set together in remote execution.".into() | ||
| ); | ||
| } | ||
| // TODO-WIP: Make the base path configurable. |
There was a problem hiding this comment.
I'd recommend a global option, not the remote_environment target. That's because we use the same remote server for all remote_environment targets, and it seems desirable for them to share the same named cache. Also simpler modeling for users imo.
There was a problem hiding this comment.
This behavior also needs to be disableable, since not all remote execution environments will support it (some will fail instead).
FWIW: I still think that I prefer explicit support in the backend rather than doing this in the client.
tdyas
force-pushed
the
remoting_append_only_caches
branch
from
d3418be to
3df7898
Compare
… tuple (#17358) Refactor `make_execute_request` to return a struct instead of tuple so that (1) it is easier to refer to the returned instances by a more descriptive name than `.2`; (2) avoid call sites needing to know the order in which the values are returned if they are unpacking the result; and (3) make it easier to add new fields later (for example, the `input_root_digest` field to be added by #17290).
… `DirectoryDigest` (#17370) Modify `make_execute_request` to be async, take a `Store` as a parameter, and to return the input root to use. This is prework for #17290 which needs the ability to make a new input root digest if a wrapper script needs to be added to the input root. (It will also be useful for other wrapper script applications, such as to solve the issue at #17367 (comment) for remote execution.)
tdyas
force-pushed
the
remoting_append_only_caches
branch
from
0c43a67 to
653f016
Compare
tdyas
changed the title
|
The latest commit rebases the PR on top of the pre-work. It also makes the wrapper for append-only caches both configurable and optional. |
| base_path: &str, | ||
| ) -> Result<String, String> { | ||
| let mut script = String::new(); | ||
| writeln!(&mut script, "#!/bin/sh").map_err(|err| format!("write! failed: {err:?}"))?; |
There was a problem hiding this comment.
Note: This assumes /bin/sh exists in the remote environment.
|
The |
tdyas
force-pushed
the
remoting_append_only_caches
branch
from
7869eb3 to
7c0024f
Compare
|
Merging so that I can cherry-pick to 2.15 to unblock some work. We can fix anything in followups. Thanks Tom! |
Use a wrapper script to create append-only caches in remote execution.
Use a wrapper script to create append-only caches in remote execution.