Snapshot is backed by LMDB not tar files #5496
Conversation
This is much more incremental, and will help us to support remote execution (where Directory protos are the lingua franca). There are still several significant things to clean up here: * Some IsolatedProcess tests are disabled * We need to work out how to do input/output conversions for IsolatedProcess for real * Process execution is hard-coded to be local, synchronous, and a little panic-y but as this is already a very large change, and the process execution pieces aren't actually used in production codepaths, I'd like those to be follow-ups.
illicitonion
force-pushed
the
dwagnerhall/itywip
branch
from
9ea9af3
to
b40ed4e
Compare
src/rust/engine/fs/src/store.rs
Outdated
| // Without this flag, each time a read transaction is started, it eats into our transaction | ||
| // limit (default: 126) until that thread dies. | ||
| // | ||
| // This flag makes transactions are removed from that limit when they are dropped, rather |
| @@ -60,7 +62,13 @@ If unspecified, local execution will be performed.", | |||
| }; | |||
| let server = args.value_of("server"); | |||
|
|
|||
| let request = process_execution::ExecuteProcessRequest { argv, env }; | |||
| // TODO: Capture the working directory as a snapshot, and materialize it to a temporary directory. | |||
There was a problem hiding this comment.
Would that be used somewhere? I'm not sure where... Seems more likely that we'd want to take a (list) of input snapshots as argument (depending on the snapshot manipulation API)? Either way, not important now.
src/rust/engine/src/context.rs
Outdated
| @@ -120,6 +105,12 @@ impl Context { | |||
| } | |||
| } | |||
|
|
|||
| impl StoreFileByDigest<Failure> for Context { | |||
There was a problem hiding this comment.
I think VFS is implemented in nodes.rs... unclear what the pros/cons of splitting impls across files would be, beyond reducing cross-file imports, so *shrug.
src/rust/engine/src/nodes.rs
Outdated
|
|
||
| // TODO: Make this much less unwrap-happy as part of moving this to run off-thread and | ||
| // asynchronously. | ||
|
|
There was a problem hiding this comment.
There might also be a potential solution involving interning Snapshot objects by digest on the rust side, and then keeping the python object more svelte? ... depends on the Snapshot manipulation API more than the threading, per-se.
It might be worth opening a Snapshot manipulation API ticket soon and then doing some design sketching there.
There was a problem hiding this comment.
Filed #5502 and linked to it there
src/rust/engine/src/nodes.rs
Outdated
| let tmpdir = TempDir::new("process-execution").unwrap(); | ||
| // TODO: Materialize the snapshot into the tmpdir when ExecuteProcessRequest has a Snapshot. | ||
| // TODO: Make this much less unwrap-happy as part of moving this to run off-thread and | ||
| // asynchronously. |
There was a problem hiding this comment.
ditto snapshot manipulation reference
There was a problem hiding this comment.
Filed #5502 and linked to it there
This reverts commit e3c38ba.
This reverts commit e3c38ba.
* Revert "Use rust logging API (#5525)" This reverts commit 1150d09. * Revert "Fix up remote process execution (#5500)" This reverts commit 628c83d. * Revert "Remote execution uploads files from a Store (#5499)" This reverts commit 935086b. * Revert "Remote command execution returns a Future (#5497)" This reverts commit fbff143. * Revert "Snapshot is backed by LMDB not tar files (#5496)" This reverts commit e3c38ba.
This is much more incremental, and will help us to support remote
execution (where Directory protos are the lingua franca).
There are still several significant things to clean up here:
IsolatedProcess for real
little panic-y
but as this is already a very large change, and the process execution
pieces aren't actually used in production codepaths, I'd like those to
be follow-ups.